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: fix post policy escape bug, update conformance tests #924

Merged
merged 2 commits into from Aug 20, 2021

Conversation

JesseLovelace
Copy link
Contributor

@JesseLovelace JesseLovelace commented Jul 20, 2021

V4 Post Policy had an issue where quotes in the json policy would get double escaped (" would became \\" instead of \"), this fixes that issue and updates the conformance tests to the latest version

@JesseLovelace JesseLovelace requested review from BenWhitehead and a team July 20, 2021 18:05
@JesseLovelace JesseLovelace requested a review from a team as a code owner July 20, 2021 18:05
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/java-storage API. label Jul 20, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 20, 2021
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Is there a bug reference for this?

if (c >= 128) { // is a unicode character
escapedJson.append(String.format("\\u%04x", (int) c));
} else {
switch (c) {
case '\\':
escapedJson.append("\\\\");
// The JsonObject/JsonArray operations above handle quote escapes, so leave any "/""
Copy link
Contributor

Choose a reason for hiding this comment

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

is forward slash meant here? Looks like it should be a backslash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be, will fix

StringBuilder escapedPolicy = new StringBuilder();

// Java automatically unescapes the unicode escapes in the conformance tests, so we need to
// manually re-escape them
for (char c : expectedPolicy.toCharArray()) {
char[] expectedPolicyArray = expectedPolicy.toCharArray();
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be reproducing the algorithm in the test. That risks copying bugs from one place to another, and verifying the buggy behavior. Can you make the expected output a literal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, they both happen to need to do the same thing (escaping certain characters). Sorry, I'm unsure what you mean by "make it a literal," can you elaborate?

@@ -93,7 +93,7 @@
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>google-cloud-conformance-tests</artifactId>
<version>0.0.11</version>
<version>0.1.1</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this verify the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, in fact the failures caused by it is what brought the bug to my attention (#510)

Copy link
Collaborator

@BenWhitehead BenWhitehead left a comment

Choose a reason for hiding this comment

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

Feel free to merge after fixing the typo

@BenWhitehead BenWhitehead added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 19, 2021
@JesseLovelace JesseLovelace removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 20, 2021
@JesseLovelace JesseLovelace merged commit d8329c3 into master Aug 20, 2021
@JesseLovelace JesseLovelace deleted the fixpostpolicy branch August 20, 2021 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/java-storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants