Skip to content
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

feat: add flag to allow UrlEncodedContent to use UriPath escaping #1100

Merged

Conversation

guillaumeblaquiere
Copy link
Contributor

@guillaumeblaquiere guillaumeblaquiere commented Aug 28, 2020

Fixes #1098
The legacy behavior is kept. Test has been updated

@guillaumeblaquiere guillaumeblaquiere requested a review from as a code owner Aug 28, 2020
@google-cla google-cla bot added the cla: yes label Aug 28, 2020
guillaumeblaquiere added a commit to guillaumeblaquiere/google-auth-library-java that referenced this issue Aug 28, 2020
idTokenWithAudience method not tested (I don't know how to do this to be useful)

Depend on this update googleapis/google-http-java-client#1100
Copy link
Collaborator

@elharo elharo left a comment

I wish we could do this unconditionally but that's risky.

}

/**
* @param data key name/value data
Copy link
Collaborator

@elharo elharo Aug 29, 2020

Choose a reason for hiding this comment

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

the key is a name and a value? That's surprising.

Copy link
Contributor Author

@guillaumeblaquiere guillaumeblaquiere Aug 29, 2020

Choose a reason for hiding this comment

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

Copy paste of the legacy comment (see line 56 of the file.) I kept the legacy signature and behavior, and I added mine with additional parameter. This one is unchanged (and I guess its description also!)

Copy link
Contributor Author

@guillaumeblaquiere guillaumeblaquiere Aug 29, 2020

Choose a reason for hiding this comment

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

Let me know if you want me to improve both or if keeping the existing description is ok.

Copy link
Collaborator

@elharo elharo Sep 1, 2020

Choose a reason for hiding this comment

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

The more I look at this the weirder it is. Why is this an object instead of a Map?

Copy link
Contributor Author

@guillaumeblaquiere guillaumeblaquiere Sep 1, 2020

Choose a reason for hiding this comment

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

I agree; it's strange and I don't know why to use an Object instead of a Map! However to ensure the compatibility with other app that depends on this library, I kept the existing signature.

Improve variable naming

Improve documentation more user oriented.
@elharo elharo changed the title Feature #1098: Improve UrlEncodedContent class feat: Improve UrlEncodedContent class Oct 7, 2020
@elharo elharo added the kokoro:run label Oct 7, 2020
@kokoro-team kokoro-team removed the kokoro:run label Oct 7, 2020
public UrlEncodedContent(Object data) {
super(UrlEncodedParser.MEDIA_TYPE);
setData(data);
this.uriPathEncodingFlag = false;
}

/**
* Initialize the UrlEncodedContent with our without the legacy and deprecated escapeUri encoder
Copy link
Collaborator

@elharo elharo Oct 12, 2020

Choose a reason for hiding this comment

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

our --> or?

* @param useUriPathEncoding Escapes the string value so it can be safely included in URI path segments. For details on
* * escaping URIs, see <a href="http://tools.ietf.org/html/rfc3986#section-2.4">RFC 3986 - section
* * 2.4</a>
* @param useUriPathEncoding Escapes the string value so it can be safely included in URI path segments.
Copy link
Collaborator

@elharo elharo Oct 12, 2020

Choose a reason for hiding this comment

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

Escapes --> escapes

* * 2.4</a>
* @param useUriPathEncoding Escapes the string value so it can be safely included in URI path segments.
* For details on escaping URIs, see <a href="http://tools.ietf.org/html/rfc3986#section-2.4">RFC 3986 -
* section 2.4</a>
*/
public UrlEncodedContent(Object data, Boolean useUriPathEncoding) {
Copy link
Collaborator

@elharo elharo Oct 12, 2020

Choose a reason for hiding this comment

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

Boolean --> boolean
per Effective Java

elharo
elharo approved these changes Oct 12, 2020
@chingor13 chingor13 added the kokoro:force-run label Oct 13, 2020
@kokoro-team kokoro-team removed the kokoro:force-run label Oct 13, 2020
@chingor13 chingor13 changed the title feat: Improve UrlEncodedContent class feat: allow UrlEncodedContent to use UriPath escaping Oct 13, 2020
@chingor13 chingor13 changed the title feat: allow UrlEncodedContent to use UriPath escaping feat: add flag to allow UrlEncodedContent to use UriPath escaping Oct 13, 2020
@chingor13 chingor13 added the kokoro:force-run label Oct 13, 2020
@kokoro-team kokoro-team removed the kokoro:force-run label Oct 13, 2020
@chingor13 chingor13 added the automerge label Oct 13, 2020
@gcf-merge-on-green gcf-merge-on-green bot merged commit 9ab7016 into googleapis:master Oct 13, 2020
10 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge label Oct 13, 2020
gcf-merge-on-green bot pushed a commit that referenced this issue Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants