feat(AWS API Gateway): Allow reuse and customization of schema models #7619

jmcguffee · 2020-04-23T20:58:57Z

What did you implement

Added in model definition capabilities. This can be done globally to create one resource for a specific model vs new for every function or resources at the function level that are customizable.

Closes #6171

How can we verify it

service: test-api

frameworkVersion: ">=1.1.0 <2.0.0"

provider:
  name: aws
  runtime: nodejs12.x
  region: us-east-1
  apiGateway:
    requestSchemas:
      todo-add-model:
        name: TodoAddModel
        schema: ${file(api_schema/todo_add_schema.json)}
        description: "A Model validation for adding todos"
      todo-update-model: ${file(api_schema/todo_update_modl_schema.json)}


functions:
  create-user:
    handler: handler.createUser
    events:
      - http:
        path: users
        method: post
        cors: true
        requestSchema
           application/json:
              definition: ${file(api_schema/user_add_schema.json)}
              name: UserAddModel
              description: "Model validation when adding new users"
  update-user:
    handler: handler.updateUser
    events:
      - http:
        path: users
        method: put
        cors: true
        requestSchema:
          application/json: ${file(api_schema/user_update_schema.json)} #This will default to UpdateUserModel

  create-todo:
    handler: handler.createTodo
    events:
      - http:
        path: todos
        method: post
        cors: true
        requestSchema:
          application/json: todo-add-model
  update-todo:
    handler: handler.updateUser
    events:
      - http:
        path: todos
        method: put
        cors: true
        requestSchema: 
          application/json: todo-update-model

Todos

Useful Scripts

npm run test:ci --> Run all validation checks on proposed changes
npm run lint:updated --> Lint all the updated files
npm run lint:fix --> Automatically fix lint problems (if possible)
npm run prettier-check:updated --> Check if updated files adhere to Prettier config
npm run prettify:updated --> Prettify all the updated files

Write and run all tests
Write documentation
Enable "Allow edits from maintainers" for this PR
Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO (Existing operation behaves as before, but can be extended)

medikoo

@jmcguffee great thanks for giving this a spin!

We definitely should have that supported. Still I think it'll be great first to agree on final configuration proposal.

I've proposed something slightly different, but I think more powerful here: #6171 (comment) It takes into account also some other needs. What do you think about it?

Let's maybe first clarify final shape at #6171 and then come back here to finalize implementation

jmcguffee · 2020-04-24T13:16:43Z

@medikoo Sounds good.

jmcguffee · 2020-04-24T19:12:38Z

@medikoo updated to include proposed in issue. Have a look and let me know what looks good and what doesn't.

jmcguffee · 2020-04-24T20:33:45Z

I don't know why the prettier commands aren't working properly on my machine

codecov-io · 2020-04-24T20:47:57Z

Codecov Report

Merging #7619 into master will increase coverage by 0.06%.
The diff coverage is 98.43%.

@@            Coverage Diff             @@
##           master    #7619      +/-   ##
==========================================
+ Coverage   87.99%   88.05%   +0.06%     
==========================================
  Files         245      246       +1     
  Lines        9188     9254      +66     
==========================================
+ Hits         8085     8149      +64     
- Misses       1103     1105       +2

Impacted Files	Coverage Δ
...kage/compile/events/apiGateway/lib/method/index.js	`100.00% <ø> (ø)`
...s/package/compile/events/apiGateway/lib/restApi.js	`98.00% <ø> (ø)`
.../compile/events/apiGateway/lib/requestValidator.js	`98.36% <98.36%> (ø)`
lib/plugins/aws/lib/naming.js	`98.10% <100.00%> (+0.02%)`	⬆️
...ins/aws/package/compile/events/apiGateway/index.js	`100.00% <100.00%> (ø)`
lib/plugins/aws/info/getResourceCount.js	`90.00% <0.00%> (-10.00%)`	⬇️
...ib/plugins/aws/package/compile/events/sqs/index.js	`100.00% <0.00%> (ø)`
.../compile/events/apiGateway/lib/method/responses.js	`100.00% <0.00%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e56dea...8faeea5. Read the comment docs.

medikoo

Thank you @jmcguffee ! It looks really promising, I've proposed some improvements that should help us in future maintenance of this feature

medikoo · 2020-04-27T08:53:28Z

README.md

@@ -23,10 +23,11 @@ Serverless is an MIT open-source project, actively maintained by a full-time, ve

 <a href="https://www.youtube.com/watch?v=-Nf0ui3qP2E" target="_blank">Watch the video overview here.</a>

-In 2020, the Serverless Framework began introducing advanced functionality for specific serverless use-cases, known as Serverless Framework Components.  Check out the Components featured below, [and more here](https://github.com/serverless-components).
+In 2020, the Serverless Framework began introducing advanced functionality for specific serverless use-cases, known as Serverless Framework Components. Check out the Components featured below, [and more here](https://github.com/serverless-components).


Let's not include unrelated changes (otherwise we're happy to take it in other PR)

I guess one of the plugins updated this file. I will revert it.

docs/providers/aws/events/apigateway.md

medikoo · 2020-04-27T09:01:55Z

lib/plugins/aws/package/compile/events/apiGateway/lib/method/index.js

+          let description;
+          let definition;
+          if (schema.schema) {
+            name = schema.name ? schema.name : `${resourceName}Model`;


If there's no name, I would let CF generate on (as automatic generation as above is immune to errors)

medikoo · 2020-04-27T09:02:14Z

lib/plugins/aws/package/compile/events/apiGateway/lib/method/index.js

+          let definition;
+          if (schema.schema) {
+            name = schema.name ? schema.name : `${resourceName}Model`;
+            description = schema.description ? schema.description : `Validation model for ${name}`;


Same here, if there's no description, then let's not add description

medikoo · 2020-04-27T09:54:38Z

lib/plugins/aws/package/compile/events/apiGateway/lib/restApi.js

@@ -139,6 +138,77 @@ module.exports = {
      );
    }

+    // Models are defined in the provider and can be used in http events later
+    if (!_.isUndefined(apiGateway.schemas)) {


Let's write it as if (apiGateway.schemas) {

medikoo · 2020-04-27T10:01:55Z

lib/plugins/aws/package/compile/events/apiGateway/lib/restApi.js

+        if (!resourceDefinition.name) {
+          name = _.upperFirst(_.camelCase(resourceName));
+        } else {
+          name = _.upperFirst(_.camelCase(resourceDefinition.name));
+        }


Let's not implement auto generation for name, it increases maintenance cost and we're not applying necessary guards to ensure we produce a valid name (that fits max length etc)

medikoo · 2020-04-27T10:04:14Z

lib/plugins/aws/package/compile/events/apiGateway/lib/restApi.js

+          name = _.upperFirst(_.camelCase(resourceDefinition.name));
+        }
+
+        const modelLogicalId = this.getModelLogicalId(name);


We should use this.provider.naming.getModelLogicalId, and ensure it supports case of reusable models

medikoo · 2020-04-27T10:08:54Z

lib/plugins/aws/package/compile/events/apiGateway/lib/restApi.js

@@ -139,6 +138,77 @@ module.exports = {
      );
    }

+    // Models are defined in the provider and can be used in http events later
+    if (!_.isUndefined(apiGateway.schemas)) {


I would also seclude it into dedicated lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidators.js (and handle generation of all AWS::ApiGateway::Model and AWS::ApiGateway::RequestValidator resources, so it's in one place)

medikoo · 2020-04-27T10:09:46Z

lib/plugins/aws/package/compile/events/apiGateway/lib/restApi.js

+          validatorLogicalId,
+        };
+
+        _.merge(this.serverless.service.provider.compiledCloudFormationTemplate.Resources, {


Let's use Object.assign

medikoo · 2020-04-27T10:15:08Z

lib/plugins/aws/package/compile/events/apiGateway/lib/method/index.test.js

@@ -157,6 +164,163 @@ describe('#compileMethods()', () => {
    });
  });

+  it('should support inline model configuration', () => {


As I suggested in other comment it'll be great to seclude logic into lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidators.js, then lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidators.test.js test file should have few tests configured through runServerless util.

Currently we're coming away from tests as you can see in most parts of the framework. Usually they focus on implementation internals and do not test against fully build serverless instance, which make them very weak.

See how new tests are written in those examples (sorry for not having a proper document on that yet, I will try to prepare one shortly):

serverless/lib/plugins/aws/package/compile/functions/index.test.js

Lines 2624 to 2719 in 7d3636f

describe('AwsCompileFunctions #2', () => {

after(fixtures.cleanup);

describe('Destinations', () => {

it('Should reference function from same service as destination', () =>

runServerless({

cwd: fixtures.map.functionDestinations,

cliArgs: ['package'],

}).then(serverless => {

const cfResources = serverless.service.provider.compiledCloudFormationTemplate.Resources;

const naming = serverless.getProvider('aws').naming;

const destinationConfig =

cfResources[naming.getLambdaEventConfigLogicalId('trigger')].Properties.DestinationConfig;

expect(destinationConfig).to.deep.equal({

OnSuccess: {

Destination: { 'Fn::GetAtt': [naming.getLambdaLogicalId('target'), 'Arn'] },

},

});

expect(destinationConfig).to.not.have.property('OnFailure');

}));

it('Should support OnFailure destinations', () =>

fixtures

.extend('functionDestinations', {

functions: { trigger: { destinations: { onSuccess: null, onFailure: 'target' } } },

})

.then(fixturePath =>

runServerless({

cwd: fixturePath,

cliArgs: ['package'],

}).then(serverless => {

const cfResources =

serverless.service.provider.compiledCloudFormationTemplate.Resources;

const naming = serverless.getProvider('aws').naming;

const destinationConfig =

cfResources[naming.getLambdaEventConfigLogicalId('trigger')].Properties

.DestinationConfig;

expect(destinationConfig).to.not.have.property('OnSuccess');

expect(destinationConfig).to.deep.equal({

OnFailure: {

Destination: { 'Fn::GetAtt': [naming.getLambdaLogicalId('target'), 'Arn'] },

},

});

})

));

it('Should support ARN to external function as destination', () => {

const arn = 'arn:aws:lambda:us-east-1:12313231:function:external';

return fixtures

.extend('functionDestinations', {

functions: { trigger: { destinations: { onSuccess: arn } } },

})

.then(fixturePath =>

runServerless({

cwd: fixturePath,

cliArgs: ['package'],

}).then(serverless => {

const cfResources =

serverless.service.provider.compiledCloudFormationTemplate.Resources;

const naming = serverless.getProvider('aws').naming;

const destinationConfig =

cfResources[naming.getLambdaEventConfigLogicalId('trigger')].Properties

.DestinationConfig;

expect(destinationConfig).to.deep.equal({ OnSuccess: { Destination: arn } });

})

);

});

it('Should respect `role` setting', () =>

fixtures

.extend('functionDestinations', { provider: { role: ' arn:aws:iam::XXXXXX:role/role' } })

.then(fixturePath =>

runServerless({

cwd: fixturePath,

cliArgs: ['package'],

}).then(serverless => {

const cfResources =

serverless.service.provider.compiledCloudFormationTemplate.Resources;

const naming = serverless.getProvider('aws').naming;

const destinationConfig =

cfResources[naming.getLambdaEventConfigLogicalId('trigger')].Properties

.DestinationConfig;

expect(destinationConfig).to.deep.equal({

OnSuccess: {

Destination: { 'Fn::GetAtt': [naming.getLambdaLogicalId('target'), 'Arn'] },

},

});

expect(destinationConfig).to.not.have.property('OnFailure');

})

));

});

});

https://github.com/serverless/serverless/blob/7d3636f9682c7c9929a9061f105ed232d139aa56/lib/plugins/aws/package/compile/events/httpApi/index.test.js

You can find documentation for runServerless util here: https://github.com/serverless/test/blob/master/docs/run-serverless.md

jmcguffee · 2020-04-27T13:38:17Z

@medikoo Thanks for the feedback. I will work on getting them updated today.

jmcguffee · 2020-04-27T20:48:11Z

@medikoo take a look at the updates and let me know if that is more of the direction that you were thinking. If so I will update all the tests and push for review again. As per your direction I will use the testing procedure specified in the PR.

medikoo

@jmcguffee thank you! Yes direction is perfect, just ensure to respect lint and prettier formatting, and reduce lodash usage to minimum.

medikoo · 2020-04-28T08:45:42Z

docs/providers/aws/guide/serverless.yml.md

@@ -269,6 +274,13 @@ functions:
            identitySource: method.request.header.Authorization
            identityValidationExpression: someRegex
            type: token # token or request. Determines input to the authorizer function, called with the auth token or the entire request event. Defaults to token
+          schema:


We can remove schema from here now, as I would consider this as deprecated

jmcguffee · 2020-04-30T16:00:23Z

@medikoo this is ready for review again with updated test

medikoo

@jmcguffee that looks great. Thank you!

Please see my suggestions, and let me know what you think. Once we have them settled I believe we're fine to get that in.

docs/providers/aws/events/apigateway.md

medikoo · 2020-04-30T18:27:42Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+        const resourceName = _.upperFirst(_.camelCase(schemaId));
+        const modelLogicalId = this.provider.naming.getModelLogicalId(
+          resourceName,
+          'any',
+          'application/json'
+        );


I think we need something more solid, in current form we're using method which was strictly designed to resolve logical id for specific HTTP endpoint and content type, without any endpoint and content type context.

I know I suggested to use same method, but that just doesn't look good.

I propose to rename current method to getEndpointModelLogicalId, and introduce another getModelLogicalId that takes just schemaId and resolves id as:

return `ApiGateway${_.upperFirst(_.camelCase(schemaId))}Model`;

medikoo · 2020-04-30T18:28:47Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+          'application/json'
+        );
+
+        const validatorLogicalId = this.provider.naming.getValidatorLogicalId(resourceName, 'any');


I light of above comment I would refactor getValidatorLogicalId to just take modelLogicalId

This one is going to be a little more involved. The resource only defines one validator as a whole and multiple models can be defined. With that route multiple validators will be created while only one is being referenced in the Method Resource.

As i checked this method is only used in two places.. It can simply be updated to accept just modelLogicalId and places were it's invoked should then be updated to just pass it (and over there it's resolved upfront)

This was updated and refactored out to another method

medikoo · 2020-04-30T18:29:46Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+          validatorLogicalId,
+        };
+
+        _.merge(this.serverless.service.provider.compiledCloudFormationTemplate.Resources, {


Let's use Object.assign

medikoo · 2020-04-30T18:31:20Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+          },
+        });
+
+        if (modelName != null) {


I think we can write it as if (modelName), at least I would treat eventual empty string as lack of value

medikoo · 2020-04-30T18:35:12Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+
+          if (schemaConfig.schema) {
+            modelName = schemaConfig.name ? schemaConfig.name : null;
+            validatorName = modelName !== null ? `${modelName}Validator` : null;


Let's simplify to: modelName ? ...

medikoo · 2020-04-30T18:42:10Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+          let validatorLogicalId;
+
+          // New resources need to be created
+          if (!models[schemaConfig]) {


Logic seems to be a bit leaky, as we're also running this check if schemaConfig is object.

I propose to restructure above logic into:

if (_.isObject(schemaConfig)) { // Schema defined inline, specific to given endpont } else { // String if (models[schemaConfig]) { // Schema reused } else { // Simple schema defined inline, specific to given endpoint } }

I will add an or to it as if (_.isObject(schemaConfig) || !models[schemaConfig]). I was trying to DRY out as much as possible without writing a bunch of helper methods.

medikoo · 2020-04-30T18:42:21Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+              event.http.method
+            );
+
+            _.merge(this.serverless.service.provider.compiledCloudFormationTemplate.Resources, {


Let's use Object.assign

Doesn't seem addressed

medikoo · 2020-04-30T18:44:46Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.test.js

+describe('#compileRequestValidators()', () => {
+  after(fixtures.cleanup);
+
+  describe(' - provider configuration -', () => {


Let's stick to convention of a project, so no - prefixes and postfixes.

Also let's maybe name it reusable schemas, note provider configuration, suggests it's a configuration of provider.

medikoo · 2020-04-30T18:45:12Z

lib/plugins/aws/package/compile/events/apiGateway/lib/restApi.js

+      name += 'Model';
+    }
+    return name;
+  },


I belive there should be no changes in that file

jmcguffee · 2020-05-12T14:45:19Z

@medikoo I believe all issues have been addressed.

medikoo

@jmcguffee that looks really good, thank you!

Please see my remarks, I believe they final, and we'd be able shortly to take that in

docs/providers/aws/events/apigateway.md

medikoo · 2020-05-12T15:19:31Z

lib/plugins/aws/lib/naming.js

+  getValidatorLogicalId(modelLogicalId) {
+    return `${modelLogicalId}Validator`;
+  },
+  getEndpointValidatorLogicalId(resourceId, methodName) {


This function can be removed, as it's used now in places where we have modelLogicalId, so we can use getValidatorLogicalId(modelLogicalId) instead

I added the additional method because there is only 1 validator allowed per API Gateway Method. This was causing issues when using the contentType in the ModelLogicalID. This only becomes an issue when multiple content types are defined in the resource. I have updated to utilize getModelLogicalId within this method now to prevent that.

medikoo · 2020-05-12T15:21:51Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+      }
+
+      // New Functionality
+      if (event.http.requestSchema) {


Let's do:

if (event.http.requestSchema) { ... } else if (event.http.request && event.http.request.schema) { ... }

At least I believe we should not support both formats put together, and if it's the case, new way should be favored

medikoo · 2020-05-12T15:22:36Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+              event.http.method
+            );
+
+            _.merge(this.serverless.service.provider.compiledCloudFormationTemplate.Resources, {


Doesn't seem addressed

medikoo · 2020-05-12T15:25:15Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.test.js

+
+  describe(' reusable schemas ', () => {
+    it('Should process schema from apiGateway provider, full config', () =>
+      fixtures.extend('requestSchema', {}).then(fixturePath =>


We do not extend fixture with anything, so let's not use fixture.extend

Im really running that to help populate the serverless block using the fixturePath. I didn't see another example just using runServerless, but I will work through it.

Here's an example of using fixture directly:

serverless/lib/plugins/aws/package/compile/functions/index.test.js

Lines 2629 to 2632 in 7310782

runServerless({

cwd: fixtures.map.functionDestinations,

cliArgs: ['package'],

}).then(serverless => {

medikoo · 2020-05-12T15:25:26Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.test.js

+      ));
+
+    it('Should process schema from apiGateway provider, missing name and description', () =>
+      fixtures.extend('requestSchema', {}).then(fixturePath =>


medikoo · 2020-05-12T15:26:02Z

lib/plugins/aws/package/compile/events/apiGateway/lib/restApi.js

@@ -6,7 +6,6 @@ const BbPromise = require('bluebird');
 module.exports = {
  compileRestApi() {
    const apiGateway = this.serverless.service.provider.apiGateway || {};
-


Let's remove this change

medikoo · 2020-05-12T15:26:27Z

tests/fixtures/requestSchema/dummy_schema.json

@@ -0,0 +1,14 @@
+{


Let's use camelCase convention for file names

medikoo · 2020-05-12T15:28:34Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.test.js

+      ));
+
+    it('Should process schema from apiGateway provider, missing name and description', () =>
+      fixtures.extend('requestSchema', {}).then(fixturePath =>


Let's reuse one serverless instance when we do not change fixture. We can put serverless instance initialization into before block, and then in it's just confirm on generated template.

It'll run also way faster then

jmcguffee · 2020-05-27T16:26:30Z

@medikoo sorry for the delay. Got tied up with work and really busy. Will update and submit new commit sometime this week.

medikoo · 2020-05-27T16:28:44Z

@jmcguffee no problem, looking forward for update

codecov-commenter · 2020-05-27T22:29:51Z

Codecov Report

Merging #7619 (a32e23c) into master (3e47dcc) will increase coverage by 0.02%.
The diff coverage is 93.16%.

@@            Coverage Diff             @@
##           master    #7619      +/-   ##
==========================================
+ Coverage   87.02%   87.04%   +0.02%     
==========================================
  Files         285      287       +2     
  Lines       10922    10987      +65     
==========================================
+ Hits         9505     9564      +59     
- Misses       1417     1423       +6

Impacted Files	Coverage Δ
...kage/compile/events/apiGateway/lib/method/index.js	`100.00% <ø> (ø)`
.../package/compile/events/apiGateway/lib/validate.js	`96.27% <ø> (ø)`
lib/plugins/aws/provider.js	`94.85% <ø> (ø)`
scripts/serverless.js	`73.80% <60.00%> (-0.88%)`	⬇️
lib/plugins/create/create.js	`90.00% <84.61%> (-0.70%)`	⬇️
lib/cli/conditionally-load-dotenv.js	`93.33% <93.33%> (ø)`
.../compile/events/apiGateway/lib/requestValidator.js	`98.14% <98.14%> (ø)`
lib/Serverless.js	`93.52% <100.00%> (-0.40%)`	⬇️
lib/cli/load-dotenv.js	`94.11% <100.00%> (ø)`
lib/configuration/variables/parse.js	`100.00% <100.00%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e47dcc...f7d9e95. Read the comment docs.

jmcguffee · 2020-05-27T22:31:42Z

@medikoo should be good now. Let me know if you spot any more issues. I noticed I couldn't use the async/await for compatibility issues. Hoping the way I did it is good, but goes beyond my knowledge of best practices for JS. This is in reference to the new test file isolating runServerless to before

medikoo

@jmcguffee that's outsanding work, thank you!

I've pointed just few last details, having that I believe we will be ready to get this in

medikoo · 2020-05-28T08:11:36Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+
+        const validatorLogicalId = this.provider.naming.getValidatorLogicalId(modelLogicalId);
+
+        if (schemaConfig.name) {


To be on a safe side, I believe this condition should be run only if definition was provided on schemaConfig.schema property.

Otherwise we're inspecting the actual definition for those properties, and e.g. description is a perfectly valid JSON Schema property, which with logic as proposed may be used as model description, which is not right

These properties are separate from the actual JSON for the schema definition. In that sense there should be no issues adding name even if those properties don't exist. I can do it just for the sake of it, but I am not seeing where the clash would happen.

medikoo · 2020-05-28T08:15:02Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+        Object.assign(this.serverless.service.provider.compiledCloudFormationTemplate.Resources, {
+          [modelLogicalId]: {
+            Type: 'AWS::ApiGateway::Model',
+            Properties: {
+              RestApiId: { Ref: this.apiGatewayRestApiLogicalId },
+              Schema: definition,
+              ContentType: 'application/json',
+            },
+          },
+          [validatorLogicalId]: {
+            Type: 'AWS::ApiGateway::RequestValidator',
+            Properties: {
+              RestApiId: { Ref: this.apiGatewayRestApiLogicalId },
+              ValidateRequestBody: true,
+              ValidateRequestParameters: true,
+            },
+          },
+        });


It'll be nice to add those resources to template only if those models are actually used in events

medikoo · 2020-05-28T08:16:50Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.test.js

+  let globalServerless;
+
+  before(() => {
+    globalServerless = runServerless({


Let's rename it to serverlessInstance, also we should store resolved instance and not promise that eventually resolves the instance

medikoo · 2020-05-28T08:21:42Z

tests/fixtures/requestSchema/dummySchema.json

+  "$schema": "http://json-schema.org/draft-04/schema#",
+  "type": "object",
+  "title": "Test Validation Schema",
+  "required": ["id"],


We may add description here, to expose eventual vulnerability I've pointed in logic

Seems not addressed

medikoo · 2020-05-28T08:23:17Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.test.js

+        expect(modelResource).to.deep.equal({
+          Type: 'AWS::ApiGateway::Model',
+          Properties: {
+            ContentType: 'application/json',
+            Description: 'Test Description',
+            Name: 'TestModel',
+            RestApiId: {
+              Ref: 'ApiGatewayRestApi',
+            },
+            Schema: {
+              $schema: 'http://json-schema.org/draft-04/schema#',
+              definitions: {},
+              properties: {
+                id: {
+                  pattern: '[0-9]+',
+                  title: 'ID for object',
+                  type: 'number',
+                },
+              },
+              required: ['id'],
+              title: 'Test Validation Schema',
+              type: 'object',
+            },
+          },
+        });
+      }));


Really great tests! Thank you!

I'm always trying to minimize dependency on implementation details, so usually I do not confirm on every single property but on one or few crucial. So e.g. some cleanup doesn't necessarily break the tests.

(just a note for future, I do not expect you to update above)

jmcguffee · 2020-08-21T15:31:23Z

@medikoo my greatest apologies for the delay. I picked up a couple of contracts and they have tied my time up. I submitted some of the changes requested from the feedback.

medikoo

@jmcguffee great thanks for update, and now worries for delay it's totally understood.

Over last months few things got improved in a framework and also critieria's for PR's change. Recently, we've added a configuration validation (that works against pre-configured JSON schema), and we're doing our best so all incoming improvements also come with fully configured schema.

Additionally we have a dedicated deprecation logging mechanism which would be good to use here for deprecating old schema configuration format.

I've also noticed that our decision to name new schemas with requestSchemas is probably not great.

Please see my comments

medikoo · 2020-08-24T07:30:03Z

docs/providers/aws/events/apigateway.md

+      - http:
+          path: posts/create
+          method: post
+          requestSchema:


I've just looked back and our naming decisions, and I think we overlooked one thing. Currently http event has request configuration that supports both parameters and schema.

When we were discussing global configuration of schemas on provider.apiGateway I've pointed that schemas name is ambigous in that context and that it'll be better to name it requestSchemas.

Having that we followed also with introduction of requestSchemas on HTTP event, and that doesn't feel right over there, as we have now http.request.parameters and http.requestSchema.

I think it was not great decision (I missed that it's not just schema that we configure there), and I believe we should not go into that.

I propose that we go with provider.apiGateway.request.schemas and http.request.schemas, and deprecate http.request.schema (note that now we have a dedicated deprecation function that should be used for that)

Also it'll be nice to back new request schema configuration with JSON schema validation, of which support was merged recently into project (for reference issue in which we tackle complete schema for http event: #8018 )

While I was wrapping up the last changes (deprecation), I've started wondering about this approach to having provider.apiGateway.request.schemas and http.request.schemas and deprecating http.request.schema - while provider.apiGateway.request.schemas sounds good, I'm wondering if we should/need to move from http.request.schema to http.request.schemas - what is the main reason here? Is it to be consistent and use plural schemas? Otherwise, we could totally accomodate the old http.request.schema to support both new and old approach (because it's only extending it). Please let me know what do you think. 🙇

Is it to be consistent and use plural schemas?

Yes, in first it's to be consistent (it's a bit confusing to have schemas as global, and schema as local, with very same definition underneath). Also I think plural form is right, as we're configuring dictionary of schemas. Singular name may suggest we're defining the single schema, and those are its properties, which is again confusing.

While I understand the pain of creating deprecation just because we add s I feel there's good reasoning behind it, and that it'll leave our API more solid.

What do you think?

Thanks for the explanation - that's what I thought here, but wanted to confirm as I wasn't 100% sure. I think it makes sense to be consistent here - the migration path from schema to schemas is not complicated so it shouldn't be too painful for users. Thanks for the clarification 💯

medikoo · 2020-08-24T07:30:55Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+
+module.exports = {
+  compileRequestValidators() {
+    const apiGateway = this.serverless.service.provider.apiGateway || {};


It'll be nicer to name it apiGatewayConfig (apiGateway suggests we deal with API Gateway instance)

medikoo · 2020-08-24T07:33:10Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+      );
+
+      // Old functionality
+      if (event.http.request && event.http.request.schema) {


It'll be nice to put precedence on event.http.request.schemas and not on deprecated event.http.request.schema

medikoo · 2020-08-24T07:44:55Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+          if (schemaConfig.schema) {
+            modelName = schemaConfig.name ? schemaConfig.name : null;
+            validatorName = modelName ? `${modelName}Validator` : null;
+            description = schemaConfig.description ? schemaConfig.description : null;
+            definition = schemaConfig.schema;
+          } else {
+            definition = schemaConfig;
+          }
+
+          let modelLogicalId;
+          let validatorLogicalId;
+
+          // New resources need to be created
+          if (
+            _.isObject(schemaConfig) ||
+            !(apiGateway.requestSchemas && apiGateway.requestSchemas[schemaConfig])
+          ) {


Can we improve readability of this construct with:

if (_.isObject(schemaConfig) { modelName = schemaConfig.name ? schemaConfig.name : null; ... } else { definition = schemaConfig; } ... if (_.isObject(schemaConfig) || !_.get(apiGatewayConfig.requestSchemas, definition)) { ...

Above will also make it more bulletproof, e.g. with what's configured currently lack of schemaConfig.schema enforces assumption that schemaConfig is string, when it doesn't have to be the case.

medikoo · 2020-08-24T07:45:40Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+                modelLogicalId
+              ].Properties.Description = description;
+            }
+          } else if (apiGateway.requestSchemas && apiGateway.requestSchemas[schemaConfig]) {


I think this condition is unconditionally true at this point ( it should be just } else {)

medikoo · 2020-08-24T07:58:49Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.test.js

+  let serverlessInstance;
+
+  before(() => {
+    return Promise.resolve(


Let's not use Promise.resolve (it's an anti pattern, runServerless().then() can be returned directly and effect is exactly same)

medikoo · 2020-08-24T07:59:04Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.test.js

+  });
+
+  after(() => {
+    fixtures.cleanup();


It's an async operation that needs to be returned

medikoo · 2020-08-24T08:00:36Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.test.js

+        cwd: fixtures.map.requestSchema,
+        cliArgs: ['package'],
+      })
+    ).then(serverless => (serverlessInstance = serverless));


runServerless got improved over course of time, and now it needs to be setup as: ({ serverless }) => (serverlessInstance = serverless)

medikoo · 2020-08-24T08:02:06Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.test.js

+      const cfResources =
+        serverlessInstance.serverless.service.provider.compiledCloudFormationTemplate.Resources;
+      const naming = serverlessInstance.serverless.getProvider('aws').naming;


It'll be nice to get both of this directly from runServerless result. See: https://github.com/serverless/test/blob/master/docs/run-serverless.md#result-values (it's also used in other tests across the project)

medikoo · 2020-08-24T08:07:10Z

tests/fixtures/requestSchema/serverless.yml

+          path: test
+          method: get
+          requestSchema:
+            application/json: test-model
+      - http:
+          path: testSimple
+          method: get
+          requestSchema:
+            application/json: test-model-simple
+      - http:
+          path: test2
+          method: get
+          requestSchema:
+            application/json: '${file(dummySchema.json)}'
+      - http:
+          path: test3
+          method: get
+          requestSchema:
+            application/json:
+              name: TestMethodModel
+              description: 'Test Method Model Desc'
+              schema: '${file(dummySchema.json)}'
+      - http:
+          path: test4
+          method: get
+          request:
+            schema:
+              application/json: '${file(dummySchema.json)}'
+      - http:
+          path: test5
+          method: get
+          requestSchema:
+            application/json: '${file(dummySchema.json)}'
+            text/plain: 'foo'
+      - http:
+          path: test6


It'll be nice to name paths after what they test, so:

test -> test-model-full
testSimple -> test-model-simple
test2 -> test-direct-simple
test3 -> test-direct-full
test4 -> test-deprecated-simple
test5 -> test-multiple
test6 -> test-deprecated-multiple

…s the text reference in the method schema. If it is plain text and doesn't match a model in the provider it will be parsed. If there are issues within CloudFormation it will error. i.e application/json having invalid json formation as part of teh draft-04 syntax. This was created this way for instances of `text/html` and `text/plain` not really having a structured layout.

…test that will validate against a YML configuration.

…block and utilizing it throughout the tests.

…r. This will prevent the creation of unnecessary resources.

pgrzesik · 2021-02-25T10:57:53Z

Hello 👋 I've rebased + adjusted the existing changes, I've also introduced the switch to request.schemas and deprecation of http.request.schema configuration. Please let me know what do you think @medikoo 🙇

medikoo

Looks great @pgrzesik ! I have just one minor style suggestion

medikoo · 2021-02-25T15:34:00Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+        ) {
+          this.serverless._logDeprecation(
+            'AWS_API_GATEWAY_SCHEMAS',
+            'Starting with next major version, `http.request.schema` property will be replaced by `http.request.schemas`.'


In CLI logs, we usually do not use back quotes, but double quotes. They look better for wrapping configuration properties

Good catch - fixed 👍

medikoo · 2021-02-25T15:38:35Z

lib/plugins/aws/package/compile/events/apiGateway/lib/requestValidator.js

+    if (!schemaConfig.schema) {
+      definition = schemaConfig;
+    } else {
+      definition = schemaConfig.schema;
+    }
+
+    const modelLogicalId = this.provider.naming.getModelLogicalId(schemaId);
+
+    const validatorLogicalId = this.provider.naming.getValidatorLogicalId(modelLogicalId);
+
+    if (schemaConfig.name) {
+      modelName = schemaConfig.name;
+      validatorName = `${modelName}Validator`;
+    }
+
+    if (schemaConfig.description) {
+      description = schemaConfig.description;
+    }


Thanks @pgrzesik I think I also got a bit lost, as here as you pointed we will deal just with object notation.
Great job on clarifying that in upper part of the code.

medikoo

Excellent 👍

medikoo added cat/aws-event-api-gateway enhancement labels Apr 24, 2020

medikoo reviewed Apr 24, 2020

View reviewed changes

medikoo assigned jmcguffee Apr 24, 2020

medikoo suggested changes Apr 27, 2020

View reviewed changes

medikoo suggested changes Apr 28, 2020

View reviewed changes

jmcguffee changed the title ~~Adding defaults for AWS request schema models to keep naming consistent~~ Adding new feature to allow further customization of API Gateway Models. Apr 30, 2020

medikoo suggested changes Apr 30, 2020

View reviewed changes

jmcguffee requested a review from medikoo May 12, 2020 14:45

medikoo suggested changes May 12, 2020

View reviewed changes

medikoo suggested changes May 28, 2020

View reviewed changes

medikoo added the help wanted label Jun 8, 2020

medikoo suggested changes Aug 24, 2020

View reviewed changes

medikoo removed the help wanted label Aug 24, 2020

medikoo added the help wanted label Oct 7, 2020

medikoo assigned pgrzesik and unassigned jmcguffee Dec 16, 2020

jmcguffee and others added 14 commits February 25, 2021 11:13

Adding defaults for request models to keep naming consistent

9d24df8

Adding defaults for request models to keep naming consistent

3ab7579

Fixing prettier issues.

4f20607

Fixing decreased coverage blocks.

f948f57

Fixing apiGatewayProvider undefined error

7b22782

Updating from feedback from PR Review.

2a69e2f

Refactoring out modela and request validators to new file. Wrote new …

84f195e

…test that will validate against a YML configuration.

Addressing feedback from review.

e61cef1

Addressing issues from review.

ec832ac

Fixing build issues.

ea875d6

Fixing feedback from review. Resolving serverless instance in before …

e260c54

…block and utilizing it throughout the tests.

Only creating models that are used if they are defined in the provide…

9e91623

…r. This will prevent the creation of unnecessary resources.

Adjustments after rebase + addressing some of the PR comments

641d8b4

pgrzesik force-pushed the master branch 5 times, most recently from cc737b6 to 0ba1046 Compare February 25, 2021 10:50

pgrzesik requested a review from medikoo February 25, 2021 10:57

pgrzesik force-pushed the master branch from 0ba1046 to 21af182 Compare February 25, 2021 13:30

medikoo suggested changes Feb 25, 2021

View reviewed changes

docs: Add deprecation plus docs adjustments

f7d9e95

pgrzesik force-pushed the master branch from 21af182 to f7d9e95 Compare February 26, 2021 08:51

pgrzesik requested a review from medikoo February 26, 2021 08:52

medikoo approved these changes Feb 26, 2021

View reviewed changes

pgrzesik changed the title ~~Adding new feature to allow further customization of API Gateway Models.~~ feat(AWS API Gateway): Allow reuse and customization of schema models Feb 26, 2021

pgrzesik merged commit aeb64fd into serverless:master Feb 26, 2021

pgrzesik mentioned this pull request Apr 12, 2021

Request validator limit of 20 #9291

Closed

	describe('AwsCompileFunctions #2', () => {
	after(fixtures.cleanup);

	describe('Destinations', () => {
	it('Should reference function from same service as destination', () =>
	runServerless({
	cwd: fixtures.map.functionDestinations,
	cliArgs: ['package'],
	}).then(serverless => {
	const cfResources = serverless.service.provider.compiledCloudFormationTemplate.Resources;
	const naming = serverless.getProvider('aws').naming;
	const destinationConfig =
	cfResources[naming.getLambdaEventConfigLogicalId('trigger')].Properties.DestinationConfig;

	expect(destinationConfig).to.deep.equal({
	OnSuccess: {
	Destination: { 'Fn::GetAtt': [naming.getLambdaLogicalId('target'), 'Arn'] },
	},
	});
	expect(destinationConfig).to.not.have.property('OnFailure');
	}));

	it('Should support OnFailure destinations', () =>
	fixtures
	.extend('functionDestinations', {
	functions: { trigger: { destinations: { onSuccess: null, onFailure: 'target' } } },
	})
	.then(fixturePath =>
	runServerless({
	cwd: fixturePath,
	cliArgs: ['package'],
	}).then(serverless => {
	const cfResources =
	serverless.service.provider.compiledCloudFormationTemplate.Resources;
	const naming = serverless.getProvider('aws').naming;
	const destinationConfig =
	cfResources[naming.getLambdaEventConfigLogicalId('trigger')].Properties
	.DestinationConfig;

	expect(destinationConfig).to.not.have.property('OnSuccess');
	expect(destinationConfig).to.deep.equal({
	OnFailure: {
	Destination: { 'Fn::GetAtt': [naming.getLambdaLogicalId('target'), 'Arn'] },
	},
	});
	})
	));

	it('Should support ARN to external function as destination', () => {
	const arn = 'arn:aws:lambda:us-east-1:12313231:function:external';
	return fixtures
	.extend('functionDestinations', {
	functions: { trigger: { destinations: { onSuccess: arn } } },
	})
	.then(fixturePath =>
	runServerless({
	cwd: fixturePath,
	cliArgs: ['package'],
	}).then(serverless => {
	const cfResources =
	serverless.service.provider.compiledCloudFormationTemplate.Resources;
	const naming = serverless.getProvider('aws').naming;
	const destinationConfig =
	cfResources[naming.getLambdaEventConfigLogicalId('trigger')].Properties
	.DestinationConfig;

	expect(destinationConfig).to.deep.equal({ OnSuccess: { Destination: arn } });
	})
	);
	});

	it('Should respect `role` setting', () =>
	fixtures
	.extend('functionDestinations', { provider: { role: ' arn:aws:iam::XXXXXX:role/role' } })
	.then(fixturePath =>
	runServerless({
	cwd: fixturePath,
	cliArgs: ['package'],
	}).then(serverless => {
	const cfResources =
	serverless.service.provider.compiledCloudFormationTemplate.Resources;
	const naming = serverless.getProvider('aws').naming;
	const destinationConfig =
	cfResources[naming.getLambdaEventConfigLogicalId('trigger')].Properties
	.DestinationConfig;

	expect(destinationConfig).to.deep.equal({
	OnSuccess: {
	Destination: { 'Fn::GetAtt': [naming.getLambdaLogicalId('target'), 'Arn'] },
	},
	});
	expect(destinationConfig).to.not.have.property('OnFailure');
	})
	));
	});
	});


		const validatorLogicalId = this.provider.naming.getValidatorLogicalId(modelLogicalId);

		if (schemaConfig.name) {

feat(AWS API Gateway): Allow reuse and customization of schema models #7619

feat(AWS API Gateway): Allow reuse and customization of schema models #7619

Conversation

jmcguffee commented Apr 23, 2020 • edited

What did you implement

How can we verify it

Todos

medikoo left a comment

Choose a reason for hiding this comment

jmcguffee commented Apr 24, 2020

jmcguffee commented Apr 24, 2020

jmcguffee commented Apr 24, 2020

codecov-io commented Apr 24, 2020 • edited

Codecov Report

medikoo 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

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

jmcguffee commented Apr 27, 2020

jmcguffee commented Apr 27, 2020

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcguffee commented Apr 30, 2020

medikoo 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

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

jmcguffee commented May 12, 2020

medikoo 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

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

jmcguffee commented May 27, 2020

medikoo commented May 27, 2020

codecov-commenter commented May 27, 2020 • edited by codecov bot

Codecov Report

jmcguffee commented May 27, 2020 • edited

medikoo 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

Choose a reason for hiding this comment

jmcguffee commented Aug 21, 2020

medikoo left a comment

Choose a reason for hiding this comment

jmcguffee commented Apr 23, 2020 •

edited

codecov-io commented Apr 24, 2020 •

edited

codecov-commenter commented May 27, 2020 •

edited by codecov bot

jmcguffee commented May 27, 2020 •

edited