returning errors back to the client. #990
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tching errors in the oauth config
shibd
approved these changes
Mar 13, 2023
shibd
requested changes
Mar 13, 2023
nodece
reviewed
Mar 14, 2023
| oauth, _ := auth.NewAuthenticationOAuth2WithParams(authParams) | ||
| return oauth | ||
| func NewAuthenticationOAuth2(authParams map[string]string) (Authentication, error) { | ||
| oauth, err := auth.NewAuthenticationOAuth2WithParams(authParams) |
There was a problem hiding this comment.
I don't think you should change the return value.
You can add a new method or use panic here.
There was a problem hiding this comment.
Okay. I dont think I will do a panic/fatal or in any other way exit with an error in a function like that. But I guess that could be a matter of personal preference or coding style.
So just to be clear we're talking about the same; your suggestion would be to make another function called something like NewAuthenticationOAuth2withErrorHandling, correct?
There was a problem hiding this comment.
In principal, the modifications that modify the public APIs should not be accepted. /cc @shibd
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
When using the pulsar go client with oauth2, setting up the auth with the function NewAuthenticationOAuth2 doesn't return an error. In case of errors in the authentication flow nothing happens and the user/developer can only assume that the authentication went well.
It turns out there is a pretty decent error handling in the entire flow, but for unknown reasons this error is discarded right before the connection is returned to the client.
Modifications
Instead of just discarding the error, it is now returned. Note that the function now returns two parameters instead of just one and should be handled accordingly.
Refer to the changelog of the file
pulsar/client.goto see the actual changes.Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Documentation