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

Deprecate Gson.excluder() exposing internal Excluder class #1986

Merged

Conversation

@Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Oct 11, 2021

Resolves #1903

Additionally adds documentation to some Gson methods which did not have documentation yet.

@google-cla google-cla bot added the cla: yes label Oct 11, 2021
@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Oct 12, 2021

Seems like some people are using this so I'm not sure we can remove it.

@Marcono1234
Copy link
Contributor Author

@Marcono1234 Marcono1234 commented Oct 12, 2021

Hmm good point. Maybe Gson should then expose (parts of) the Excluder class? It looks like most projects are interested in whether Gson is configured to ignore a certain class (excludeClass) or field (excludeField). Everything else of Excluder can most likely be implemented by the user themselves.
So I think Gson should not directly expose Excluder. These used methods are actually pretty close to the methods of the public ExclusionStrategy interface. The only issue is that it does not differentiate between serialization and deserialization (so Gson would need one serialization and deserialization method which returns an ExclusionStrategy wrapping the underyling Excluder), and that ExclusionStrategy for some reason uses FieldAttributes instead of directly Field. From the Git commit 839b0c2 which added this class it is not entirely clear to my why this was done. Maybe to avoid users messing with the state of the reflection Field object, e.g. whether it is accessible or not.

What do you think? We can also deprecate Gson.excluder() for now, but probably eventually need to do something about this. Or we deprecate it and just wait until people start to complain ...; once they use recent Gson versions with Java module descriptor they will notice that they are using internal classes.

@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Oct 15, 2021

Obviously it was a mistake to add a method to the public API that returns an .internal. type, but I'm not sure we can fix that mistake without making things worse. I think deprecation is probably the way to go, and we probably won't ever remove the method.

@Marcono1234 Marcono1234 force-pushed the marcono1234/fix-exposing-Excluder branch from 2fe4674 to e9921d8 Oct 24, 2021
@Marcono1234 Marcono1234 changed the title Remove Gson.excluder() exposing internal Excluder class Deprecate Gson.excluder() exposing internal Excluder class Oct 24, 2021
@Marcono1234
Copy link
Contributor Author

@Marcono1234 Marcono1234 commented Oct 24, 2021

Ok, I have repurposed this pull request to only deprecate the excluder() method, saying that it "might be removed in a future version", even if we never remove it.

@eamonnmcmanus eamonnmcmanus merged commit c54caf3 into google:master Oct 25, 2021
3 checks passed
@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Oct 25, 2021

Thanks!

@Marcono1234 Marcono1234 deleted the marcono1234/fix-exposing-Excluder branch Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants