feat: add support for defining/importing api gateway models #2102

john-shaskin · 2019-03-27T15:07:50Z

Adds support to @aws-cdk/api-gateway module to define one or more models, attached to a given REST API.

Note: Did not yet add JSON schema validation for the schema property. Toyed with the jsonschema Node module, but that wouldn't work, due to the lack of JSII support. Open to some suggestions on this, if this problem has been tackled elsewhere in CDK.

Addresses issue: #1695

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- 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.

RomainMuller · 2019-03-28T10:21:49Z

In order to get the jsonschema package to work, you'll have to add it to the package's bundledDependencies. This way, JSII will not need to have a .jsii assembly for it.

RomainMuller · 2019-03-28T10:23:49Z

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

+/**
+ * Interface share for creating or importing a model.
+ */
+export interface IModelConstruct extends cdk.IConstruct, IModel {


I think IModel should be the IConstruct sub interface. I'm not sure what you mean when you talk about simple text reference on the comment of IModel itself...

When I initially added the IModel interface, it was providing support just for the default Empty and Error models that are generated by default for a REST API. In that pull request, the EmptyModel and ErrorModel types were not set up to implement the Construct type, in the interest of keeping things simple. I can try to look into a low-impact way to converge on one IModel interface.

I've been looking at this, and it seems weird to require the EmptyModel and ErrorModel classes to extend the full Construct class, when they are just there to be able to return "Empty" and "Error", for method responses using these built-in models.

I can, but it may just mean that I would stop exporting those types, and only expose them as static properties on the Model class. If that seems best, I can do it, and then converge things down to one interface.

At the moment, I've changed the naming around:

IModel -> IModelRef refers to the base interface that just exposes a model ID. Used for (EmptyModel, ErrorModel)

IModelConstruct -> IModel now refers to the full interface that inherits from IConstruct. Used for (Model and ImportedModel)

RomainMuller · 2019-03-28T10:24:18Z

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

+/**
+ * Model CFN resource


That comment shouldn't be necessary, as the CfnModel class would already encapsulate this information.

Removed the comment.

RomainMuller · 2019-03-28T10:27:05Z

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

+   */
+  public get referenceForSchema(): string {
+    return `https://apigateway.amazonaws.com/restapis/${this.restApi.restApiId}/models/${this.modelId}`;


Might need to use cdk.Aws.urlSuffix instead of amazonaws.com here.

Sure. I can change that.

RomainMuller · 2019-03-28T10:29:27Z

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

+   * @param schema The schema to use to transform data to one or more output formats.
+   * @param description A description that identifies this model.
+   */
+  public addModel(name: string, contentType: string, schema: any, description?: string): Model {


You should wrap contentType, schema, and description? in a "props" type object.

Also, I would advise against using any for the schema. You will not be able to export the type from jsonschema (if you decide to go with it) because it is not a JSII module... But I guess you could duplicate it (it's not ideal, but it's pragmatic).

Yeah. I was definitely trying to add that type safety to the schema before, but was running into those JSII errors. I'll try your suggestion around adding jsonschema to the bundledDependencies.

I was toying with creating a duplicate of the JSONschema type. The JSII linting rules were not fond of the $schema and $ref properties, as they were detected as not conforming to camelCasing.

Perhaps this could be handled by removing those $ characters from the interface, and then transforming the schema property on Model into an object matching the JSON schema spec, with the $ prefixed properties.

eladb · 2019-03-28T11:35:56Z

packages/@aws-cdk/aws-apigateway/README.md

+const method = api.beer.addMethod('GET', getBeerLambdaHandler, {
+  methodResponses: [{
+      statusCode: '200',
+      responseModels: {


the model seems to know what content type it supports. Is there a more concise api that can take advantage of this (something like “addResponseModel”?

Yeah, that seems like a useful way to help steer people away from accidental content type mismatches.

eladb · 2019-03-28T11:36:26Z

packages/@aws-cdk/aws-apigateway/README.md

+
+```ts
+// Direct addition to REST API
+const beerModel = api.addModel('Beer', 'application/json', {


Use keyword arguments instead of positional (basically extract the common options from ModelProps and reuse here)

eladb · 2019-03-28T11:37:47Z

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

+   * The ID of a REST API with which to associate this model.
+   * Required.
+   */
+  restApi: IRestApi,


Use semi-colons instead of commas

eladb · 2019-03-28T11:38:16Z

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

+   * Required.
+   * @see http://json-schema.org/
+   */
+  schema: any, // TODO: Validate that the schema provided matches the JSON schema spec.


Yes please!

Working on it, but JSII linting does not like the $ref and $schema properties in my JsonSchema interface. Attempting to remove those from the interface, but transform the output schema object to use $schema and $ref. It's getting kind of ugly with all the recursive properties...

Alright, I've got a little mapper class to take care of transforming the ref and schema properties to include the $ prefix.

eladb · 2019-03-28T11:39:39Z

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

+ * Defines a reference to a model created outside of the current CloudFormation stack.
+ */
+export class ImportedModel extends cdk.Construct implements IModelConstruct {


Shouldn’t be exported

Removed the export.

eladb · 2019-03-28T11:40:51Z

packages/@aws-cdk/aws-apigateway/package.json

@@ -82,7 +82,9 @@
  },
  "awslint": {
    "exclude": [
-      "resource-attribute:@aws-cdk/aws-apigateway.IRestApi.restApiRootResourceId"
+      "resource-attribute:@aws-cdk/aws-apigateway.IRestApi.restApiRootResourceId",
+      "resource-interface-extends-construct:@aws-cdk/aws-apigateway.IModel",


I don’t see a good reason to exclude these

This ties into my explanation to @RomainMuller's comment above about the two interfaces for model. The original IModel was added to start getting model support in for MethodResponse, but only going as far as helping to allow the default Empty and Error models to be referenced. I didn't think it necessarily made sense to complicate those model types by making them implement Construct. That said, as above, I can look into what's needed to converge these into one interface. In which case, I will remove this exclusion.

I was originally getting build errors on the IModel interface, with those complaints about extending construct and including an export() method, but I don't seem to be now. So, I've removed the exclusions. 🤷‍♂️

john-shaskin · 2019-04-02T00:35:52Z

@RomainMuller and @eladb, I've made an attempt to address all your requests/comments above. Most of it was pretty straight forward. However, there are a couple of things that I'm still a bit unsure of:

My most recent commit is an attempt to address the suggestion that MethodReponse objects should have an addResponseModel method. To do this, I've renamed the existing interface, and converted MethodResponse to a class, so that this method can be added. Exposing the content type on a model ended up feeling a bit awkward. It's easy on the concrete Model implementation, as the content type is explicitly set, when defining the model. However, if you are importing a model or trying to use the default Empty or Error models, then I wasn't sure quite what to do. In the latter case, I can perhaps assume that those default Empty and Error models are always JSON. But for imported models, it felt weird to add content type to the import prop interface.
On the suggestion to converge the two interfaces for Model, I've left the interfaces separate. The IModelRef base interface has remained separate from IModel largely to support referring to the Empty and Error default models, without needing to have those classes implement Construct and an export method. It didn't feel quite right to me to go that route. However, if you guys disagree, I'm happy to make the necessary changes to converge on a single IModel interface.

…h a single test. Add a test for adding a model with valid JSON schema. Document some test cases. Add support for optional model name/description properties. Add test for consumption of a model in a MethodResponse. Add test case for importing an existing model, and using it. Clean up testing todo list. Add test case for exporting a model. Update formatting from 4 to 2 spaces. Create IModelConstruct to expose IConstruct properties on concrete implementations of IModel. Fix tests from rebase to updated master. Add method to RestApi to direclty add a model. Tweak some documentation and add an integration test for an API with defined models. Expose property on model that can be used to generate a canonical reference for another external model to consume. Correct typo in README, and add some comments into code sample. Update comments and make minor syntactic changes. Split ModelProps into two interfaces to handle the constructor vs addModel method cases. Clean up IModel awslint exclusions. Rename IModel -> IModelRef, and IModelConstruct -> IModel. Add JSON schema type safety to the schema property of Model. Fix test errors after merge with 'master'

…type to be inferred from a created model. Fix methodresponse usage in README, and update formatting of integration test expected files. Convert JsonSchemaSchema enum to a static class, to fix build errors. Fix README for changes in JsonSchemaSchema, again.

john-shaskin requested a review from a team as a code owner March 27, 2019 15:07

john-shaskin force-pushed the feature/add-apigateway-model branch 2 times, most recently from cc477ef to ff7ca01 Compare March 27, 2019 18:59

john-shaskin mentioned this pull request Mar 27, 2019

apigateway: missing support for models #1695

Closed

RomainMuller reviewed Mar 28, 2019

View reviewed changes

eladb suggested changes Mar 28, 2019

View reviewed changes

john-shaskin force-pushed the feature/add-apigateway-model branch from ec33e0b to b1bdb3d Compare April 1, 2019 00:42

john-shaskin force-pushed the feature/add-apigateway-model branch 2 times, most recently from abfb8a9 to fb1615d Compare April 2, 2019 13:44

john-shaskin added 2 commits April 2, 2019 12:24

john-shaskin force-pushed the feature/add-apigateway-model branch from fb1615d to 3161b42 Compare April 2, 2019 18:25

john-shaskin closed this Jun 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for defining/importing api gateway models #2102

feat: add support for defining/importing api gateway models #2102

john-shaskin commented Mar 27, 2019 •

edited

Loading

RomainMuller commented Mar 28, 2019

RomainMuller Mar 28, 2019

john-shaskin Mar 28, 2019

john-shaskin Mar 28, 2019 •

edited

Loading

RomainMuller Mar 28, 2019

john-shaskin Mar 28, 2019

RomainMuller Mar 28, 2019

john-shaskin Mar 28, 2019

RomainMuller Mar 28, 2019

john-shaskin Mar 28, 2019

john-shaskin Mar 28, 2019 •

edited

Loading

eladb Mar 28, 2019

john-shaskin Mar 28, 2019

eladb Mar 28, 2019

john-shaskin Mar 29, 2019

eladb Mar 28, 2019

john-shaskin Mar 29, 2019

eladb Mar 28, 2019

john-shaskin Mar 29, 2019

john-shaskin Apr 1, 2019

eladb Mar 28, 2019

john-shaskin Mar 28, 2019

eladb Mar 28, 2019

john-shaskin Mar 28, 2019 •

edited

Loading

john-shaskin Mar 28, 2019

john-shaskin commented Apr 2, 2019

feat: add support for defining/importing api gateway models #2102

feat: add support for defining/importing api gateway models #2102

Conversation

john-shaskin commented Mar 27, 2019 • edited Loading

Pull Request Checklist

RomainMuller commented Mar 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-shaskin Mar 28, 2019 • edited Loading

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

john-shaskin Mar 28, 2019 • edited Loading

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

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 Mar 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-shaskin commented Apr 2, 2019

john-shaskin commented Mar 27, 2019 •

edited

Loading

john-shaskin Mar 28, 2019 •

edited

Loading

john-shaskin Mar 28, 2019 •

edited

Loading

john-shaskin Mar 28, 2019 •

edited

Loading