-
Notifications
You must be signed in to change notification settings - Fork 346
Fix: Convert scopes set to list in Credentials.__init__ #1898
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
Fix: Convert scopes set to list in Credentials.__init__ #1898
Conversation
This change ensures that if a set of scopes is passed to Credentials, it is converted to a list. This prevents issues with JSON serialization (to_json failure) and ensures consistent mutability behavior for the scopes property. Fixes #1145
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. New to Jules? Learn more at jules.google/docs. |
This change ensures that if a set of scopes is passed to Credentials, it is converted to a list. This prevents issues with JSON serialization (to_json failure) and ensures consistent mutability behavior for the scopes property. Also updated tests/test__oauth2client.py to handle the type change (set -> list) in equality checks. Fixes #1145
This change ensures that if a set of scopes is passed to Credentials, it is converted to a list. This prevents issues with JSON serialization (to_json failure) and ensures consistent mutability behavior for the scopes property. Also updated tests/test__oauth2client.py to handle the type change (set -> list) in equality checks. Fixes #1145
This change ensures that if a set of scopes is passed to Credentials, it is converted to a list. This prevents issues with JSON serialization (to_json failure) and ensures consistent mutability behavior for the scopes property. Also updated tests/test__oauth2client.py to handle the type change (set -> list) in equality checks. Fixes #1145
This change ensures that if a set of scopes is passed to Credentials, it is converted to a list. This prevents issues with JSON serialization (to_json failure) and ensures consistent mutability behavior for the scopes property. Also updated tests/test__oauth2client.py to handle the type change (set -> list) in equality checks. Fixes #1145
|
Note: the requirement for system-3.10 to pass has been temporarily removed due to a known issue with that CI/CD check that is being addressed in an separate PR #1901 Failure for it to pass should not preclude this PR from being approved and merged. |
Understood. Thank you for the clarification regarding the system-3.10 failures. I have verified that the changes in this PR are correct and have addressed the regression in |
This change ensures that if a set of scopes is passed to Credentials, it is converted to a list. This prevents issues with JSON serialization (to_json failure) and ensures consistent mutability behavior for the scopes property. Also updated tests/test__oauth2client.py to handle the type change (set -> list) in equality checks. Fixes #1145
chalmerlowe
left a comment
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.
LGTM
This PR addresses an issue where passing a
setof scopes toCredentialscausesto_jsonto fail and introduces inconsistent mutability behavior if handled via a property accessor.Changes:
google.oauth2.credentials.Credentials.__init__to convertscopesto alistif it is aset.google.oauth2.credentials.Credentials.scopesproperty docstring toOptional[Sequence[str]].test_init_with_set_scopesintests/oauth2/test_credentials.pyto verify the fix, ensuring type conversion, object stability, and serialization support.tests/test__oauth2client.pyto relax strict equality checks onscopes(comparing assets) to accommodate the new behavior where internal storage is enforced as alist.This PR supercedes PR #1145 which potentially introduced a mutability inconsistency and failed to address a type correctness issue. This PR avoids both of those concerns.
._scopesis aset, thescopesproperty returns a newlistevery time it is accessed.creds.scopes.append("new_scope")), the modification is lost because the next access returns a fresh list from the underlying set._scopeswas originally a list (passed in__init__),creds.scopesreturns the same list reference, so modifications are preserved. This inconsistent behavior between "set-initialized" and "list-initialized" credentials is a potential bug trap.__init__is documented asSequence[str]. A set is not aSequence(it's not indexable/ordered). While Python is lenient, it is better to normalize the input at the boundary (__init__) rather than in the accessor.Moving the conversion logic to
__init__ensures:None).to_jsonworks correctly because it accessesself.scopes(or self._scopes).PR created automatically by Jules for task 15605314498929918698 started by @chalmerlowe