-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(apigatewayv2-integrations): http api - support for request parameter mapping #15630
feat(apigatewayv2-integrations): http api - support for request parameter mapping #15630
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this PR. Sorry for the delay.
So nice to review a README change before seeing all the code that implements it.
Some comments below. Let me know what you think.
mappingValue: MappingValue.requestHeader('header1'), | ||
}) | ||
.addParameter({ | ||
mappingKey: HttpMappingKey.removeHeader(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose you meant to say HttpMappingKey.removeHeader('header1')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I will fix that!
Co-authored-by: Niranjan Jayakar <[email protected]>
Co-authored-by: Niranjan Jayakar <[email protected]>
Co-authored-by: Niranjan Jayakar <[email protected]>
@nija-at I think I have adressed all your comments now, should I go ahead with a first iteration of the implementation? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nija-at I've made a first attempt at implementation, only in the base HttpIntegration at this point. Would like some feedback on this before I continue with the higher level construct for the ALB. Creating the mappings is slightly different from what we discussed, you would now create parameter mappings like this
The idea was to lay the foundation which can later be reused when adding support for parameter mappings with integration subtypes (not in this PR). The same structure can then be used without much change, maybe it would look something like this for SQS
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dan-lind. See my comments below.
readonly value: IMappingValue, | ||
}; | ||
|
||
export class ParameterMapping { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move all of these classes and interfaces into their own file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I still should? Only one class left
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think you still should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should also move into the base apigateway
module.
export interface IMappingKey { | ||
key: string; | ||
}; | ||
|
||
export interface IMappingValue { | ||
value: string; | ||
}; | ||
|
||
export class MappingKey implements IMappingKey { | ||
public static custom(mappingKey: string) {return new MappingKey(mappingKey); } | ||
protected constructor(public readonly key: string) {} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear why we need these classes and interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
const httpEndpoint = new HttpApi(stack, 'HttpProxyPrivateApi', { | ||
defaultIntegration: new HttpAlbIntegration({ | ||
listener, | ||
requestParameters: new RequestParameters() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be ParameterMapping
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
.addParameter({ | ||
mappingKey: MappingKey.custom('new.header'), | ||
mappingValue: MappingValue.custom('new.value'), | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just
new ParameterMapping().custom('new.header', MappingValue.custom('new.value'))?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's not fixed though? It's still the same in the README.
EDIT: Ahh, I see you fixed the API but not the README,
@@ -174,6 +175,18 @@ describe('HttpRoute', () => { | |||
connectionType: HttpConnectionType.VPC_LINK, | |||
uri: 'some-target-arn', | |||
secureServerName: 'some-server-name', | |||
parameterMapping: new ParameterMapping( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a separate unit test for parameter mapping instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
[ | ||
{ | ||
key: HttpMappingKey.appendHeader('header2'), | ||
value: MappingValue.requestHeader('header1'), | ||
}, | ||
{ | ||
key: HttpMappingKey.removeHeader('header1'), | ||
value: MappingValue.NONE, | ||
}, | ||
], | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the API we decided, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See latest changes
@nija-at Haven't been able to look at this for a while, but I've pushed some updates now, let me know if this addresses the concerns you had. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me @dan-lind
Can you add docs, get the build to succeed and update the PR description? This should be close to shipping with one more iteration.
.addParameter({ | ||
mappingKey: MappingKey.custom('new.header'), | ||
mappingValue: MappingValue.custom('new.value'), | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's not fixed though? It's still the same in the README.
EDIT: Ahh, I see you fixed the API but not the README,
const httpEndpoint = new HttpApi(stack, 'HttpProxyPrivateApi', { | ||
defaultIntegration: new HttpAlbIntegration({ | ||
listener, | ||
requestParameters: new RequestParameters() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
readonly value: IMappingValue, | ||
}; | ||
|
||
export class ParameterMapping { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think you still should.
readonly value: IMappingValue, | ||
}; | ||
|
||
export class ParameterMapping { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should also move into the base apigateway
module.
924c117
to
ebfd5f2
Compare
Sorry, not a lot of bandwidth currently, but I've tried to address your comments. I think what's left is also the actual implementation in the higher level constructs in the |
Couldn't find anything in the docs that suggests that there are any limits to where the parameter mapping is valid, so I've added them for all higher level constructs in the I´ve added docs and tests as well. Hopefully close to complete now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@nija-at Build failed after you approved, probably was too much behind master. I've synced with master now, can you please approve again? |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…eter mapping (aws#15630) ---- This is an initial PR as discussed with @nija-at in an attempt to describe the user experience for supporting parameter mapping. This PR will only support parameter mappings for HTTP APIs _without_ an integration subtypes, but it will provide interfaces that can (and probably should) be reused when adding support for integration subtypes as well. Since it also provides the possibility to provide custom key/value pairs for maximum flexibility, it can support and integration subtype although it requires a bit more work on the user side. closes aws#15293 *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This is an initial PR as discussed with @nija-at in an attempt to describe the user experience for supporting parameter mapping.
This PR will only support parameter mappings for HTTP APIs without an integration subtypes, but it will provide interfaces that can (and probably should) be reused when adding support for integration subtypes as well. Since it also provides the possibility to provide custom key/value pairs for maximum flexibility, it can support and integration subtype although it requires a bit more work on the user side.
closes #15293
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license