Skip to content
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

Include params set in provide-client-param event handlers in dynamic context params for endpoint resolution #2920

Merged
merged 7 commits into from
May 11, 2023
9 changes: 5 additions & 4 deletions botocore/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -819,7 +819,6 @@ def _resolve_signature_version(self, service_name, resolved):


class BaseClient:

dlm6693 marked this conversation as resolved.
Show resolved Hide resolved
# This is actually reassigned with the py->op_name mapping
# when the client creator creates the subclass. This value is used
# because calls such as client.get_paginator('list_objects') use the
Expand Down Expand Up @@ -913,6 +912,11 @@ def _make_api_call(self, operation_name, api_params):
'has_streaming_input': operation_model.has_streaming_input,
'auth_type': operation_model.auth_type,
}
api_params = self._emit_api_params(
api_params=api_params,
operation_model=operation_model,
context=request_context,
)
Comment on lines +915 to +919
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this could be called from within _resolve_endpoint_ruleset at the very start of it? I'm thinking in purely organizational terms just because _make_api_call is a rather long, complex function already.

endpoint_url, additional_headers = self._resolve_endpoint_ruleset(
operation_model, api_params, request_context
)
Expand Down Expand Up @@ -984,9 +988,6 @@ def _convert_to_request_dict(
headers=None,
set_user_agent_header=True,
):
api_params = self._emit_api_params(
api_params, operation_model, context
)
request_dict = self._serializer.serialize_to_request(
api_params, operation_model
)
Expand Down
12 changes: 10 additions & 2 deletions botocore/signers.py
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,11 @@ def generate_presigned_url(
context,
ignore_signing_region=(not bucket_is_arn),
)

dlm6693 marked this conversation as resolved.
Show resolved Hide resolved
params = self._emit_api_params(
Copy link
Contributor

Choose a reason for hiding this comment

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

The only concern I have with this PR is we're moving this into multiple places now making any future changes harder to keep consistent.

It would be nice to keep these consolidated with _convert_to_request_dict, but I don't see an immediate solution for that.

Copy link
Contributor

@dlm6693 dlm6693 May 5, 2023

Choose a reason for hiding this comment

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

I think using my suggestion of putting this in _resolve_endpoint_ruleset would accomplish that no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or does this still need to happen immediately after endpoint resolution for presigned URLs?

api_params=params,
operation_model=operation_model,
context=context,
)
request_dict = self._convert_to_request_dict(
api_params=params,
operation_model=operation_model,
Expand Down Expand Up @@ -786,7 +790,11 @@ def generate_presigned_post(
context,
ignore_signing_region=(not bucket_is_arn),
)

params = self._emit_api_params(
api_params=params,
operation_model=operation_model,
context=context,
)
request_dict = self._convert_to_request_dict(
api_params=params,
operation_model=operation_model,
Expand Down
41 changes: 40 additions & 1 deletion tests/functional/test_context_params.py
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ def test_client_context_param_sent_to_endpoint_resolver(
),
)

# Stub client to prevent a request from getting sent and asceertain that
# Stub client to prevent a request from getting sent and ascertain that
# only a single request would get sent. Wrap the EndpointProvider's
# resolve_endpoint method for inspecting the arguments it gets called with.
with ClientHTTPStubber(client, strict=True) as http_stubber:
Expand Down Expand Up @@ -488,3 +488,42 @@ def test_dynamic_context_param_sent_to_endpoint_resolver(
)
else:
mock_resolve_endpoint.assert_called_once_with(Region='us-east-1')


def test_dynamic_context_param_from_event_handler_sent_to_endpoint_resolver(
monkeypatch,
patched_session,
):
# patch loader to return fake service model and fake endpoint ruleset
patch_load_service_model(
patched_session,
monkeypatch,
FAKE_MODEL_WITH_DYNAMIC_CONTEXT_PARAM,
FAKE_RULESET_WITH_DYNAMIC_CONTEXT_PARAM,
)

# event handler for provide-client-params that modifies the value of the
# MockOpParam parameter
def change_param(params, **kwargs):
params['MockOpParam'] = 'mock-op-param-value-2'

client = patched_session.create_client(
'otherservice', region_name='us-east-1'
)
client.meta.events.register_last(
'provide-client-params.other-service.*', change_param
)

with ClientHTTPStubber(client, strict=True) as http_stubber:
http_stubber.add_response(status=200)
with mock.patch.object(
client._ruleset_resolver._provider,
'resolve_endpoint',
wraps=client._ruleset_resolver._provider.resolve_endpoint,
) as mock_resolve_endpoint:
client.mock_operation(MockOpParam='mock-op-param-value-1')

mock_resolve_endpoint.assert_called_once_with(
Region='us-east-1',
FooDynamicContextParamName='mock-op-param-value-2',
)