Skip to content

feat: GsonFactory to have read leniency option #1819

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Feb 24, 2023
Merged

feat: GsonFactory to have read leniency option #1819

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5ad553b
feat: GsonFactory to have read leniency option
suztomo Feb 21, 2023
85b74b9
copy method
suztomo Feb 21, 2023
b605bf2
protected to let subclasses to create copy
suztomo Feb 21, 2023
24e2b11
static clone method to avoid clone problem
suztomo Feb 21, 2023
7aaf5b5
Added note for subclass
suztomo Feb 23, 2023
041460f
Builder class
suztomo Feb 23, 2023
0da37e6
private constructor to read from builder.
suztomo Feb 23, 2023
11d7165
document
suztomo Feb 23, 2023
44bd456
removed irrelevant test
suztomo Feb 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Added note for subclass
  • Loading branch information
@suztomo
suztomo committed Feb 23, 2023
commit 7aaf5b578fcce8f72c5cdd4f913b06991cdbbda9
10 changes: 8 additions & 2 deletions google-http-client-gson/src/main/java/com/google/api/client/json/gson/GsonFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ public static GsonFactory getDefaultInstance() {
return InstanceHolder.INSTANCE;
}

private boolean readLeniency = false;
/** Controls the behavior of leniency in reading JSON value */
protected boolean readLeniency = false;

/** Holder for the result of {@link #getDefaultInstance()}. */
@Beta
Expand Down Expand Up @@ -104,7 +105,12 @@ private static GsonFactory newInstance(GsonFactory gsonFactory) {
return copy;
}

/** Returns a copy of GsonFactory instance which is lenient when reading JSON value. */
/**
* Returns a copy of GsonFactory instance which is lenient when reading JSON value.
*
* <p>Subclasses should not call this method. Set {@code readLeniency} field to {@code true}
* instead.
*/
public GsonFactory withReadLeniency() {
GsonFactory copy = newInstance(this);
Copy link
Member

@burkedavison burkedavison Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

But now it's static.

Copy link
Member

@burkedavison burkedavison Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Consider avoiding the copy problem altogether.

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

Copy link
Member Author

@suztomo suztomo Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@suztomo suztomo Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

"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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

copy.readLeniency = true;
Expand Down