Set operationName in local API request context

[ramosbugs]

Which issue(s) does this change fix?

Fixes #2457.

Why is this change necessary?

The operationName field is currently missing from the API Gateway proxy request context, which prevents certain Lambda functions from working correctly in the local environment.

How does it address the issue?

This PR adds the missing operationName based on the Swagger operationId, as the production AWS Gateway does.

What side effects does this change have?

Hopefully none :-)

Checklist

• Write design document (Do I need to write a design document?)
• Write unit tests
• Write/update functional tests
• Write/update integration tests
• make pr passes
• make update-reproducible-reqs if dependencies were changed
• Write documentation

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

[ramosbugs]

Oh shoot, this doesn't quite fix the issue because of the route deduping done here:

aws-sam-cli/samcli/lib/providers/api_collector.py

Line 104 in 3ee79db

routes = self.dedupe_function_routes(self.routes)

The deduping combines all of the routes that share the same path (but different methods) into a single Route object:

aws-sam-cli/samcli/lib/providers/api_collector.py

Lines 152 to 166 in 3ee79db

	for route in routes:
	key = "{}-{}".format(route.function_name, route.path)
	config = grouped_routes.get(key, None)
	methods = route.methods
	if config:
	methods += config.methods
	sorted_methods = sorted(methods)
	grouped_routes[key] = Route(
	function_name=route.function_name,
	path=route.path,
	methods=sorted_methods,
	event_type=route.event_type,
	payload_format_version=route.payload_format_version,
	)
	return list(grouped_routes.values())

However, they should be treated as separate routes if they have different operation names. Given this, should we still be deduping the routes at all? I can't quite tell from the comments what the original rationale was. It looks like this was introduced in #1239. @viksrivat: do you happen to remember?

[jfuss]

@ramosbugs SAM CLI uses Flask under the hood to create an API Gateway like endpoint. We need to dedup the urls and methods to wire it up with Flask.

[ramosbugs]

@ramosbugs SAM CLI uses Flask under the hood to create an API Gateway like endpoint. We need to dedup the urls and methods to wire it up with Flask.

@jfuss: thanks for the quick reply! the call site you linked to is passing the applicable HTTP method(s) to Flask as an argument:

aws-sam-cli/samcli/local/apigw/local_apigw_service.py

Line 161 in 3ee79db

methods=api_gateway_route.methods,

The add_url_rule docs mention that the kwargs are passed to Werkzeug Rule, which documents a methods argument for specifying which HTTP methods should be handled by the rule. My reading of this is that Flask supports different route handlers for different HTTP methods, which is true of every HTTP server framework I've used. Independent of SAM, it would be very limiting if every REST API implemented in Flask had to use the same function to handle all HTTP methods for a given path.

Locally, I replaced this line with routes = self.routes, and sam local start-api properly routes to the correct Lambda depending on the HTTP method (with the same URL path for each):

aws-sam-cli/samcli/lib/providers/api_collector.py

Line 104 in 3ee79db

routes = self.dedupe_function_routes(self.routes)

Consequently, it seems safe to remove the dedupe functionality, which may have been based on a misunderstanding of Flask's behavior, or some limitation that used to exist but no longer does.

[ramosbugs]

or maybe the issue is this:

aws-sam-cli/samcli/lib/providers/api_collector.py

Line 141 in 3ee79db

Remove duplicate routes that have the same function_name and method

the line above says that it should dedupe based on the same function_name and method, but what it actually does is dedupe based on function_name and path:

aws-sam-cli/samcli/lib/providers/api_collector.py

Line 153 in 3ee79db

key = "{}-{}".format(route.function_name, route.path)

[ramosbugs]

@jfuss: any thoughts on my comments above?

iiuc, the current SAM CLI isn't capable of testing most REST APIs, which frequently use multiple HTTP methods (GET/POST/DELETE, etc.) for a single route to represent different operations (i.e., create the resource, fetch the resource, delete the resource). Flask definitely supports this, but not the way that the SAM CLI is currently leveraging Flask...

I'm happy to help contribute a fix if we can align on the approach :-)

[ramosbugs]

See https://github.com/ramosbugs/aws-sam-rest-api-repro for an example repro of the route dedupe bug described above (on the latest version of this PR)

[jfuss]

@ramosbugs Thanks for providing the repro app. It is super helpful. I need to do some deeper testing on what is going on here, as API Gateway doesn't document this (from what I can tell).

What I am going to try and answer:

1. Is operationid only supported in the payload version 1? Looking at https://github.com/aws/aws-lambda-go/blob/5bc50ab1c1dc8f32fce7f7745efcf601f8054fbe/events/apigw.go#L35 (which you linked in the issue) is only for v1. The v2 format doesn't seem to have operationid, at least in the aws-lambda-go library.
2. operationid is part of the OpenApi Path object: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/2.0.md#operationObject. I understand how this is supported in the "get", "post", etc verbs but not sure on the x- extension. This is important for how this works with x-amazon-apigateway-any-method.

From your example, it is clear how this will work in the non swagger model but we need to make sure we support both. I would like to get integrations tests added for various combinations, without that we will risk missing or breaking this. I am happy to add them myself but need to dig deeper and understand this all, especially because it is not documented behavior of API Gateway.

[ramosbugs]

thank you @jfuss! great points.

once implemented, I think this should simplify the internal routing logic that Lambda functions need to implement: based on unique operation ID rather than (HTTP method, route) pair, which partially duplicates the routing logic API Gateway does for us.

[jfuss]

@ramosbugs I am not sure I get your point on: "I think this should simplify the internal routing logic that Lambda functions need to implement: based on unique operation ID rather than (HTTP method, route) pair, which partially duplicates the routing logic API Gateway does for us."

My understanding of API Gateway is that the routing done is by HTTP method and route. Operation ID is optional, so I don't quite understand how this improves any routing logic in SAM CLI.

[jfuss]

The OperationName is only sent to the Lambda Function from API Gateway V1 (Rest API). Rest API's only support payload version 1.0. However for Http Apis (v2), API Gateway never sends the OperationName (payload version 1 or 2).

With this change, we need to reflect the same behavior as API Gateway. @ramosbugs Are you willing to make changes to this PR or should I take it over completely?

[ramosbugs]

@ramosbugs I am not sure I get your point on: "I think this should simplify the internal routing logic that Lambda functions need to implement: based on unique operation ID rather than (HTTP method, route) pair, which partially duplicates the routing logic API Gateway does for us."

My understanding of API Gateway is that the routing done is by HTTP method and route. Operation ID is optional, so I don't quite understand how this improves any routing logic in SAM CLI.

it's a somewhat minor point, but if you have a single Lambda function that's serving multiple API endpoints (particularly useful for compiled languages to avoid emitting dozens of binaries), that Lambda function has to know which endpoint handler to invoke internally. if the operation name/ID from the OpenAPI spec is present in the request, that provides a convenient input for making that routing decision. it also means that only the API Gateway even needs to be aware of the URL paths (e.g., /user/{user_id}) for each endpoint (to translate it to an operation ID), and then the Lambda only needs to reason in terms of operation IDs (e.g., getUser).

The OperationName is only sent to the Lambda Function from API Gateway V1 (Rest API). Rest API's only support payload version 1.0. However for Http Apis (v2), API Gateway never sends the OperationName (payload version 1 or 2).

With this change, we need to reflect the same behavior as API Gateway. @ramosbugs Are you willing to make changes to this PR or should I take it over completely?

it's been awhile since I've touched this code and you're much more familiar with this codebase than I am, so please feel free to take it over :-)

[wchengru]

@ramosbugs I have raised a PR against your PR branch to add the integration tests, and only let operationname send to APIGW v1. Can you please merge it and get it into this PR? ramosbugs#1

Thanks!

[ramosbugs]

@ramosbugs I have raised a PR against your PR branch to add the integration tests, and only let operationname send to APIGW v1. Can you please merge it and get it into this PR? ramosbugs#1

merged, thank you!

[jfuss]

Is this used for all API Events? If so, shouldn't we model different RequestsContext's for different events?

[wchengru]

Looks like we have different RequestContext for V1 and V2 events. This is for v1 events only, it only used in

aws-sam-cli/samcli/local/apigw/local_apigw_service.py

Line 682 in 59f2d19

context = RequestContext(

For v2 api, it is using RequestContextV2:

aws-sam-cli/samcli/local/apigw/local_apigw_service.py

Line 752 in 59f2d19

context = RequestContextV2(http=context_http, route_key=route_key, stage=stage_name)

The httpapi with payload v1 is also using this type of event. For httpapi or restapi without "operationName", it won't be passed into the RequestContext map: https://github.com/aws/aws-sam-cli/pull/2458/files#diff-0ffb9968392fca338c633173f593848b7301fb166fcf56bd3178994d15e1c88aR150

[jfuss]

LGTM

[ramosbugs]

thank you all!