feat(apigateway): add methodresponse support #1572

john-shaskin · 2019-01-18T17:41:17Z

Add MethodResponse type and the ability to add one or more method responses, when creating an API gateway REST API method.

Pull Request Checklist

Testing
- Unit test added
- CLI change?: manually run integration tests and paste output as a PR comment
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

sam-goodwin

Thanks for the pull! I have reservations about shipping this without support for Models.

sam-goodwin · 2019-01-18T19:07:42Z

packages/@aws-cdk/aws-apigateway/lib/methodresponse.ts

+    /**
+     * The method response's status code, which you map to an IntegrationResponse.
+     */
+    responseModels?: { [destination: string]: string };


If we're going to implement L2 support for method responses, I think we should also support Model, since the value of each key here should be a reference to https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigateway-model.html

sam-goodwin · 2019-01-18T19:08:17Z

packages/@aws-cdk/aws-apigateway/test/test.method.ts

+            },
+            responseModels: {
+              'application/json': 'Empty',
+              'text/plain': 'Empty'


Do you have a use-case where you'll be hard-coding the model names or creating them as part of your stack?

I don't. My current use-case is just using the default Empty model. Maybe this is worth at least adding an IModel interface, as @RomainMuller mentioned below, and perhaps having some default Empty implementation.

sam-goodwin · 2019-01-18T19:09:53Z

packages/@aws-cdk/aws-apigateway/lib/methodresponse.ts

+    responseParameters?: { [destination: string]: boolean };
+
+    /**
+     * The method response's status code, which you map to an IntegrationResponse.


This is a mapping of content type to model name, not the status code.

Totally grabbed the wrong description from the docs 😛 Will fix that.

RomainMuller

I agree with @sam-goodwin, it would be awesome if we can push this a little further and have basic support for models, at least to the point the APIs here can be reasonably expected not to change once we make a more comprehensive L2 support for model definition. Maybe it's just a matter of introducing an IModel interface and passing this instead of the model names...

john-shaskin · 2019-01-21T19:38:48Z

Working on getting base level support for referencing an IModel, instead of passing in a hard-coded string for the model name. Just working out some kinks around being able to reference the default Empty or Error models, when you're not defining your own.

rix0rrr · 2019-01-21T22:31:56Z

Just working out some kinks around being able to reference the default Empty or Error models, when you're not defining your own.

If I may make a suggestion, a default class of class EmptyModel extends IModel might do the trick.

john-shaskin · 2019-01-22T00:43:54Z

Just working out some kinks around being able to reference the default Empty or Error models, when you're not defining your own.

If I may make a suggestion, a default class of class EmptyModel extends IModel might do the trick.

That'll work. I was probably overcomplicating the association to the RestAPI in my head, and thinking I should deal with that. But, it sounds like that's not necessary.

john-shaskin · 2019-01-23T16:52:34Z

I've added a start at models, with IModel. I haven't tried to add full support for models yet. I may look into that next.

john-shaskin · 2019-02-04T05:08:36Z

@RomainMuller and/or @sam-goodwin, any chance you guys have some time to look at the updates on this?

eladb · 2019-02-04T10:34:18Z

packages/@aws-cdk/aws-apigateway/lib/method.ts

@@ -34,11 +35,15 @@ export interface MethodOptions {
   */
  apiKeyRequired?: boolean;

+  /**
+   * The responses that can be sent to the client who calls the method.
+   */


Add a "@default" clause and describe the behavior if this is not defined

Sure. I can add that. I'll also look at doing the same for the properties of the MethodResponse type itself.

eladb · 2019-02-04T20:50:16Z

assigning to @sam-goodwin

john-shaskin · 2019-02-05T16:39:16Z

@sam-goodwin, I've augmented the documentation around MethodResponse, as per @eladb's ask.

sam-goodwin · 2019-02-05T20:48:05Z

packages/@aws-cdk/aws-apigateway/lib/model.ts

+    readonly modelId: string;
+}
+
+export class EmptyModel implements IModel {


Please add docs and provide a link to official documentation describing the keywords 'Empty' and 'Error'.

I've added some documentation around both the Empty and Error models. As far as reference links, I was able to find some mention in the official docs for the Empty model, but not specifically the Error model (outside of mentioning that Empty and Error are available by default). Are you aware of a page in the documentation that addresses the Error model in any further detail?

eladb · 2019-02-06T14:27:22Z

packages/@aws-cdk/aws-apigateway/lib/model.ts

+    public readonly modelId = 'Error';
+}
+
+// TODO: Implement Model, enabling management of custom models.


Update README roadmap section

I'm adding an issue to track the lack of support for API gateway models, and will quote that in the api-gateway readme.

eladb · 2019-02-06T14:29:30Z

packages/@aws-cdk/aws-apigateway/test/test.method.ts

+            responseParameters: {
+              'method.response.header.errthing': true
+            },
+            responseModels: {


I am wondering if a bunch of static properties on Model to make it “enum-like” would make a nicer and more discoverable API here: “Model.Empty” and “Model.Error”

I explored adding static properties to a stubbed Model class, but I get an error that the build wants me to have Model implement cdk.Construct, so I backed away for now. Perhaps there's a way to get the build to temporarily ignore that requirement?

Remove Dockerfile that was no longer needed Update python base image from 3.6 to 3.6.5 Make the dockerfile work Add MethodResponse support for aws-apigateway Remove Dockerfile that was no longer needed Update python base image from 3.6 to 3.6.5 Make the dockerfile work Add MethodResponse to API Gateway Method. Add some documentation to the MethodResponse properties. Update the test for MethodResponse with response models. Fix some formatting and finish adding code documentation for MethodResponse. Remove Dockerfile from this branch Fix bad merge to methodresponse test. Correct the MethodResponse response models documentation. Add IModel type to reference when configuring a MethodResponse Slight update to comments. Update API Gateway MethodResponse documentation. Author: John Shaskin <[email protected]> Date: Mon Feb 05 5:30:45 2019 +0100 Add some documentation around the referencing default Empty/Error models. Edit documentation around the referencing default Empty/Error models. Edit documentation around the referencing default Empty/Error models. Fix 'trailing whitespace' in comments Add roadmap section with item for adding support for API gateway models

john-shaskin · 2019-02-12T23:08:20Z

@eladb, is there anything else pending on this PR to get it merged?

john-shaskin requested a review from a team as a code owner January 18, 2019 17:41

john-shaskin mentioned this pull request Jan 18, 2019

apigateway: "methodResponses" is missing from MethodOptions #905

Closed

john-shaskin force-pushed the feature/add-apigateway-methodresponse branch from efc8afd to ed930fe Compare January 18, 2019 18:17

sam-goodwin added the feature-request A feature should be added or improved. label Jan 18, 2019

sam-goodwin reviewed Jan 18, 2019

View reviewed changes

sam-goodwin added needs-additional-reviewer @aws-cdk/aws-apigateway Related to Amazon API Gateway labels Jan 18, 2019

RomainMuller reviewed Jan 18, 2019

View reviewed changes

piotrkubisa mentioned this pull request Jan 19, 2019

apigateway: add explicit support for CORS #906

Closed

16 tasks

sam-goodwin removed the feature-request A feature should be added or improved. label Jan 22, 2019

john-shaskin force-pushed the feature/add-apigateway-methodresponse branch 3 times, most recently from b3d24c1 to ddcb89f Compare January 23, 2019 05:31

eladb reviewed Feb 4, 2019

View reviewed changes

eladb assigned sam-goodwin Feb 4, 2019

john-shaskin force-pushed the feature/add-apigateway-methodresponse branch 2 times, most recently from be86fed to 1cf909e Compare February 5, 2019 15:15

sam-goodwin reviewed Feb 5, 2019

View reviewed changes

john-shaskin force-pushed the feature/add-apigateway-methodresponse branch 4 times, most recently from 6ac7b17 to c9a77f5 Compare February 6, 2019 00:27

eladb approved these changes Feb 6, 2019

View reviewed changes

john-shaskin mentioned this pull request Feb 6, 2019

apigateway: missing support for models #1695

Closed

john-shaskin force-pushed the feature/add-apigateway-methodresponse branch from c9a77f5 to 624bda3 Compare February 6, 2019 19:00

sam-goodwin approved these changes Feb 13, 2019

View reviewed changes

sam-goodwin merged commit 46236d9 into aws:master Feb 13, 2019

jamesiri unassigned sam-goodwin Jul 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(apigateway): add methodresponse support #1572

feat(apigateway): add methodresponse support #1572

john-shaskin commented Jan 18, 2019 •

edited

Loading

sam-goodwin left a comment

sam-goodwin Jan 18, 2019

rix0rrr Jan 21, 2019

sam-goodwin Jan 18, 2019

john-shaskin Jan 18, 2019

sam-goodwin Jan 18, 2019

john-shaskin Jan 18, 2019

RomainMuller left a comment

john-shaskin commented Jan 21, 2019

rix0rrr commented Jan 21, 2019

john-shaskin commented Jan 22, 2019

john-shaskin commented Jan 23, 2019

john-shaskin commented Feb 4, 2019

eladb Feb 4, 2019

john-shaskin Feb 4, 2019

eladb commented Feb 4, 2019

john-shaskin commented Feb 5, 2019

sam-goodwin Feb 5, 2019

john-shaskin Feb 5, 2019

eladb Feb 6, 2019

john-shaskin Feb 6, 2019

eladb Feb 6, 2019

john-shaskin Feb 6, 2019

john-shaskin commented Feb 12, 2019

feat(apigateway): add methodresponse support #1572

feat(apigateway): add methodresponse support #1572

Conversation

john-shaskin commented Jan 18, 2019 • edited Loading

Pull Request Checklist

sam-goodwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomainMuller left a comment

Choose a reason for hiding this comment

john-shaskin commented Jan 21, 2019

rix0rrr commented Jan 21, 2019

john-shaskin commented Jan 22, 2019

john-shaskin commented Jan 23, 2019

john-shaskin commented Feb 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Feb 4, 2019

john-shaskin commented Feb 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-shaskin commented Feb 12, 2019

john-shaskin commented Jan 18, 2019 •

edited

Loading