Fix duplicate OPTIONS method error for paths with trailing slash#3844
Fix duplicate OPTIONS method error for paths with trailing slash#3844abhishektang wants to merge 3 commits intoaws:developfrom
Conversation
4faeba3 to
6b3afda
Compare
| try: | ||
| editor.add_cors( # type: ignore[no-untyped-call] | ||
| path, | ||
| normalized_path, |
There was a problem hiding this comment.
We should continue using path here (instead of normalized_path).
From an API Gateway perspective it's equivalent, but by still using path we keep backwards compatibility, in the sense that existing templates (without duplicated paths) continue generating exactly the same resources before and after this change (and changing this won't remove the fact that we will skip the duplicated normalized paths, because they're handled above)
There was a problem hiding this comment.
I see the edge case you are talking about here as I didn't consider the API Gateway treats them equally, but don't you think using path is non-deterministic and using normalized_path is deterministic.
Would you be open to a sorted iteration approach instead? It gives determinism without breaking the path lookup:
for path in sorted(editor.iter_on_path(), key=lambda p: (p.rstrip("/"), p)):
This guarantees /path is always iterated before /path/, so OPTIONS always lands on the non-trailing-slash path, or should I be worried about the latency of the code by introducing the sort method. Happy to go with whatever direction you prefer.
There was a problem hiding this comment.
I'm thinking mainly on what happens in the case when there are no duplicated options really. The cors configuration will be added to a normalized path, which may be different than what the user has configured. Again, in theory it shouldn't affect the behavior in the end because for API GW it's the same, but it's just different than what currently happens before this change.
It's usually frowned upon to change SAM's behavior (obviously except on previously failing scenarios), because even if the final state is equivalent, changing the generated resources can cause CFN to "redeploy" a resource without the user making changes, which can confuse users. (and I guess we don't have an existing test case with this, that would have changed)
Same thing happens with a "sorted" iteration. After that, the generated API GW will have the paths in a different order than before this change.
Also, my understanding is that this will still be consistent, where the item added will be "the first one" configured, since iter_on_path should return the items in the same order (dict.items() in iter_in_path returns elements in the order of insertion)
There was a problem hiding this comment.
Let me know if that makes sense to you, because I'm still open to be convinced if you have a strong argument.
There was a problem hiding this comment.
Yes, that clarifies everything for me. I have made the change and pushed the new code, please review the code and kindly let me know if there are any further changes required. Thanks a lot @valerena for explaining the whole scenario!
Fixes aws#3816 When CORS is enabled on an API with paths that differ only by a trailing slash (e.g., /datasets and /datasets/), API Gateway would throw an error: 'Duplicate method OPTIONS found for resource /datasets. Only one is allowed.' This occurs because API Gateway treats /path and /path/ as the same resource, but SAM was adding OPTIONS methods to both paths separately. Solution: - Added path normalization in ApiGenerator._add_cors() method - Paths are normalized by removing trailing slashes (except for root '/') - Track normalized paths in a set to skip duplicates - Ensures only one OPTIONS method is added per normalized path Testing: - Added unit test test_add_cors_with_trailing_slash_paths to verify fix - All existing CORS integration tests (68 tests) pass without regression - Tested with real-world production templates with multiple HTTP methods
Address review feedback: use normalized_path instead of path when adding CORS OPTIONS method. This ensures consistent behavior regardless of which path (/datasets vs /datasets/) appears first in the template. Fixes aws#3816
Description
Fixes #3816
This PR fixes the issue where enabling CORS on an API with paths that differ only by a trailing slash (e.g.,
/datasetsand/datasets/) causes API Gateway to throw an error:Problem
When CORS is enabled, SAM adds OPTIONS methods to all paths. However, API Gateway treats
/pathand/path/as the same resource. If both paths exist in the template, SAM was adding OPTIONS to both separately, causing a duplicate method error.Solution
The fix normalizes paths before adding CORS OPTIONS methods:
/)Changes
Modified files:
samtranslator/model/api/api_generator.py: Added path normalization in_add_cors()methodtests/model/api/test_api_generator.py: Added unit testtest_add_cors_with_trailing_slash_pathsTesting
✅ Unit Tests
test_add_cors_with_trailing_slash_pathsthat verifies:/datasets(POST) and/datasets/(PUT) are correctly handled✅ Integration Tests
✅ Real-world Validation
Related Issues
Closes #3816
Checklist:
developbranch