Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support httpApi payload override on handler #1312

Merged
merged 2 commits into from Dec 3, 2021
Merged

Support httpApi payload override on handler #1312

merged 2 commits into from Dec 3, 2021

Conversation

shalvah
Copy link
Contributor

@shalvah shalvah commented Dec 2, 2021

Description

This PR adds support for getting the payload version for httpApi functions from the function definition (ie overriding the provider level).

Motivation and Context

According to the serverless docs, you can specify httpApi.payload at either the service or function level. However, serverless-offline was ignoring the value set at the function level. This PR fixes that.

How Has This Been Tested?

I've tested this in my current project.

Copy link
Collaborator

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shalvah great thanks for that PR. It's a great call, still please see my comment

? service.provider.httpApi.payload
: '2.0'
if (functionDefinition.httpApi && functionDefinition.httpApi.payload) {
httpEvent.http.payload = functionDefinition.httpApi.payload;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe in that case eventual setting on httpEvent should take precedence

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

httpEvent? Do you mean the provider setting? If so, isn't that counterintuitive?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, ignore my comment. There's no setting on event directly (it obviously wouldn't make sense :)

@medikoo medikoo added the bug label Dec 2, 2021
Copy link
Collaborator

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you ! That's a great fix

? service.provider.httpApi.payload
: '2.0'
if (functionDefinition.httpApi && functionDefinition.httpApi.payload) {
httpEvent.http.payload = functionDefinition.httpApi.payload;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, ignore my comment. There's no setting on event directly (it obviously wouldn't make sense :)

Copy link
Collaborator

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shalvah I wanted to merge it, but CI reported an issue with style formatting.

All changes need to respect Prettier formatting. Can you update the PR?

@shalvah
Copy link
Contributor Author

shalvah commented Dec 2, 2021

Fixed.

I imagine there may be some other overrides currently being overlooked like this. I'll send in a PR if I come across any.

Copy link
Collaborator

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@medikoo medikoo merged commit 8db63dd into dherault:master Dec 3, 2021
@shalvah shalvah deleted the http-payload-override branch February 13, 2022 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants