fix: add AuthOtpResponse #419
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Enhancement
PR Summary: This pull request introduces a new response type, AuthOtpResponse, specifically designed for handling responses from one-time password (OTP) sign-ins. It modifies the sign_in_with_otp method in the gotrue_client.py file to return this new type instead of the generic AuthResponse. Additionally, it simplifies the parse_auth_response function in helpers.py and adds a new function parse_auth_otp_response to parse the OTP-specific responses. The changes also include the necessary adjustments in imports and type declarations to accommodate the new AuthOtpResponse type.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.
General suggestions:
- Ensure that the
model_validatefunction used in the simplifiedparse_auth_responseand the newparse_auth_otp_responseis capable of handling all necessary validations, especially for the fieldsaccess_token,refresh_token, andexpires_in. This is crucial to prevent potential bugs due to missing or invalid data. - Consider adding detailed documentation for the
message_idfield in theAuthOtpResponseclass. This should include its purpose, any constraints on its value, and how it should be used by consumers of the API. Clear documentation will aid in maintaining clarity and ease of use for developers. - Verify that the new
parse_auth_otp_responsefunction and the change in the transformation function toparse_auth_otp_responseingotrue_client.pycorrectly handle all possible variations of the OTP response. This is important to ensure robustness and prevent runtime errors.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
gotrue/helpers.py
Outdated
| user_data = data.get("user", data) | ||
| user = model_validate(User, user_data) if user_data else None | ||
| return AuthResponse(session=session, user=user) | ||
| return model_validate(AuthResponse, data) |
There was a problem hiding this comment.
issue (llm): The refactoring to directly return model_validate(AuthResponse, data) simplifies the code significantly. However, ensure that model_validate is capable of handling all the previous checks and conditions, especially the presence and validity of access_token, refresh_token, and expires_in. If model_validate doesn't perform these checks, this change might introduce a bug risk by assuming all necessary fields are present and valid.
| session: Union[Session, None] = None | ||
|
|
||
|
|
||
| class AuthOtpResponse(BaseModel): |
There was a problem hiding this comment.
suggestion (llm): Introducing AuthOtpResponse as a new response type is a good design decision for handling OTP-specific responses. However, it's important to ensure that the message_id field is adequately documented, including its purpose and any relevant constraints or expected values. This will help maintain clarity and ease of use for developers interacting with this part of the API.
| }, | ||
| redirect_to=email_redirect_to, | ||
| xform=parse_auth_response, | ||
| xform=parse_auth_otp_response, |
There was a problem hiding this comment.
suggestion (llm): Switching the transformation function to parse_auth_otp_response for OTP sign-ins is a logical step following the introduction of AuthOtpResponse. This change ensures that the response parsing is aligned with the expected response structure. It's a good practice to verify that the transformation function correctly handles all possible response variations to avoid runtime errors.
abyesilyurt
left a comment
abyesilyurt
left a comment
There was a problem hiding this comment.
Moved changes to _async and run make build_sync.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #419 +/- ##
==========================================
- Coverage 45.33% 44.71% -0.63%
==========================================
Files 23 23
Lines 1963 2044 +81
==========================================
+ Hits 890 914 +24
- Misses 1073 1130 +57 ☔ View full report in Codecov by Sentry. |
2 participants
What kind of change does this PR introduce?
This PR adds a new type
AuthOtpResponsereturned whensign_in_with_otpis called. The new type has an optional message_id. Closes #406.What is the current behavior?
#406
What is the new behavior?
AuthOtpResponse correctly parses an optional
message_idfield in the OTP response.