Skip to content

OAuth2AccessTokenResponseBodyExtractor supports Object values #6096

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

Merged
merged 1 commit into from
Nov 15, 2018

Conversation

raphaelDL
Copy link
Contributor

From #6087

@rwinch help please, I took this one but I wonder if this is the right approach for this. What do you think?

@rwinch
Copy link
Member

rwinch commented Nov 15, 2018

@raphaelDL Thank you so much for the PR! The code is exactly what I had imagined. I have a few minor suggestions that help ensure project consistency.

  1. Can you please clean up the commit message to follow Spring's commit message format? In particular, I'd like to see the subject line as concise as possible. I realize this can be difficult to keep under 50, but we really like to keep it concise. The second aspect is we like the commit to end in a line that states "Fixes: OAuth2AccessTokenResponseBodyExtractor should support Object values #6087" This means that when the Pull Request is merged, it automatically closes the ticket and links the commit to the ticket for tracking purposes. An example of what the cleaned up commit might look like is:
OAuth2AccessTokenResponseBodyExtractor supports Object values

This commit ensures the token response is parsed correctly if the values are not a String.

Fixes: gh-6087
  1. In the test, can you please add a comment that refers to the github issue that it is testing for? This helps us for tractability later on. For example, if we run into that test breaking when we do some refactoring later, we can easily see the issue that it is testing for and get more context on why it was necessary. The comment would look like:
@Test
// gh-6087
public void oauth2AccessTokenResponseWhenMultipleAttributeTypesThenCreated() throws Exception {

You can make these changes locally, amend the commit, and force push to your branch and this PR will be updated. As always, if you have any questions, feedback, or need any help, please don't hesitate to reach out.

Thanks again for your help!

@rwinch rwinch added this to the 5.2.0.M1 milestone Nov 15, 2018
@raphaelDL
Copy link
Contributor Author

Working on it :)

@raphaelDL raphaelDL force-pushed the gh-6087 branch 4 times, most recently from 93fd7dd to 2bf5d70 Compare November 15, 2018 18:13
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR and the fast turnaround on the changes. I had one more comment inline and then we are ready to merge. Thanks again for contributing!



@Test
// Fixes: gh-6087
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the convention is that the commit message says "Fixes: ", the comment should just be gh-6087. Can you please change this and then I think we are ready to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup

This commit ensures the token response is parsed correctly if the values are not a String.

Fixes: spring-projectsgh-6087
@rwinch rwinch changed the title OAuth2AccessTokenResponseBodyExtractor now handles additional attribu… OAuth2AccessTokenResponseBodyExtractor supports Object values Nov 15, 2018
@rwinch rwinch merged commit 75a2c2b into spring-projects:master Nov 15, 2018
@rwinch
Copy link
Member

rwinch commented Nov 15, 2018

Thanks for the PR @raphaelDL! This is now merged into master. I also backported your commit into 5.1.x via #6100

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 this pull request may close these issues.

2 participants