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

Replace ReflectionCheckingTypeAdapterFactory with a ReflectionAccessFilter #1496

Merged
merged 3 commits into from
Dec 21, 2022

Conversation

richardm-stripe
Copy link
Contributor

@richardm-stripe richardm-stripe commented Dec 20, 2022

Notify

r? @stripe/api-library-reviewers

Summary

Fixes #1494

  • Updates GSON by a patch version.
  • Removes ReflectionCheckingTypeAdapterFactory -- this should be non breaking as the visibility is package-private.
  • Adds a ReflectionAccessFilter (new in Gson 2.9.1) as a replacement.

I have NOT actually set up a project to reproduce #1494, but I am still highly confident this will fix it as it is completely removing the class with the suspect import statement.

Context

Basically, we want to throw an exception when GSON is asked to use reflection and deserialize a class that isn't beneath com.stripe., usually this happens when the class has an "infrastructure" field that doesn't represent data, but the field is not marked "transient", e.g. in #1197 the issue was that StripeCollection had a private RequestOptions requestOptions that should have been private transient RequestOptions requestOptions. ("transient" is a Java keyword that means "shouldn't be serialized"). The ReflectionCheckingTypeAdapter was added to give a better error message and help prevent us from making mistakes like this again (the error message in #1190 is pretty hard to interpret), but the ReflectionAccessFilter should do the same thing without causing the module path issues mentioned in #1494.

@richardm-stripe richardm-stripe changed the title [WIP] Replace ReflectionCheckingTypeAdapterFactory with a ReflectionAccessFilter Replace ReflectionCheckingTypeAdapterFactory with a ReflectionAccessFilter Dec 20, 2022
@pakrym-stripe
Copy link
Contributor

pakrym-stripe commented Dec 20, 2022

This is such an amazing find! There's barely a mention of addReflectionAccessFilter on the internet. Are we worried that this feature is too new and will break customers that have older GSON versions?

@richardm-stripe richardm-stripe marked this pull request as ready for review December 20, 2022 22:20
@richardm-stripe
Copy link
Contributor Author

All credit goes to Marcono1234 who commented on #1197.

Stripe-java presently needs GSON >= 2.8.2 (when JsonElement.deepCopy was added used here), from 2017.

By SemVer, 2.9.1 should be completely backwards compatible with 2.8.2 so in theory any user upgrading stripe-java should also be able to upgrade GSON without problems.

This is five years of GSON versions we lose compatibility with, but I don't think we should default to assuming SemVer is untrustworthy and users won't be able to update. If we ever get any feedback about a versioning conflict, we can always release a newer version with a workaround.

@richardm-stripe richardm-stripe enabled auto-merge (squash) December 21, 2022 20:42
@richardm-stripe richardm-stripe merged commit e9fcfaa into master Dec 21, 2022
richardm-stripe added a commit that referenced this pull request Dec 29, 2022
#1476)

* Solution For "Regarding ability to override stripe api url per API request #1181".
#1181

* normalizeApiBase method added

* Replace ReflectionCheckingTypeAdapterFactory with a ReflectionAccessFilter (#1496)

* Replace ReflectionCheckingTypeAdapterFactory with a Filter

* Add test

* API Updates (#1497)

* Codegen for openapi v216

* Bump version to 22.4.0

* Actually release (#1499)

* Deprecate ApiResource.classUrl, etc.

* Tweak RequestOptions and ApiResource

* Format

* Codegen

* Overridden methods

* Update src/main/java/com/stripe/net/ApiResource.java

Co-authored-by: pakrym-stripe <99349468+pakrym-stripe@users.noreply.github.com>

* Revert "Update src/main/java/com/stripe/net/ApiResource.java"

This reverts commit c263ce0.

Co-authored-by: Yasir Shabbir <javaProgrammer44@gmail.com>
Co-authored-by: Richard Marmorstein <52928443+richardm-stripe@users.noreply.github.com>
Co-authored-by: Richard Marmorstein <richardm@stripe.com>
Co-authored-by: anniel-stripe <97691964+anniel-stripe@users.noreply.github.com>
Co-authored-by: pakrym-stripe <99349468+pakrym-stripe@users.noreply.github.com>
@remi-stripe remi-stripe deleted the richardm-better-reflection-blocking branch September 28, 2023 16:24
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.

Library accesses GSON internal API, breaks with gson on the module path
3 participants