feat: GsonFactory to have read leniency option #1819

suztomo · 2023-02-21T15:52:51Z

Fixes b/269515918, where the prefix )]}'\n for Cross-site script inclusion (XSSI) didn't work for GsonFactory.

Users will call GsonFactory.builder().setReadLeniency(true).build() when they want this library to process JSON input that may have the prefix.

For this pull request, it controlls only the leniency when reading input JSON. This design withReadLeniency() gives flexibility for us to choose writeLeniency in future if necessary. (It's possible that read leniency is true and write leniency is false.)
It does not affect existing usage of GsonFactory. (No global state in GsonFactory)

BEGIN_COMMIT_OVERRIDE
feat: GsonFactory to have read leniency option via GsonFactory.builder().setReadLeniency(true).build()
END_COMMIT_OVERRIDE

suztomo · 2023-02-21T16:06:52Z

lint (non required) fails:

Error:  Found 1 non-complying files, failing build
Error:  To fix formatting errors, run "mvn com.coveo:fmt-maven-plugin:format"
Error:  Non complying file: /home/runner/work/google-http-java-client/google-http-java-client/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java

This is irrelevant to this pull request.

google-http-client-gson/src/main/java/com/google/api/client/json/gson/GsonFactory.java

suztomo · 2023-02-23T20:39:10Z

google-http-client-gson/src/main/java/com/google/api/client/json/gson/GsonFactory.java

+    return readLeniency;
+  }
+
+  private static GsonFactory newInstance(GsonFactory gsonFactory) {


As per Effective Java's clone section, creating a factory method. I don't think a subclass needs to access this method.

burkedavison · 2023-02-23T20:56:01Z

google-http-client-gson/src/main/java/com/google/api/client/json/gson/GsonFactory.java

+
+  /** Returns a copy of GsonFactory instance which is lenient when reading JSON value. */
+  public GsonFactory withReadLeniency() {
+    GsonFactory copy = newInstance(this);


If we're supporting the subclassing use-case, I don't see how this works. Is the intent that subclasses are required to override this function? (If so, we're introducing a backwardly incompatible change to the subclass contract.)

As an example:

// Logs all method invocations. public class LoggingGsonFactory extends GsonFactory { /* ... */ } // In some 'main'-type class of the application... public static GsonFactory createGsonFactory() { return new LoggingGsonFactory(); } // Then elsewhere... private GsonFactory gsonFactory = createGsonFactory().withReadLeniency(); // Oops! No more logging.

Good point. Then subclasses needs to override newInstance. I'm making newInstance protected.

But now it's static.

Consider avoiding the copy problem altogether.

private boolean isLenient; public GsonFactory() { this(false); } public GsonFactory(boolean isLenient) { this.isLenient = isLenient; } // No withReadLeniency() method.

The boolean flag in the constructor is not good (Avoid boolean parameter go/java-practices/boolean-parameters). It's ambiguous, especially when we want to introduce leniency in writing JSON.

Added * <p>Subclasses should not call this method. Set {@code readLeniency} field to {@code true} instead..

"Subclasses should not call this method" is fine if you know you're dealing with a subclass. The point of polymorphism is that you shouldn't care.

There's other ways to take in a setting besides using a boolean if you feel it goes against best practices.

public enum Leniency { LENIENT, STRICT } private final Leniency leniency; public GsonFactory() { this(Leniency.STRICT); } public GsonFactory(Leniency leniency) { this.leniency = Objects.requireNonNull(leniency); }

or a Builder, or a Settings object parameter.

As talked, we may want to add more parameters (such as writeLeniency) in future and having different contstructors for different parameter is not good.

I'm bringing Builder class.

suztomo

@burkedavison PTAL.

suztomo · 2023-02-23T21:21:45Z

google-http-client-gson/src/main/java/com/google/api/client/json/gson/GsonFactory.java

+
+  /** Returns a copy of GsonFactory instance which is lenient when reading JSON value. */
+  public GsonFactory withReadLeniency() {
+    GsonFactory copy = newInstance(this);


Added * <p>Subclasses should not call this method. Set {@code readLeniency} field to {@code true} instead..

suztomo · 2023-02-23T21:52:47Z

google-http-client-gson/src/test/java/com/google/api/client/json/gson/GsonFactoryTest.java

+
+  public final void testReaderLeniency_lenient() throws IOException {
+    JsonObjectParser parser =
+        new JsonObjectParser(GsonFactory.builder().setReadLeniency(true).build());


Memo: This is how to create GsonFactory with readLeniency true.

suztomo

@burkedavison PTAL.

suztomo · 2023-02-24T02:20:17Z

google-http-client-gson/src/test/java/com/google/api/client/json/gson/GsonParserTest.java


 public class GsonParserTest extends AbstractJsonParserTest {

  @Override
  protected JsonFactory newJsonFactory() {
    return new GsonFactory();
  }
+
+  public void testParse_leniency() throws IOException {


This wasn't supposed to come here.

This also changes the JDK distribution from zulu to temurin https://github.com/actions/setup-java#eclipse-temurin Source-Link: googleapis/synthtool@ef9fe2e Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:31c8276a1bfb43766597d32645721c029cb94571f1b8d996cb2c290744fe52f9

suztomo requested a review from a team as a code owner February 21, 2023 15:52

product-auto-label bot added the size: s Pull request size is small. label Feb 21, 2023

suztomo force-pushed the gson_factory_leniency branch from fbde6d2 to 8528bdb Compare February 21, 2023 15:58

feat: GsonFactory to have read leniency option

5ad553b

suztomo force-pushed the gson_factory_leniency branch from 8528bdb to 5ad553b Compare February 21, 2023 15:59

copy method

85b74b9

product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Feb 21, 2023

suztomo requested a review from chingor13 February 21, 2023 16:06

burkedavison reviewed Feb 21, 2023

View reviewed changes

google-http-client-gson/src/main/java/com/google/api/client/json/gson/GsonFactory.java Outdated Show resolved Hide resolved

suztomo requested review from meltsufin and a team February 21, 2023 16:07

suztomo added 2 commits February 21, 2023 11:11

protected to let subclasses to create copy

b605bf2

static clone method to avoid clone problem

24e2b11

suztomo requested a review from burkedavison February 23, 2023 20:37

suztomo commented Feb 23, 2023

View reviewed changes

burkedavison reviewed Feb 23, 2023

View reviewed changes

Added note for subclass

7aaf5b5

suztomo commented Feb 23, 2023

View reviewed changes

Builder class

041460f

suztomo commented Feb 23, 2023

View reviewed changes

suztomo added 2 commits February 23, 2023 17:34

private constructor to read from builder.

0da37e6

document

11d7165

suztomo force-pushed the gson_factory_leniency branch from cfbeec9 to 11d7165 Compare February 23, 2023 22:37

burkedavison approved these changes Feb 24, 2023

View reviewed changes

suztomo commented Feb 24, 2023

View reviewed changes

removed irrelevant test

44bd456

suztomo merged commit 00d61b9 into googleapis:main Feb 24, 2023

release-please bot mentioned this pull request Feb 24, 2023

chore(main): release 1.43.0 #1768

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: GsonFactory to have read leniency option #1819

feat: GsonFactory to have read leniency option #1819

suztomo commented Feb 21, 2023 •

edited

suztomo commented Feb 21, 2023

suztomo Feb 23, 2023

burkedavison Feb 23, 2023 •

edited

suztomo Feb 23, 2023

suztomo Feb 23, 2023

burkedavison Feb 23, 2023 •

edited

suztomo Feb 23, 2023 •

edited

suztomo Feb 23, 2023 •

edited

burkedavison Feb 23, 2023

suztomo Feb 23, 2023

suztomo left a comment

suztomo Feb 23, 2023 •

edited

suztomo Feb 23, 2023

suztomo left a comment

suztomo Feb 24, 2023

feat: GsonFactory to have read leniency option #1819

feat: GsonFactory to have read leniency option #1819

Conversation

suztomo commented Feb 21, 2023 • edited

suztomo commented Feb 21, 2023

suztomo Feb 23, 2023

Choose a reason for hiding this comment

burkedavison Feb 23, 2023 • edited

Choose a reason for hiding this comment

suztomo Feb 23, 2023

Choose a reason for hiding this comment

suztomo Feb 23, 2023

Choose a reason for hiding this comment

burkedavison Feb 23, 2023 • edited

Choose a reason for hiding this comment

suztomo Feb 23, 2023 • edited

Choose a reason for hiding this comment

suztomo Feb 23, 2023 • edited

Choose a reason for hiding this comment

burkedavison Feb 23, 2023

Choose a reason for hiding this comment

suztomo Feb 23, 2023

Choose a reason for hiding this comment

suztomo left a comment

Choose a reason for hiding this comment

suztomo Feb 23, 2023 • edited

Choose a reason for hiding this comment

suztomo Feb 23, 2023

Choose a reason for hiding this comment

suztomo left a comment

Choose a reason for hiding this comment

suztomo Feb 24, 2023

Choose a reason for hiding this comment

suztomo commented Feb 21, 2023 •

edited

burkedavison Feb 23, 2023 •

edited

burkedavison Feb 23, 2023 •

edited

suztomo Feb 23, 2023 •

edited

suztomo Feb 23, 2023 •

edited

suztomo Feb 23, 2023 •

edited