Skip to content

[ggj]fix: handle special characters in string object value#72

Merged
xiaozhenliu-gg5 merged 55 commits intomasterfrom
fix-string-object
Jul 16, 2020
Merged

[ggj]fix: handle special characters in string object value#72
xiaozhenliu-gg5 merged 55 commits intomasterfrom
fix-string-object

Conversation

@xiaozhenliu-gg5
Copy link
Copy Markdown
Contributor

@xiaozhenliu-gg5 xiaozhenliu-gg5 commented Jun 24, 2020 •
edited
Loading

resolve left comments in #61

  1. write manually the escaper to help handle special characters.
  2. add unit tests to cover special/unicode strings.

more details can be found in the design doc, investigation is appended.

summer-ji-eng
summer-ji-eng reviewed Jun 24, 2020
View reviewed changes
Comment thread src/main/java/com/google/api/generator/engine/ast/StringObjectValue.java Outdated
miraleung
miraleung suggested changes Jun 24, 2020
View reviewed changes
Comment thread src/main/java/com/google/api/generator/engine/ast/StringObjectValue.java Outdated
Comment thread src/main/java/com/google/api/generator/engine/ast/StringObjectValue.java Outdated
xiaozhenliu-gg5
xiaozhenliu-gg5 commented Jun 24, 2020
View reviewed changes
Comment thread src/test/java/com/google/api/generator/engine/ast/StringObjectValueTest.java Outdated
miraleung
miraleung suggested changes Jun 24, 2020
View reviewed changes
Comment thread src/main/java/com/google/api/generator/engine/ast/StringObjectValue.java Outdated
Comment thread src/main/java/com/google/api/generator/engine/ast/StringObjectValue.java Outdated
miraleung
miraleung suggested changes Jun 26, 2020
View reviewed changes
Comment thread src/main/java/com/google/api/generator/engine/ast/StringObjectValue.java Outdated
miraleung
miraleung reviewed Jun 26, 2020
View reviewed changes
Comment thread WORKSPACE Outdated
miraleung
miraleung suggested changes Jul 9, 2020
View reviewed changes
Comment thread src/main/java/com/google/api/generator/engine/ast/StringObjectValue.java Outdated
Comment thread src/main/java/com/google/api/generator/engine/escaper/StringValueEscaper.java Outdated
Comment thread src/main/java/com/google/api/generator/engine/escaper/StringValueEscaper.java Outdated
Comment thread src/test/java/com/google/api/generator/engine/ast/StringObjectValueTest.java
@xiaozhenliu-gg5 xiaozhenliu-gg5 changed the title [ggj]fix: string object value unit test [ggj]fix: handle special characters in string object value Jul 10, 2020
@xiaozhenliu-gg5
Copy link
Copy Markdown
Contributor Author

xiaozhenliu-gg5 commented Jul 10, 2020

StringValueEscaper is moved to StringObjectValue class as a nested class, EscaperException is added in escaper package, which will be shared by the commentEscaperthat would be implemented.

miraleung
miraleung suggested changes Jul 13, 2020
View reviewed changes
Comment thread src/main/java/com/google/api/generator/engine/ast/StringObjectValue.java Outdated
public StringObjectValue build() {
// `\"` is added to the escaped string value for interpreting it correctly in file.
String value =
String.format("\"%s\"", StringValueEscaper.getInstance().escape(autoBuild().value()));
Copy link
Copy Markdown
Contributor

@miraleung miraleung Jul 13, 2020

Choose a reason for hiding this comment

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

Instead of calling autoBuild twice, what about just using value()? IIRC we discussed this in a prior review.

Copy link
Copy Markdown
Contributor Author

@xiaozhenliu-gg5 xiaozhenliu-gg5 Jul 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

right, we had this discussion, here we cannot use value() because it's not static, Builder class has static context.

Copy link
Copy Markdown
Contributor

@miraleung miraleung Jul 14, 2020

Choose a reason for hiding this comment

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

One way to avoid calling autoBuild() twice is to add a "private" value() method to the Builder class. Please see the other examples in this codebase.

Copy link
Copy Markdown
Contributor Author

@xiaozhenliu-gg5 xiaozhenliu-gg5 Jul 15, 2020

Choose a reason for hiding this comment

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

Right! Thank you for the advice, also found this doc that illustrate how/why it works. Learned from this PR!

Comment thread src/main/java/com/google/api/generator/engine/escaper/BUILD.bazel Outdated
Comment thread src/main/java/com/google/api/generator/engine/ast/StringObjectValue.java Outdated
Comment thread src/main/java/com/google/api/generator/engine/ast/StringObjectValue.java Outdated
Comment thread src/main/java/com/google/api/generator/engine/ast/StringObjectValue.java Outdated
return escaper.escape(sourceString);
}

public static Escaper getInstance() {
Copy link
Copy Markdown
Contributor

@miraleung miraleung Jul 13, 2020

Choose a reason for hiding this comment

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

Consider whether this method is still needed.

Copy link
Copy Markdown
Contributor Author

@xiaozhenliu-gg5 xiaozhenliu-gg5 Jul 13, 2020

Choose a reason for hiding this comment

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

yes! This is basically for wrapping the escape() in as a static method so that we can call it from the static context (class Builder).

Copy link
Copy Markdown
Contributor

@miraleung miraleung Jul 14, 2020

Choose a reason for hiding this comment

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

Could you please try calling escape() without getInstance() and consider whether getInstance() is still needed?

Copy link
Copy Markdown
Contributor Author

@xiaozhenliu-gg5 xiaozhenliu-gg5 Jul 15, 2020

Choose a reason for hiding this comment

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

sorry if I confused you, but I did try remove the getInstance()and directly call StringValueEscaper.escape(value) and it throws error of cannot calling a non-static method from static context. since the override escape() is not static.

      String value = String.format("\"%s\"", StringValueEscaper.escape(value()));

please LMK if I did something wrong.

Copy link
Copy Markdown
Contributor Author

@xiaozhenliu-gg5 xiaozhenliu-gg5 Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

But this works, thank you!

      String value = String.format("\"%s\"", StringValueEscaper.escaper.escape(value()));

Copy link
Copy Markdown
Contributor Author

@xiaozhenliu-gg5 xiaozhenliu-gg5 Jul 15, 2020

Choose a reason for hiding this comment

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

TWIL: if the member or constructor is declared private, then access is permitted if and only if it occurs within the body of the top level class that encloses the declaration of the member or constructor.

miraleung
miraleung suggested changes Jul 14, 2020
View reviewed changes
Comment thread src/main/java/com/google/api/generator/engine/ast/StringObjectValue.java Outdated
public StringObjectValue build() {
// `\"` is added to the escaped string value for interpreting it correctly in file.
String value =
String.format("\"%s\"", StringValueEscaper.getInstance().escape(autoBuild().value()));
Copy link
Copy Markdown
Contributor

@miraleung miraleung Jul 14, 2020

Choose a reason for hiding this comment

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

One way to avoid calling autoBuild() twice is to add a "private" value() method to the Builder class. Please see the other examples in this codebase.

@xiaozhenliu-gg5 xiaozhenliu-gg5 merged commit 5f44411 into master Jul 16, 2020
@miraleung miraleung deleted the fix-string-object branch July 25, 2020 09:57
suztomo pushed a commit that referenced this pull request Mar 21, 2023
@renovate-bot
build(deps): update dependency com.google.cloud:google-cloud-shared-c…
d72591b 
…onfig to v0.8.0 (#72)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [com.google.cloud:google-cloud-shared-config](https://togithub.com/googleapis/java-shared-config) | minor | `0.6.0` -> `0.8.0` |

---

### Release Notes

<details>
<summary>googleapis/java-shared-config</summary>

### [`v0.8.0`](https://togithub.com/googleapis/java-shared-config/blob/master/CHANGELOG.md#&#8203;080-httpswwwgithubcomgoogleapisjava-shared-configcomparev070v080-2020-06-10)

[Compare Source](https://togithub.com/googleapis/java-shared-config/compare/v0.7.0...v0.8.0)

##### Features

-   revert "feat: mark auto-value-annotations scope as provided" ([#&#8203;154](https://www.github.com/googleapis/java-shared-config/issues/154)) ([88afb4e](https://www.github.com/googleapis/java-shared-config/commit/88afb4e7c57cb6e00929c098135314a926d9da30))

### [`v0.7.0`](https://togithub.com/googleapis/java-shared-config/blob/master/CHANGELOG.md#&#8203;070-httpswwwgithubcomgoogleapisjava-shared-configcomparev060v070-2020-06-10)

[Compare Source](https://togithub.com/googleapis/java-shared-config/compare/v0.6.0...v0.7.0)

##### Features

-   mark auto-value-annotations scope as provided ([#&#8203;151](https://www.github.com/googleapis/java-shared-config/issues/151)) ([44ea4cb](https://www.github.com/googleapis/java-shared-config/commit/44ea4cbbf92b4ad35ffaffb7a01a1bce05063daf))

##### Bug Fixes

-   lock the google-java-format version used by formatter plugin ([#&#8203;149](https://www.github.com/googleapis/java-shared-config/issues/149)) ([d58c054](https://www.github.com/googleapis/java-shared-config/commit/d58c05437a4ea8767db2bebfcc405ec77aeb9705))

</details>

---

### Renovate configuration

:date: **Schedule**: At any time (no schedule defined).

:vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

:recycle: **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

:no_bell: **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#googleapis/java-shared-dependencies).
suztomo pushed a commit that referenced this pull request Mar 21, 2023
@renovate-bot @chingor13
deps: update dependency io.grpc:grpc-bom to v1.25.0 (#72)
88ab7d8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@miraleung miraleung miraleung approved these changes

@summer-ji-eng summer-ji-eng summer-ji-eng approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@xiaozhenliu-gg5 @miraleung @summer-ji-eng