Skip to content
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 - Creating ability to add validators to api gw method without needing a model #2450

Closed
wants to merge 5 commits into from

Conversation

Rondineli
Copy link
Contributor

Issue #, if available: #1403 - Feature improvement

Description of changes: Add ability to add validators if no Model is present. In some cases, when request Parameters is present but no model we still want to add validators to the path. This PR aim add a second way of add a validator to the swagger specs, even if a model is not defined.
In case both are defined (model and RequestValidators) it will add the unique validator to the path, keeping model and requestParameters as part of the swagger spec definitions.

Description of how you validated changes: Create a path with RequestParameters and add a RequestValidators to it. It will update the validators to the final spec, validating the requests as intended.

Checklist:

  • Add/update unit tests using:
    • Correct values
    • Bad/wrong values (None, empty, wrong type, length, etc.)
    • [-] Intrinsic Functions
  • Add/update integration tests
  • make pr passes
  • Update documentation
  • [*] Verify transformed template deploys and application functions as expected
  • [*] Do these changes include any template validations?
    • [-] Did the newly validated properties support intrinsics prior to adding the validations? (If unsure, please review Intrinsic Functions before proceeding).
      • [*] Does the pull request ensure that intrinsics remain functional with the new validations?

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.

@github-actions github-actions bot added pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Jul 20, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2022

Codecov Report

Merging #2450 (b0c30fd) into develop (e7a1496) will increase coverage by 0.92%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2450      +/-   ##
===========================================
+ Coverage    93.58%   94.51%   +0.92%     
===========================================
  Files           90       97       +7     
  Lines         6124     7305    +1181     
  Branches      1260     1524     +264     
===========================================
+ Hits          5731     6904    +1173     
- Misses         183      193      +10     
+ Partials       210      208       -2     
Impacted Files Coverage Δ
samtranslator/region_configuration.py 77.77% <0.00%> (-22.23%) ⬇️
samtranslator/model/codedeploy.py 90.90% <0.00%> (-9.10%) ⬇️
samtranslator/validator/validator.py 91.80% <0.00%> (-3.85%) ⬇️
samtranslator/model/exceptions.py 97.67% <0.00%> (-2.33%) ⬇️
samtranslator/open_api/open_api.py 90.16% <0.00%> (-1.81%) ⬇️
samtranslator/model/s3_utils/uri_parser.py 68.42% <0.00%> (-0.81%) ⬇️
samtranslator/yaml_helper.py 89.47% <0.00%> (-0.53%) ⬇️
samtranslator/translator/logical_id_generator.py 90.62% <0.00%> (-0.29%) ⬇️
samtranslator/model/apigateway.py 96.98% <0.00%> (-0.18%) ⬇️
samtranslator/intrinsics/resource_refs.py 95.83% <0.00%> (-0.17%) ⬇️
... and 49 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Rondineli Rondineli changed the title feat - Creating ability to add validators to api gw method without need a model feat - Creating ability to add validators to api gw method without needing a model Aug 23, 2022
Copy link
Contributor

@aahung aahung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR!

@mndeveci mndeveci removed the stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. label Sep 9, 2022
@Rondineli
Copy link
Contributor Author

@aahung fixed, thanks for the review!!

@Rondineli Rondineli requested a review from aahung September 14, 2022 13:17
Copy link
Contributor

@aahung aahung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

# if we have validator configured to a request parameter
# we want to add it to the editor but remove it from original dict before it iterates
# If validators is present or not, this method will handle the check
self._add_validators(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this should be the correct behaviour. I found it confusing, if the customer defined set the validation to True in the RequestModel but to False in the RequestValidators, why should we disable the validations. I think it is not a good customer experience.

@moelasmar
Copy link
Contributor

Thanks @Rondineli for your contribution. I just have a concern about the customer experience of the new property RequestValidators. My issue is if the customer enables the validations in the requestModel, and disables it in the RequestValidators, I found it confusing for the customers to use the request validators to overwrite the request model.

My understanding of the purpose of this PR is to allow the customers to set validations for a service Path even if there is no model. Let me share my thoughts, and I will also validate it with the team. Is it possible to allow the customer to do that by making the Model attribute to be optional in the RequestModel property (https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/sam-property-function-requestmodel.html). So the customer can enable the validations even if there is no model.

@Rondineli
Copy link
Contributor Author

@moelasmar thanks for replying. That makes sense and it is a valid concern. My thoughts were get a way of not change the actual behaviour as it can break existent config. Let me know the best way to proceed then I can change my PR to meet the requirements. The idea of changing the model to no be required also sounds confusing as it still having to have a "RequestModel" on the template.

We could maybe validate If the template has a model with a validator and if both are present, raise an exception saying that it must have either a validator on its model block to the method or a validator on its request parameters to the method.

In any case I would be happy to change the PR to meet the desired behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants