Skip to content

fix ResponseMetaDataTransformer #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 27, 2025
Merged

fix ResponseMetaDataTransformer #13

merged 1 commit into from
Jun 27, 2025

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Jun 19, 2025

While working with API Gateway and trying to snapshot responses coming from AWS_PROXY Integration Subtypes, AWS will return a response containing the ResponseMetadata field but already filtered, with no HTTPHeaders field in it.

The current ResponseMetaDataTransformer will always try access the HTTPHeaders field as a dict, even if the value from http_headers = metadata.get("HTTPHeaders") is None, raising this exception:

                for h in headers_to_collect:
>                   if http_headers.get(h):
E                   AttributeError: 'NoneType' object has no attribute 'get'
../localstack_snapshot/snapshots/transformer.py:110: AttributeError

As a workaround, I've implemented a hack like that:
service_resp["_ResponseMetadata"] = service_resp.pop("ResponseMetadata")

This PR now properly fixes it, and adds some test for the transformer.

I'd like to note that it seems we were trying to get the Content-Type header from the response, but as the value was wrong, it was never captured in any snapshots.
Adding it now would need a full refresh, or a big skip, so this is out of scope of this PR.

For the release version, I'd say this could be a 0.3.1?

@bentsku bentsku self-assigned this Jun 19, 2025
@bentsku bentsku added the bug Something isn't working label Jun 19, 2025
Copy link
Contributor

@tiurin tiurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good improvement in library's robustness, thanks @bentsku!

@@ -102,8 +102,14 @@ def transform(self, input_data: dict, *, ctx: TransformContext) -> dict:
if k == "ResponseMetadata":
metadata = v
http_headers = metadata.get("HTTPHeaders")
if not isinstance(http_headers, dict):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good handling of non-expected values here 👏

# TODO "x-amz-bucket-region"
# TestS3.test_region_header_exists -> verifies bucket-region

# FIXME: proper value is `content-type` with no underscore in lowercase, but this will necessitate a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big problem actually. It's not the first time that we defer fixing issues in snapshot library because of backwards compatibility. Thanks for highlighting it! I added the issue about this to snapshots library backlog.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does yes. Maybe versioning could actually help here? Store the snapshot library version along the snapshot, to enjoy new features / support backward compatibility. Thanks for adding the issue on the backlog.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, versioning is the option I added to the backlog issue. It is not the straightforward one when projected onto test runs as we will need to run tests with different dependency versions, but definitely something to try.

@bentsku bentsku merged commit b302a67 into main Jun 27, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants