feat: Add ValidateBody/ValidateParameters attribute to setup API model validators

[Rondineli]

Issue #, if available: #1403

Description of changes: Adding a request validator to the method when a model is defined and set required to True

Description of how you validated changes:
Create a sam template like the following:

Resources:
  HtmlFunction:
    Type: AWS::Serverless::Function
    Properties:
      CodeUri: s3://sam-demo-bucket/member_portal.zip
      Handler: index.gethtml
      Runtime: nodejs12.x
      Events:
        GetHtml:
          Type: Api
          Properties:
            RestApiId: HtmlApi
            Path: /
            Method: get
            RequestModel:
              Model: User
              Required: true
              ValidateBody: true
              ValidateParameters: true

  HtmlApi:
    Type: AWS::Serverless::Api
    Properties:
      StageName: Prod
      Models:
        User:
          type: object
          properties:
            username:
              type: string

Then on the swagger definitions it should create a validation definition with the path-method-validator format and set it on the integration. Then on the console, we should see this flagged:

Checklist:

• [ x ] Write/update tests
• [ x ] make pr passes
• Update documentation
• [ x ] Verify transformed template deploys and application functions as expected

Examples?

Please reach out in the comments, if you want to add an example. Examples will be
added to sam init through https://github.com/awslabs/aws-sam-cli-app-templates/

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

[gfechio]

+1

[AllanOricil]

@Rondineli How would the model validator work with a GET? I thought we could use it only to validate body on POST and PATCH I never really passed a body to a GET so that is why I am asking.

[Rondineli]

@Rondineli How would the model validator work with a GET? I thought we could use it only to validate body on POST and PATCH I never really passed a body to a GET so that is why I am asking.

Exist a possibility of passing a body request trouh a get, yes: example:

curl -XGET https://<apigw id>.execute-api.us-east-1.amazonaws.com/test/ -H "Content-Type: application/json" -d '{"message": "foo"}'

Then on lambda event:

{'resource': '/', 'path': '/', 'httpMethod': 'GET', 'headers': {'accept': '*/*', 'content-type': 'application/json', ..., 'body': '{"message": "foo"}', 'isBase64Encoded': False}

If you define a model you can validate the body on those methods.

[jfuss]

@Rondineli Unfortunately we cannot accept this in the current state. The Required key is not meant to also add parameter validation. It will only add the keys to make the model required.

In order to not break existing customers (magically add validation into their API), this needs to be a separate property within the Spec. Are you still interesting in contributing this? We will want to understand the design of the property at a high level, could be done in a comment here, and then we can go with code changes once we agree on the Spec design.

[Rondineli]

@jfuss Got it! Under the issue comments there are comments around maybe inside of the Model block, adding two new parameters: ValidateBody and ValidateRequest options, this way we can keep things as it is and add those 2 new fields to set the validators. I would be happy to change my PR to achieve that if you are ok :)

[Rondineli]

would be something like:

Resources:
  HtmlFunction:
    Type: AWS::Serverless::Function
    Properties:
      CodeUri: s3://sam-demo-bucket/member_portal.zip
      Handler: index.gethtml
      Runtime: nodejs12.x
      Events:
        GetHtml:
          Type: Api
          Properties:
            RestApiId: HtmlApi
            Path: /
            Method: get
            RequestModel:
              Model: User
              Required: true
              ValidateBody: true
              ValidateRequest: true

if they are not present on the config, I don't add the validator.

[jfuss]

@Rondineli

...
RequestModel:
              Model: User
              Required: true
              ValidateBody: true
              ValidateRequest: true

That looks reasonable to me. I think that makes sense. It's possible to condense this into one property too.

RequestModel:
              Model: User
              Required: true
              Validate: # Accepts ValidateBody, ValidateRequest, NoValue

I think I like your a little better. Less conditional logic to handle and makes intrinsic a little cleaner.

Side note:
Super top of mind to me. SAM doesn't have good support for intrinsics. Mostly because we would have to reimplement CloudFormation's behavior and CloudFormation doesn't resolve intrinsics before SAM is invoked. So in either way, we will want to make sure we have tests around the case for a customer doing "Ref: Param" or "FindInMap: ...", etc. It's ok to explicitly not support intrinsics but we will have to do some basic checks to ensure thing work properly. If the values of the Properties are pass throughs to the underlying CloudFormation resources, that is best (as things will just work). This may impact implementation so trying to bring in up early.

[Rondineli]

@Rondineli

...
RequestModel:
              Model: User
              Required: true
              ValidateBody: true
              ValidateRequest: true

That looks reasonable to me. I think that makes sense. It's possible to condense this into one property too.

RequestModel:
              Model: User
              Required: true
              Validate: # Accepts ValidateBody, ValidateRequest, NoValue

I think I like your a little better. Less conditional logic to handle and makes intrinsic a little cleaner.

Side note:
Super top of mind to me. SAM doesn't have good support for intrinsics. Mostly because we would have to reimplement CloudFormation's behavior and CloudFormation doesn't resolve intrinsics before SAM is invoked. So in either way, we will want to make sure we have tests around the case for a customer doing "Ref: Param" or "FindInMap: ...", etc. It's ok to explicitly not support intrinsics but we will have to do some basic checks to ensure thing work properly. If the values of the Properties are pass throughs to the underlying CloudFormation resources, that is best (as things will just work). This may impact implementation so trying to bring in up early.

ok! Will make a PR with those 2 fields and to set only booleans, I like the idea of not having an intrinsics better as there is a way of setting up customised validators trough swagger body definitions. I think set those two fields (as the validators only supports those two anyways) the only case I would see set a custom validator is to change the name of it, and it can be done trough swagger definitions.

Ok, will change my PR to achieve this then! Thanks!

[codecov-commenter]

Codecov Report

Merging #2026 (be312b9) into develop (be9b9d9) will increase coverage by 0.28%.
The diff coverage is 93.93%.

@@             Coverage Diff             @@
##           develop    #2026      +/-   ##
===========================================
+ Coverage    93.58%   93.87%   +0.28%     
===========================================
  Files           90       91       +1     
  Lines         6124     6201      +77     
  Branches      1260     1272      +12     
===========================================
+ Hits          5731     5821      +90     
+ Misses         183      175       -8     
+ Partials       210      205       -5     

Impacted Files	Coverage Δ
samtranslator/swagger/swagger.py	93.81% <92.00%> (+0.44%)	⬆️
samtranslator/model/eventsources/push.py	91.05% <100.00%> (+0.12%)	⬆️
samtranslator/model/apigateway.py	97.15% <0.00%> (ø)
samtranslator/model/sam_resources.py	91.23% <0.00%> (ø)
samtranslator/translator/translator.py	97.18% <0.00%> (ø)
samtranslator/feature_toggle/dialup.py	100.00% <0.00%> (ø)
samtranslator/model/api/api_generator.py	94.39% <0.00%> (+0.03%)	⬆️
samtranslator/translator/logical_id_generator.py	100.00% <0.00%> (+9.09%)	⬆️
samtranslator/feature_toggle/feature_toggle.py	100.00% <0.00%> (+12.16%)	⬆️

---

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 be9b9d9...be312b9. Read the comment docs.

[Rondineli]

@jfuss PR is ready to a re-review. :)

[codepay4999]

would be great to have this reviewed and merged

[jfuss]

@Rondineli Thanks for updating the PR. I haven't gotten a chance to review yet but on my list to do by the end of the week (6/4).

[Rondineli]

Thanks @Rondineli for the PR and also, seconding Jacob, the extensive tests with intrinsics/odd values!

Added a few remarks.

Thanks for reviewing this and pointing out the improvements. I fixed every comment!

[tking2096]

On the subject of "adding N validators" and not knowing what validation API Gateway might add in the future:

Why not at just add RequestValidatorId to the event and let the user reference a validator that's defined in the template? This way, if API Gateway adds to the RequestValidator's capabilities, SAM doesn't have to worry about keeping up. As you mentioned, there are limited combinations so the user would at most need to define 3 validator resources. What's the point of auto-generating the validator(s)?

[jfuss]

@Rondineli I appreciate the patience from our end and working through everything. I have one additional comment (based upon some other changes going on). If you could address that, I think we will be in really good shape to get this approved on our side and work through our release process (doc updates, service updates, etc).

[jfuss]

We have been moving away from this kind of access pattern and moving towards what is in this pr: https://github.com/aws/serverless-application-model/pull/2031/files

The reason is the normalization causes KeyErrors when methods like "any" are used, since that will get translated to x-amazon-apigateway-any-method. Can you match what is in the PR I linked? for method_definition in self.get_method_contents(method):

[Rondineli]

@jfuss done!

[Rondineli]

@jfuss thinking a bit more about this last change, it doesn't seems right to me:

for method_name, method in self.get_path(path).items():
            # It is possible that the method could have two definitions in a Fn::If block.
            for method_definition in self.get_method_contents(method):
                # If no integration given, then we don't need to process this

So, it would be something like:

        for method_name, method in self.get_path(path).items():
            # Adding it to only path  
            if method_name == normalized_method_name:
                for method_definition in self.get_method_contents(method):

right?

[Rondineli]

without checking the method_name with the normalized_method_name it would add this to all methods for the given path.

[jfuss]

Seems reasonable to me. I am more comfortable with this since it is a new code path (unlikely to accidentally break existing customers for something we are unaware of).

[jfuss]

@Rondineli Thank you for all the effort and time spent on the contribution. It is very much appreciated.

As for next steps: I need to kick of some processes internally (docs for example). I will eventually merge this into develop (later this week). Just giving you the heads up and communicate no further action on you.

[ChristianET-DS]

this will be only for the requestmodel or also for requestparameter ?

[Rondineli]

this will be only for the requestmodel or also for requestparameter ?

ValidateRequestBody and validateRequestParams:

ValidateBody: true
              ValidateParameters: true

[AllanOricil]

Congratulations @Rondineli
You did an amazing work.
Im looking forward to test it. How can I use the CLI using this aws:develop branch? Is there a tutorial somewhere that helps me to build the cli from the develop branch?

[iRoachie]

Hey guys, if I understand this code correctly, no validator will be created if Model is not set correct?

Does this mean in a scenario where I just want to validate if a user is passing in a query param that I have to create a "fake" model anyway to create this?

e.g

Properties:
  Method: get
  RequestParameters:
    - method.request.querystring.query:
        Required: true
  RequestModel:
    ValidateParameters: true

^ This creates the parameter but no validator.

If I create a dummy model, however, then it works. Is this something that should be added (open a new issue)? Just trying to gauge if it was the intention of this PR to support this.

[AllanOricil]

@iRoachie how are you using these changes if they are still on aws:develop branch?

[iRoachie]

@AllanOricil by the looks of the release notes, this is released no? https://github.com/aws/serverless-application-model/releases/tag/v1.39.0

[AllanOricil]

@iRoachie great. I will give it a try :D

[Rondineli]

Hey guys, if I understand this code correctly, no validator will be created if Model is not set correct?

Does this mean in a scenario where I just want to validate if a user is passing in a query param that I have to create a "fake" model anyway to create this?

e.g

Properties:
  Method: get
  RequestParameters:
    - method.request.querystring.query:
        Required: true
  RequestModel:
    ValidateParameters: true

^ This creates the parameter but no validator.

If I create a dummy model, however, then it works. Is this something that should be added (open a new issue)? Just trying to gauge if it was the intention of this PR to support this.

the validators supposed to work with models, the model is not "dummy", it is where you will set the type of data you are validating, ie: query is part of a querystring, but is it a number, string? The model needs to exist to then set a validator.

[iRoachie]

@Rondineli in the console or using cdk (which I ended up using), no model is needed to validate the presence of a query string parameter.

I should be able to perform the same in sam but the implementation of this PR requires a model to be created.

[stevehogdahl]

I have a question regarding the implementation of Models. Our architecture uses the implicitly created AWS::Serverless::Api resource rather than creating the resource manually and adding every Function (70-80) to it. I'm trying to create a AWS::ApiGateway::Model resource and set the RestApiId to the implicit resource (ServerlessRestApi). This fails the sam validation with the following error "the related API does not contain valid Models". Is the only way to implement this to explicitly create a AWS::Serverless::Api resource, add the models, then add everything to the explicit resource rather than using the implicitly created one.

[santiperone]

https://github.com/aws/serverless-application-model/releases/tag/v1.39.0

Hey guys, if I understand this code correctly, no validator will be created if Model is not set correct?
Does this mean in a scenario where I just want to validate if a user is passing in a query param that I have to create a "fake" model anyway to create this?
e.g

Properties:
  Method: get
  RequestParameters:
    - method.request.querystring.query:
        Required: true
  RequestModel:
    ValidateParameters: true

^ This creates the parameter but no validator.

If I create a dummy model, however, then it works. Is this something that should be added (open a new issue)? Just trying to gauge if it was the intention of this PR to support this.

the validators supposed to work with models, the model is not "dummy", it is where you will set the type of data you are validating, ie: query is part of a querystring, but is it a number, string? The model needs to exist to then set a validator.

I'm having the same issue as @iRoachie. On a GET method you typically want to validate only requestParameters, not RequestModel. Nevertheless you have to define a Model in orther to activate de parameter validation. Unless you can somehow use a Model to define querystringparameters, which is unclear to me.