feat: cors httpapi #1381

ShreyaGangishetty · 2020-01-13T18:38:13Z

Issue #, if available:
#1295

Description of changes:

Description of how you validated changes:

Checklist:

Write/update tests
make pr passes
Update documentation
Verify transformed template deploys and application functions as expected
Add/update example to examples/2016-10-31

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

codecov-io · 2020-01-13T18:42:37Z

Codecov Report

Merging #1381 into develop will decrease coverage by 0.42%.
The diff coverage is 63.63%.

@@             Coverage Diff             @@
##           develop    #1381      +/-   ##
===========================================
- Coverage    94.36%   93.93%   -0.43%     
===========================================
  Files           78       78              
  Lines         4685     4686       +1     
  Branches       937      953      +16     
===========================================
- Hits          4421     4402      -19     
- Misses         121      132      +11     
- Partials       143      152       +9

Impacted Files	Coverage Δ
samtranslator/plugins/globals/globals.py	`99.02% <ø> (-0.03%)`	⬇️
samtranslator/model/sam_resources.py	`94.05% <ø> (-0.02%)`	⬇️
samtranslator/model/api/http_api_generator.py	`89.55% <57.14%> (-8.57%)`	⬇️
samtranslator/open_api/open_api.py	`90.19% <70.37%> (-3.32%)`	⬇️
samtranslator/plugins/exceptions.py	`83.33% <0%> (-2.39%)`	⬇️
samtranslator/model/iam.py	`92.85% <0%> (-1.43%)`	⬇️
samtranslator/translator/arn_generator.py	`86.95% <0%> (-0.55%)`	⬇️
samtranslator/model/sqs.py	`93.33% <0%> (-0.42%)`	⬇️
samtranslator/validator/validator.py	`95.23% <0%> (-0.42%)`	⬇️
... and 12 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 77fa3b9...5ddd1b0. Read the comment docs.

keetonian · 2020-02-19T21:53:28Z

tests/translator/input/http_api_existing_openapi.yaml

      DefinitionBody:
        info:
          version: '1.0'
          title:
            Ref: AWS::StackName
+        # when this is specified `CorsConfiguration` property is ignored


Good question about this. What we need to decide is if CorsConfiguration should override, merge, or be ignored if there already exists a configuration in the openapi document.

This brings up the bigger question of how do we do this in other areas of SAM for HTTP APIs. I will need to check on this, but I believe that:

If a configuration doesn't exist in the openapi document, SAM should add it

If a configuration exists, SAM should have a set pattern of either ignoring or overriding it

As we discussed offline, I think we need to merge the CorsConfiguration with open api cors extension header if exists and override by giving preference to values in CorsConfiguration property. This way SAM can give the flexibility for template authors to override some of the cors headers mentioned in CorsConfiguration property and also will be able to add other/new Cors headers irrespective of SAMs support

updated the code to merge and override CorsConfiguration

We are now ignoring the configurations here if there is a corresponding configuration in CorsConfiguration (remove comment)

tests/translator/input/http_api_explicit_stage.yaml

alexw91 · 2020-02-21T18:11:37Z

Codecov Report

Merging #1381 into develop will decrease coverage by 0.34%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1381      +/-   ##
===========================================
- Coverage    94.36%   94.01%   -0.35%     
===========================================
  Files           78       78              
  Lines         4685     4779      +94     
  Branches       937      965      +28     
===========================================
+ Hits          4421     4493      +72     
- Misses         121      133      +12     
- Partials       143      153      +10

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 77fa3b9...5cd7618. Read the comment docs.

samtranslator/model/api/http_api_generator.py

examples/2016-10-31/http_api_cors/template.yaml

praneetap

Great work so far! few comments in the review.

examples/2016-10-31/http_api_cors/template.yaml

samtranslator/model/api/http_api_generator.py

samtranslator/model/sam_resources.py

samtranslator/model/api/http_api_generator.py

examples/2016-10-31/http_api_cors/README.md

samtranslator/open_api/open_api.py

codecov-io · 2020-02-27T18:50:10Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@5088d94). Click here to learn what that means.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             develop    #1381   +/-   ##
==========================================
  Coverage           ?   94.14%           
==========================================
  Files              ?       78           
  Lines              ?     4780           
  Branches           ?      965           
==========================================
  Hits               ?     4500           
  Misses             ?      130           
  Partials           ?      150

Impacted Files	Coverage Δ
samtranslator/plugins/globals/globals.py	`99.05% <ø> (ø)`
samtranslator/model/sam_resources.py	`94.09% <ø> (ø)`
samtranslator/model/api/http_api_generator.py	`93.19% <66.66%> (ø)`

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 5088d94...6b9deb1. Read the comment docs.

keetonian · 2020-02-28T00:10:03Z

tests/translator/input/http_api_existing_openapi.yaml

      DefinitionBody:
        info:
          version: '1.0'
          title:
            Ref: AWS::StackName
+        # when this is specified `CorsConfiguration` property is ignored


We are now ignoring the configurations here if there is a corresponding configuration in CorsConfiguration (remove comment)

samtranslator/model/api/http_api_generator.py

ShreyaGangishetty · 2020-02-28T00:37:28Z

In this PR, CorsConfiguration overrides the open api specification when intrinsic functions are present instead of merging both because it is not possible to resolve all the combinations of intrinsics at open api level.

alexw91 · 2020-02-28T02:44:20Z

Codecov Report

Merging #1381 into develop will decrease coverage by 0.25%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1381      +/-   ##
===========================================
- Coverage    94.40%   94.14%   -0.26%     
===========================================
  Files           78       78              
  Lines         4716     4780      +64     
  Branches       948      965      +17     
===========================================
+ Hits          4452     4500      +48     
- Misses         121      130       +9     
- Partials       143      150       +7

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 b834f89...6b9deb1. Read the comment docs.

keetonian self-assigned this Jan 15, 2020

ShreyaGangishetty force-pushed the cors-httpapi branch from e70d418 to d3e0d76 Compare February 19, 2020 17:37

keetonian reviewed Feb 19, 2020

View reviewed changes

Shreya Gangishetty added 6 commits February 21, 2020 09:53

feat: add cors configuration for httpapi

c0856b3

fix: typo

a4e5134

fix: updated tests

4839946

fix: black errors and update test

410aa8d

docs: add example

9d1ac0f

fix: override cors config

f87ad72

ShreyaGangishetty force-pushed the cors-httpapi branch from d1b4649 to f87ad72 Compare February 21, 2020 17:57

fix: add test for merge and override

5ddd1b0

keetonian reviewed Feb 21, 2020

View reviewed changes

samtranslator/model/api/http_api_generator.py Outdated Show resolved Hide resolved

samtranslator/model/api/http_api_generator.py Outdated Show resolved Hide resolved

samtranslator/model/api/http_api_generator.py Outdated Show resolved Hide resolved

praneetap reviewed Feb 21, 2020

View reviewed changes

samtranslator/model/api/http_api_generator.py Outdated Show resolved Hide resolved

examples/2016-10-31/http_api_cors/template.yaml Outdated Show resolved Hide resolved

praneetap reviewed Feb 21, 2020

View reviewed changes

ShreyaGangishetty changed the title ~~Cors httpapi~~ feat: cors httpapi Feb 26, 2020

Shreya Gangishetty added 9 commits February 26, 2020 17:31

feat: add cors configuration for httpapi

2ce8ea1

fix: typo

122d1e1

fix: updated tests

26b0c68

fix: black errors and update test

bd376e1

docs: add example

1130dd5

fix: override cors config

3311c00

fix: add test for merge and override

2acbae2

fix: support intrinsics

b60d93a

fix: merge conflicts

b7fe6e3

ShreyaGangishetty changed the base branch from develop to master February 27, 2020 17:42

ShreyaGangishetty changed the base branch from master to develop February 27, 2020 17:42

fix: updated doc

5cd7618

keetonian reviewed Feb 27, 2020

View reviewed changes

examples/2016-10-31/http_api_cors/README.md Outdated Show resolved Hide resolved

keetonian reviewed Feb 27, 2020

View reviewed changes

samtranslator/open_api/open_api.py Outdated Show resolved Hide resolved

keetonian reviewed Feb 27, 2020

View reviewed changes

samtranslator/open_api/open_api.py Outdated Show resolved Hide resolved

keetonian reviewed Feb 27, 2020

View reviewed changes

samtranslator/open_api/open_api.py Outdated Show resolved Hide resolved

praneetap approved these changes Feb 27, 2020

View reviewed changes

fix: pr comments

74965d1

ShreyaGangishetty force-pushed the cors-httpapi branch from 211a6bb to 74965d1 Compare February 28, 2020 00:04

keetonian reviewed Feb 28, 2020

View reviewed changes

keetonian approved these changes Feb 28, 2020

View reviewed changes

Shreya Gangishetty added 2 commits February 27, 2020 16:24

fix: update comments

5088d94

fix: merge conflicts

6b9deb1

ShreyaGangishetty merged commit c5221fc into aws:develop Feb 28, 2020

ShreyaGangishetty mentioned this pull request Mar 3, 2020

Release tracker for SAM v1.22.0 release #1486

Closed

adrai mentioned this pull request Mar 13, 2020

FindInMap does not work in AllowOrigins (CorsConfiguration) #1515

Closed

jamesiri unassigned keetonian Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: cors httpapi #1381

feat: cors httpapi #1381

ShreyaGangishetty commented Jan 13, 2020 •

edited

Loading

codecov-io commented Jan 13, 2020 •

edited

Loading

keetonian Feb 19, 2020

ShreyaGangishetty Feb 19, 2020

ShreyaGangishetty Feb 21, 2020

keetonian Feb 28, 2020

alexw91 commented Feb 21, 2020 •

edited

Loading

praneetap left a comment

codecov-io commented Feb 27, 2020 •

edited

Loading

keetonian Feb 28, 2020

ShreyaGangishetty commented Feb 28, 2020

alexw91 commented Feb 28, 2020

feat: cors httpapi #1381

feat: cors httpapi #1381

Conversation

ShreyaGangishetty commented Jan 13, 2020 • edited Loading

codecov-io commented Jan 13, 2020 • edited Loading

Codecov Report

keetonian Feb 19, 2020

Choose a reason for hiding this comment

ShreyaGangishetty Feb 19, 2020

Choose a reason for hiding this comment

ShreyaGangishetty Feb 21, 2020

Choose a reason for hiding this comment

keetonian Feb 28, 2020

Choose a reason for hiding this comment

alexw91 commented Feb 21, 2020 • edited Loading

Codecov Report

praneetap left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 27, 2020 • edited Loading

Codecov Report

keetonian Feb 28, 2020

Choose a reason for hiding this comment

ShreyaGangishetty commented Feb 28, 2020

alexw91 commented Feb 28, 2020

Codecov Report

ShreyaGangishetty commented Jan 13, 2020 •

edited

Loading

codecov-io commented Jan 13, 2020 •

edited

Loading

alexw91 commented Feb 21, 2020 •

edited

Loading

codecov-io commented Feb 27, 2020 •

edited

Loading