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

Delete unused LinkedHashTreeMap #1992

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Oct 11, 2021

Class seems to be unused since commit f29d5bc.
Gson currently only uses LinkedTreeMap.

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

eamonnmcmanus commented Oct 12, 2021

I'm concerned that this a public class. Even though it's in a package with .internal. in the name, somebody out there is probably using it. Maybe just mark it deprecated?

@Marcono1234
Copy link
Collaborator Author

Marcono1234 commented Oct 12, 2021

Marking it deprecated for now would also be alright for me, but I think it should be removed eventually. There is no point in including a class (and maintaining tests for it) which is not used by Gson at all; if we ever need it again we can revive it from the Git history. It appears that class was originally introduced to protect against DoS attacks (see this StackOverflow comment), but Java's HashMap implementation has been adjusted for a quite a while now already to protect against hash collision DoS attacks (see JDK-8046170), so if someone is using Gson's LinkedHashTreeMap they can probably just switch to using Java's LinkedHashMap (which extends HashMap) or TreeMap. Maybe there is even no need for Gson's LinkedTreeMap anymore and Gson could use Java's LinkedHashMap instead, but that is a different story.
What do you think?

@inder123
Copy link
Collaborator

inder123 commented Oct 13, 2021

Users use classes from internal at their own risk, we should merge it.

@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Oct 16, 2021

OK, given that the need for this class is obviated by DoS protection in the JDK and given that the class is .internal. I think we can probably delete it. However, as I mentioned in #1989, I'd like to make a Gson release before merging these potentially-breaking changes.

Class seems to be unused since commit f29d5bc.
Gson currently only uses LinkedTreeMap.
@Marcono1234 Marcono1234 force-pushed the marcono1234/delete-LinkedHashTreeMap branch from 4efb3dd to 42f2076 Compare Oct 24, 2021
@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Nov 1, 2021

2.8.9 has now been released, so as agreed I am merging this.

@eamonnmcmanus eamonnmcmanus merged commit e0de45f into google:master Nov 1, 2021
3 checks passed
@Marcono1234 Marcono1234 deleted the marcono1234/delete-LinkedHashTreeMap branch Nov 1, 2021
tibor-universe pushed a commit to getuniverse/gson that referenced this pull request Nov 21, 2021
Class seems to be unused since commit f29d5bc.
Gson currently only uses LinkedTreeMap.
YavorPaunov pushed a commit to YavorPaunov/vespa that referenced this pull request Sep 8, 2022
The  is an internal class never
intended for use outside of GSON.
It was removed in google/gson#1992 (version 2.9.0) and
its use in this project prevents the GSON version from being bumped.

The standard  is a perfect drop-in replacement as it
also preserves insertion order.
YavorPaunov pushed a commit to YavorPaunov/vespa that referenced this pull request Sep 8, 2022
LinkedHashTreeMap is an internal class never intended for use outside of GSON.
It was removed in google/gson#1992 (version 2.9.0) and
its use in this project prevents the GSON version from being bumped.

The standard LinkedHashMap is a perfect drop-in replacement as it
also preserves insertion order.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants