Fix datastore grpc doc class generation for python and ruby #716

swcloud · 2016-11-03T00:15:03Z

#716 This is due to datastore API's GqlQuery message's comment contains '$' so we have to escape it, otherwise it will cause java's regex failure (throwing IAE exception)

In https://github.com/googleapis/googleapis/blob/master/google/datastore/v1/query.proto#L232,

Key must match regex [A-Za-z_$][A-Za-z_$0-9]*

The original CommentPatterns.PROTO_LINK_PATTERN causes the above [A-Za-z_$][A-Za-z_$0-9] to be parsed as a proto link, using Ruby as an example (similar with Python and NodeJS)

In RDocCommentFixer.java:

  private static String rdocifyProtoMarkdownLinks(String comment) {
    StringBuffer sb = new StringBuffer();
    Matcher m = CommentPatterns.PROTO_LINK_PATTERN.matcher(comment);
    if (!m.find()) {
      return comment;
    }
    do {
      m.appendReplacement(sb, String.format("%s", protoToRubyDoc(m.group(1))));
    } while (m.find());
    m.appendTail(sb);
    return sb.toString();
  }

When m.appendReplacement is called, the second argument contains a '$' in it (because m.group(1) is A-Za-z_$, so that causes appendReplacement to throw IAE.

src/main/java/com/google/api/codegen/py/PythonSphinxCommentFixer.java

@@ -22,6 +22,8 @@

  /** Returns a Sphinx-formatted comment string. */
  public static String sphinxify(String comment) {
+    // escape '$' in the comment first
+    comment = comment.replaceAll("\\$", "\\\\\\$");


src/main/java/com/google/api/codegen/py/PythonSphinxCommentFixer.java

@@ -22,6 +22,8 @@

  /** Returns a Sphinx-formatted comment string. */
  public static String sphinxify(String comment) {
+    // escape '$' in the comment first
+    comment = comment.replaceAll("\\$", "\\\\\\$");


src/main/java/com/google/api/codegen/py/PythonSphinxCommentFixer.java

@@ -22,6 +22,8 @@

  /** Returns a Sphinx-formatted comment string. */
  public static String sphinxify(String comment) {
+    // escape '$' in the comment first


src/main/java/com/google/api/codegen/ruby/RDocCommentFixer.java

  public static String rdocify(String comment) {
+    // escape '$' in the comment first


codecov-io · 2016-11-03T01:01:30Z

Current coverage is 81.36% (diff: 100%)

Merging #716 into master will increase coverage by 0.03%

@@             master       #716   diff @@
==========================================
  Files           292        294     +2   
  Lines         11117      11190    +73   
  Methods           0          0          
  Messages          0          0          
  Branches       1465       1480    +15   
==========================================
+ Hits           9042       9105    +63   
- Misses         1673       1680     +7   
- Partials        402        405     +3

Powered by Codecov. Last update e4f977a...5833031

* Use Matcher.quoteReplacement to escape '$' in cloud/absolute link

bjwatson

I think some unit tests would really help clarify what this PROTO_LINK_PATTERN and these changes are actually doing. I find myself focusing on the details of this PR without really understanding the big picture, and some usage examples would help with that.

src/main/java/com/google/api/codegen/CommentPatterns.java

@@ -26,6 +26,7 @@
      Pattern.compile("\\[([^\\]]+)\\]\\((\\p{Alpha}+:[^\\)]+)\\)");
  public static final Pattern CLOUD_LINK_PATTERN =
      Pattern.compile("\\[([^\\]]+)\\]\\(((?!\\p{Alpha}+:)[^\\)]+)\\)");
-  public static final Pattern PROTO_LINK_PATTERN = Pattern.compile("\\[([^\\]]+)\\]\\[[^\\]]*\\]");
+  public static final Pattern PROTO_LINK_PATTERN =
+      Pattern.compile("\\[([^\\]]+)\\]\\[([A-Za-z_][A-Za-z_.0-9]*)*\\]");


jmuk

It is worth adding a new random comment in the test case to see the behaviors and expected results.

src/main/java/com/google/api/codegen/CommentPatterns.java

@@ -26,6 +26,7 @@
      Pattern.compile("\\[([^\\]]+)\\]\\((\\p{Alpha}+:[^\\)]+)\\)");
  public static final Pattern CLOUD_LINK_PATTERN =
      Pattern.compile("\\[([^\\]]+)\\]\\(((?!\\p{Alpha}+:)[^\\)]+)\\)");
-  public static final Pattern PROTO_LINK_PATTERN = Pattern.compile("\\[([^\\]]+)\\]\\[[^\\]]*\\]");
+  public static final Pattern PROTO_LINK_PATTERN =
+      Pattern.compile("\\[([^\\]]+)\\]\\[([A-Za-z_][A-Za-z_.0-9]*)*\\]");


bjwatson · 2016-11-03T19:30:52Z

@jmuk Like fuzzing?

jmuk · 2016-11-03T19:55:11Z

oh, not saying that random :)

I'd like to recommend to copy&paste some comments which causes this problem from the production proto files (or write something by ourselves).

bjwatson · 2016-11-03T20:05:21Z

Got it. SGTM

…ixer

swcloud · 2016-11-04T02:02:45Z

@jmuk @bjwatson @geigerj I added unit test cases to test how Python/Ruby/NodeJS CommentFixer should handle '$' to avoid throwing IAE. These are not comprehensive test cases for the CommentFixers, because I think this PR should focus on related problems. I filed #722 to add more comprehensive tests. I think @geigerj understands best for these markdown link requirements, so I assigned it to him.

swcloud · 2016-11-04T02:09:41Z

@jmuk I updated the description to add more details how this problem is caused.

swcloud · 2016-11-04T22:33:37Z

@jmuk @bjwatson @geigerj PTAL

bjwatson

Almost done. :)

src/test/java/com/google/api/codegen/ProtoDocumentLinkTest.java

+    Truth.assertThat(m.find()).isTrue();
+    m = CommentPatterns.PROTO_LINK_PATTERN.matcher("[$Shelf][]");
+    Truth.assertThat(m.find()).isTrue();
+    m = CommentPatterns.PROTO_LINK_PATTERN.matcher("[][abc]");


src/test/java/com/google/api/codegen/ProtoDocumentLinkTest.java

+    Truth.assertThat(m.find()).isTrue();
+    m = CommentPatterns.PROTO_LINK_PATTERN.matcher("[][abc]");
+    Truth.assertThat(m.find()).isFalse();
+    m = CommentPatterns.PROTO_LINK_PATTERN.matcher("[A-Za-z_$][A-Za-z_$0-9]");


geigerj

LGTM, although as Brian mentioned, more comments would be nice.

src/test/java/com/google/api/codegen/ProtoDocumentLinkTest.java

+    Truth.assertThat(m.find()).isTrue();
+    m = CommentPatterns.PROTO_LINK_PATTERN.matcher("[][abc]");
+    Truth.assertThat(m.find()).isFalse();
+    m = CommentPatterns.PROTO_LINK_PATTERN.matcher("[A-Za-z_$][A-Za-z_$0-9]");


bjwatson · 2016-11-07T18:20:16Z

OK, makes sense. Comments will help make this clear.

On Nov 7, 2016 10:15 AM, "Song Wang" [email protected] wrote:

@swcloud commented on this pull request.

In src/test/java/com/google/api/codegen/ProtoDocumentLinkTest.java
#716:

+import org.junit.Test;
+
+public class ProtoDocumentLinkTest {
+

@test

public void testMatchProtoLink() {

Matcher m;

m = CommentPatterns.PROTO_LINK_PATTERN.matcher("[Shelf][google.example.library.v1.Shelf]");

Truth.assertThat(m.find()).isTrue();

m = CommentPatterns.PROTO_LINK_PATTERN.matcher("[Shelf][]");

Truth.assertThat(m.find()).isTrue();

m = CommentPatterns.PROTO_LINK_PATTERN.matcher("[$Shelf][]");

Truth.assertThat(m.find()).isTrue();

m = CommentPatterns.PROTO_LINK_PATTERN.matcher("[][abc]");

Truth.assertThat(m.find()).isFalse();

m = CommentPatterns.PROTO_LINK_PATTERN.matcher("[A-Za-z_$][A-Za-z_$0-9]");

The reason why [A-Za-z_$][A-Za-z_$0-9] is added as a test case is that it
is a corner case causing the issue.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#716, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAcyy7nNMWCEsO08SqP0UDW47XU1dZ1zks5q72rTgaJpZM4Kn5Ml
.

jmuk · 2016-11-07T18:32:55Z

I was thinking of adding comments to library.proto (so that you can make sure everything is fine in the baseline files on every language).

It is actually not safe as unit tests you wrote, but it could reveal errors on other languages too.

swcloud · 2016-11-07T18:42:32Z

If we want to do comprehensive unit test cases and comments, it should be done in #722 separately.

bjwatson

LGTM, although there is a small typo in one of the comments.

src/test/java/com/google/api/codegen/ProtoDocumentLinkTest.java

    m = CommentPatterns.PROTO_LINK_PATTERN.matcher("[A-Za-z_$][A-Za-z_$0-9]");
    Truth.assertThat(m.find()).isFalse();
+    // Fully qualified name many NOT be numbers only


Fix datastore grpc doc class generation for python and ruby

10c9f12

swcloud assigned bjwatson, jmuk and geigerj Nov 3, 2016

bjwatson reviewed Nov 3, 2016

View reviewed changes

jmuk reviewed Nov 3, 2016

View reviewed changes

* Fix PROTO_LINK_PATTERN

d53c396

* Use Matcher.quoteReplacement to escape '$' in cloud/absolute link

bjwatson suggested changes Nov 3, 2016

View reviewed changes

jmuk reviewed Nov 3, 2016

View reviewed changes

Add unit tests for ProtoLink matching and Ruby/Python/NodeJS CommentF…

6ac372d

…ixer

swcloud mentioned this pull request Nov 4, 2016

Add unit tests for CommentPatterns regular expressions #722

Closed

Improve PROTO_LINK_PATTERN to avoid ambiguity

d3015de

bjwatson reviewed Nov 7, 2016

View reviewed changes

geigerj approved these changes Nov 7, 2016

View reviewed changes

Add more comments in unit test cases

5833031

bjwatson approved these changes Nov 7, 2016

View reviewed changes

Fix typo in comment

77c8684

swcloud merged commit bcf335b into googleapis:master Nov 7, 2016

		public static String rdocify(String comment) {
		// escape '$' in the comment first

Fix datastore grpc doc class generation for python and ruby #716

Fix datastore grpc doc class generation for python and ruby #716

Conversation

swcloud commented Nov 3, 2016 • edited Loading

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

codecov-io commented Nov 3, 2016 • edited Loading

Current coverage is 81.36% (diff: 100%)

bjwatson left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

jmuk left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

bjwatson commented Nov 3, 2016

jmuk commented Nov 3, 2016

bjwatson commented Nov 3, 2016

swcloud commented Nov 4, 2016 • edited Loading

swcloud commented Nov 4, 2016

swcloud commented Nov 4, 2016

bjwatson left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

geigerj left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

bjwatson commented Nov 7, 2016

@swcloud commented on this pull request.

jmuk commented Nov 7, 2016

swcloud commented Nov 7, 2016 • edited Loading

bjwatson left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

swcloud commented Nov 3, 2016 •

edited

Loading

codecov-io commented Nov 3, 2016 •

edited

Loading

swcloud commented Nov 4, 2016 •

edited

Loading

swcloud commented Nov 7, 2016 •

edited

Loading