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

Update vendored GSON dependency to latest available version #415

Merged
merged 3 commits into from
Jan 18, 2019

Conversation

fractalwrench
Copy link
Contributor

Goal

The JsonWriter class was originally vendored in 2014, and has not been updated since then despite bug fixes to the GSON library.

Design

It was originally suggested that we use the framework version of JsonWriter. However, this approach has several issues:

  1. The framework class is final, therefore cannot be extended in Java, meaning we would need to make substantial modifications to our implementation of JsonStream
  2. setSerializeNulls is not present, meaning we would always serialize null values if we used the framework version
  3. The beforeValue method is private and therefore cannot be accessed, meaning we would need to make further alterations to JsonStream to support serialising a file.

Therefore the lowest risk approach seemed to be updating the vendored GSON dependency, rather than relying on the framework version.

Changeset

Updated the JsonWriter and JsonScope classes to the latest available version on the GSON repository.

SuppressAllWarnings was used to suppress sites in the GSON code which violate our existing checkstyle/lint rules.

Tests

Ran existing tests on CI.

Copy link
Contributor

@bengourley bengourley left a comment

Choose a reason for hiding this comment

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

Seems good to me.

I would just like clarification on why the false parameter was removed from the beforeValue() call.

Also this was mentioned elsewhere "reference the commit hash somewhere" – I think you should add a comment to the top of the file saying // retrieved version [x] from [url] on [date]. And on that note, what code did you copy? I would take it from the latest stable tag (rather than master) which looks to be gson-parent-2.8.5.

@fractalwrench
Copy link
Contributor Author

Thanks @bengourley - I've added a note to each class that states where it was retrieved from.

@fractalwrench fractalwrench merged commit 840c49d into next Jan 18, 2019
@fractalwrench fractalwrench deleted the update-gson-dep branch January 18, 2019 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants