-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fixed URL decoding bug. #862
Conversation
Keys were being decoded twice when syncing from s3 to s3.
@@ -335,7 +335,8 @@ def __init__(self, session, event_name, handler): | |||
self._handler = handler | |||
|
|||
def __enter__(self): | |||
self._session.register(self._event_name, self._handler) | |||
self._session.register(self._event_name, self._handler, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ScopedEventHandler
class is a generic class. I don't think it should have the hard coded BucketListerDecodeKeys
. It would make more sense if the unique_id could be passed in through the __init__
.
I updated the pull request to ensure |
LGTM 👍 |
|
||
def __enter__(self): | ||
self._session.register(self._event_name, self._handler) | ||
self._session.register(self._event_name, self._handler, self._unique_id) | ||
|
||
def __exit__(self, exc_type, exc_value, traceback): | ||
self._session.unregister(self._event_name, self._handler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last thing, I believe you need to also pass through the unique_id when you're unregistering it so it can clear that unique id cache that checks if a handler with that name has already been registered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I was just going off the documentation in session.py
. It said that it only needed the handler or the unique_id to unregister. Did not realize the unique_id remains in a cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need to update those docs. Rereading the code again, it definitely looks like you have to provide the unique_id. https://github.com/boto/botocore/blob/develop/botocore/hooks.py#L170
Looks good, just some small feedback. |
Just fixed including the unique_id in |
Looks good! |
Keys were being decoded twice when syncing from s3 to s3.