From c65ac173c743ce677a8acf2ad01d4170abbc944c Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Mon, 24 Oct 2016 09:43:00 -0700 Subject: [PATCH] Validate you don't have OPTIONS when cors=True You'll get an error message from API gateway about a conflict exception which isn't that helpful. By adding it to the validation step here, we get a much clearer error message: ``` ValueError: Route entry cannot have both cors=True and methods=['OPTIONS', ...] configured. When CORS is enabled, an OPTIONS method is automatically added for you. Please remove 'OPTIONS' from the list of configured HTTP methods for: /badview ``` Closes #142. --- CHANGELOG.rst | 2 ++ chalice/deployer.py | 33 +++++++++++++++++++++++++++------ tests/unit/test_deployer.py | 10 ++++++++++ 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 46b73d221..053c911fe 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -9,6 +9,8 @@ Next Release (TBD) (`#139 `__) * Raise errors when unknown kwargs are provided to ``app.route(...)`` (`#144 `__) +* Raise validation error when configuring CORS and an OPTIONS method + (`#142 `__) 0.3.0 diff --git a/chalice/deployer.py b/chalice/deployer.py index fd3e95ee0..bcff48398 100644 --- a/chalice/deployer.py +++ b/chalice/deployer.py @@ -133,7 +133,7 @@ def build_url_trie(routes): :return: A prefix trie of URL patterns. """ - _validate_routes(routes) + validate_routes(routes) root = node('', '/') for route in routes: if route == '/': @@ -170,21 +170,42 @@ def validate_configuration(config): """ routes = config.chalice_app.routes - _validate_routes(routes) + validate_routes(routes) _validate_manage_iam_role(config) -def _validate_routes(routes): +def validate_routes(routes): # type: (Dict[str, Any]) -> None # We're trying to validate any kind of route that will fail # when we send the request to API gateway. # We check for: # # * any routes that end with a trailing slash. - for route in routes: - if route != '/' and route.endswith('/'): + for route_name, route_entry in routes.items(): + if route_name != '/' and route_name.endswith('/'): raise ValueError("Route cannot end with a trailing slash: %s" - % route) + % route_name) + if route_entry is not None: + # This 'is not None' check is not strictly needed. + # It's used because some of the tests don't populate + # a route_entry when creating test routes. + # This should be cleaned up. + _validate_route_entry(route_name, route_entry) + + +def _validate_route_entry(route_url, route_entry): + # type: (str, app.RouteEntry) -> None + if route_entry.cors: + # If the user has enabled CORS, they can't also have an OPTIONS method + # because we'll create one for them. API gateway will raise an error + # about duplicate methods. + if 'OPTIONS' in route_entry.methods: + raise ValueError( + "Route entry cannot have both cors=True and " + "methods=['OPTIONS', ...] configured. When " + "CORS is enabled, an OPTIONS method is automatically " + "added for you. Please remove 'OPTIONS' from the list of " + "configured HTTP methods for: %s" % route_url) def _validate_manage_iam_role(config): diff --git a/tests/unit/test_deployer.py b/tests/unit/test_deployer.py index 7629a44ab..cb61896b9 100644 --- a/tests/unit/test_deployer.py +++ b/tests/unit/test_deployer.py @@ -12,6 +12,7 @@ from chalice.deployer import APIGatewayMethods from chalice.deployer import FULL_PASSTHROUGH, ERROR_MAPPING from chalice.deployer import validate_configuration +from chalice.deployer import validate_routes from chalice.deployer import Deployer from chalice.app import RouteEntry, ALL_ERRORS from chalice.app import Chalice @@ -427,3 +428,12 @@ def test_lambda_deployer_repeated_deploy(): # And should result in the lambda function being updated with the API. aws_client.update_function_code.assert_called_with( 'appname', 'package contents') + + +def test_cant_have_options_with_cors(sample_app): + @sample_app.route('/badcors', methods=['GET', 'OPTIONS'], cors=True) + def badview(): + pass + + with pytest.raises(ValueError): + validate_routes(sample_app.routes)