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

botocore 1.28.0 expects endpoint_url param to _convert_to_request_dict() #1081

Closed
tom-a-c opened this issue Oct 25, 2022 · 10 comments · Fixed by #1083
Closed

botocore 1.28.0 expects endpoint_url param to _convert_to_request_dict() #1081

tom-a-c opened this issue Oct 25, 2022 · 10 comments · Fixed by #1083

Comments

@tom-a-c
Copy link

tom-a-c commented Oct 25, 2022

The code here (L358-L361) calls an internal botocore method (L961-L969):

request_dict = self.client._convert_to_request_dict(
    operation_kwargs,
    operation_model,
)

botocore release 1.28.0 expects a new endpoint_url parameter in this method call which pynamodb does not provide. We recieved the following stacktrace:

[ERROR] TypeError: _convert_to_request_dict() missing 1 required positional argument: 'endpoint_url'
Traceback (most recent call last):
  ...
  File "/var/task/pynamodb/pagination.py", line 194, in __next__
    self._get_next_page()
  File "/var/task/pynamodb/pagination.py", line 179, in _get_next_page
    page = next(self.page_iter)
  File "/var/task/pynamodb/pagination.py", line 115, in __next__
    page = self._operation(*self._args, settings=self._settings, **self._kwargs)
  File "/var/task/pynamodb/connection/table.py", line 270, in query
    return self.connection.query(
  File "/var/task/pynamodb/connection/base.py", line 1288, in query
    tbl = self.get_meta_table(table_name)
  File "/var/task/pynamodb/connection/base.py", line 548, in get_meta_table
    data = self.dispatch(DESCRIBE_TABLE, operation_kwargs)
  File "/var/task/pynamodb/connection/base.py", line 329, in dispatch
    data = self._make_api_call(operation_name, operation_kwargs, settings)
  File "/var/task/pynamodb/connection/base.py", line 358, in _make_api_call
    request_dict = self.client._convert_to_request_dict(
@DeepSpace2
Copy link

DeepSpace2 commented Oct 25, 2022

:( This issue could have been avoided by:

  • botocore not making a breaking change in a minor version, and
  • pynamodb not accessing botocore client's "private" member (_convert_to_request_dict)

We need a patched version of dynamodb.

@tom-a-c
Copy link
Author

tom-a-c commented Oct 25, 2022

I respectfully disagree that this is a breaking change for botocore. As you say, the contract when using libraries should be with the public methods, if pynamodb used the library in the intended way, this wouldn't have been a breaking change.

It seems to still be a continuing theme of calling private methods of botocore: E.g. draft PR here #1079

Some lessons learned here for sure, and an understanding that depending on botocore in this way is having major downstream effects on people using pynamodb who rely upon the stability of open source libraries for critical use cases

@igoreisenberg
Copy link

This issue could have been avoided by not having open-ended dependencies (e.g. botocore>=1.12.54)

@DeepSpace2
Copy link

DeepSpace2 commented Oct 25, 2022

@igoreisenberg

Pinning a botocore version is a bad idea as it is a strongly coupled dependency of boto3.

This is a theoretical discussion. The actual and only valid solution is do not access an attribute/function that is prefixed by a _.

@igoreisenberg
Copy link

@DeepSpace2
While I agree that the actual solution is to not access a protected function, but if it is already being accessed, it should do it in a way that would avoid being broken when the dependent library would get updated.

(I've never proposed pinning botocore, I've proposed to not have it open-ended, which caused this issue)

@ikonst
Copy link
Contributor

ikonst commented Oct 25, 2022

Limiting botocore up to a specific minor version would be overly restrictive to our ecosystem. Ultimately it's our "fault" in pynamodb for relying on a private method, so it's our responsibility to update for the upstream change.

P.S. thanks everyone for quickly analyzing this!

@ikonst ikonst linked a pull request Oct 25, 2022 that will close this issue
@DeepSpace2
Copy link

@ikonst @garrettheel

Thanks for the quick fix.
Any chance to get this as a 5.2.2 hotfix rather than waiting to 6.0.0?

@ikonst
Copy link
Contributor

ikonst commented Oct 25, 2022

Any chance to get this as a 5.2.2 hotfix rather than waiting to 6.0.0?

Absolutely.

@ikonst
Copy link
Contributor

ikonst commented Oct 25, 2022

This should be fixed in 5.2.3.

@tom-a-c tom-a-c changed the title boto3 1.25.0 expects endpoint_url param to _convert_to_request_dict() botocore 1.28.0 expects endpoint_url param to _convert_to_request_dict() Oct 26, 2022
@sravanAruru
Copy link

Any idea on this error

latestenv/lib/python3.8/site-packages/pynamodb/connection/base.py:1311: in query
tbl = self.get_meta_table(table_name)
latestenv/lib/python3.8/site-packages/pynamodb/connection/base.py:571: in get_meta_table
data = self.dispatch(DESCRIBE_TABLE, operation_kwargs)
latestenv/lib/python3.8/site-packages/pynamodb/connection/base.py:332: in dispatch
data = self._make_api_call(operation_name, operation_kwargs, settings)
latestenv/lib/python3.8/site-packages/pynamodb/connection/base.py:368: in _make_api_call
endpoint_url, additional_headers = self.client._resolve_endpoint_ruleset(
E ValueError: too many values to unpack (expected 2)
versions am using Python 3.8.16, pytest-5.2.0, py-1.11.0, pluggy-0.13.1 ,pynamodb-4.4.0,botocore-1.34.45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants