feat: Swagger api operation simple validation #2023
Conversation
| @@ -620,15 +624,15 @@ def set_path_default_apikey_required(self, path): | |||
| :param string path: Path name | |||
| """ | |||
|
|
|||
| valid_non_http_method_sections = ["parameters", "summary", "description", "$ref", "servers"] | |||
There was a problem hiding this comment.
This seems to match the OpenAPI 3.0. Looking at references, this class seems to be specific to Rest APIs. Rest APIs support both OpenAPI 2.0 and 3.0: https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-export-api.html
I think we may need to think deeper about this and the recent revert: #2021 as a swagger validation overhaul.
There was a problem hiding this comment.
This is a block-list while #2021 was an allow-list. I mean, we know as a fact that ["parameters", "summary", "description", "$ref", "servers"] are not API operations (HTTP methods)
There was a problem hiding this comment.
Maybe asking this a different way. If a customer is using OpenAPI 2.0 and specifies "servers" in the path object, should SAM fail with this being invalid? I know this just ignores the values and moves on, but not sure where we should fail this.
If SAM doesn't fail in the case above, then API Gateway should reject it. Which could be enough, just poking at this to see if we really need or want to do these deeper level of validation or not.
There was a problem hiding this comment.
Are you asking whether we should validate that the API definition complies with the OpenApi specification or not?
If so, I think this is not the responsibility of SAM. Here, we are trying to do our best to raise 4xx error instead of 5xx when applicable. So, if the API definition is invalid but it doesn't cause SAM to fail, I believe it shouldn't be a concern for SAM. Agree?
There was a problem hiding this comment.
I assumed this was in part validation we do.
Your reasoning makes sense to me.
There was a problem hiding this comment.
Change looks good.
Can we add test that do the translator so we can build out our test coverage? https://github.com/aws/serverless-application-model/tree/develop/tests/translator/input
Codecov Report
@@ Coverage Diff @@
## develop #2023 +/- ##
===========================================
+ Coverage 93.58% 93.67% +0.08%
===========================================
Files 90 90
Lines 6080 6084 +4
Branches 1247 1248 +1
===========================================
+ Hits 5690 5699 +9
+ Misses 182 179 -3
+ Partials 208 206 -2
Continue to review full report at Codecov.
|
mgrandis
changed the title
|
I think we already solved this in a different way (looking at the conflicts). Therefore closing this pr. |
Issue #, if available:
Description of changes:
The method
method_definition_has_integrationinSwaggerEditorclass expects adictobject, it raise 5XX if it receives a String instead. This change is to raise 4XX error instead as this is an invalid User template.Description of how you validated changes:
Unit testing
Checklist:
make prpassesExamples?
Please reach out in the comments, if you want to add an example. Examples will be
added to
sam initthrough 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.