Replace `BiMap` with plain maps
Maintainers usually reply within 1 day
Nobody has claimed this yet.
Assessment
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Newbie friendliness
- 45/100
- Issue type
- Refactor
- Clarity
- Mostly clear
- Activity status
- Active
- Domain
- backend-api-design
Research direction
Start by tracing how SchemaClassScanner builds TypeClassDictionary and how SchemaParser uses it in both directions, including resolver return types. Then review SchemaParserDictionary.getDictionary() and the graphql-spring-boot call to builder.dictionary(); done means duplicate classes produce a SchemaClassScannerError naming the class and types, the new map-returning method is used without breaking the old API, and BiMap can be removed in a later major release.
Written by the indexing model from the issue text.
Description
graphql.kickstart.tools.util.BiMap is a copy of Guava's BiMap we added when we dropped Guava (661769e). Neither Java nor Kotlin has a bidirectional map, so it stuck around. After #851 it doesn't really pull its weight anymore.
Where it's used:
SchemaClassScannerbuilds aBiMap<TypeDefinition, JavaType>(TypeClassDictionary).SchemaParserreads it both ways:dictionary[definition]for enum classes anddictionary.inverse()forInterfaceTypeResolverandUnionTypeResolver.BiMap#putthrowingvalue already presentis the only thing that enforces one class per output type. It shows up asError creating bimap of type => class, which isn't a very helpful message. It also catches classes found through resolver return types, not just dictionary entries, so a replacement needs an explicit check.SchemaParserDictionarystores a plain map since #851.getDictionary()still returns aBiMapfor backwards compatibility (graphql-spring-boot callsbuilder.dictionary(dictionary.getDictionary())), but itsinverse()only keeps the first name when several input types share a class.
Proposal:
- Internally, use two plain maps plus an explicit duplicate check that throws a
SchemaClassScannerErrornaming the class and the types. No API change.
typeToClass.entries.groupBy({ it.value }, { it.key })
.filterValues { it.size > 1 }
.forEach { (clazz, types) -> throw SchemaClassScannerError("...") }
val classToType = typeToClass.entries.associate { (type, clazz) -> clazz to type }
- Add a replacement for
getDictionary()that returnsMap<String, Class<*>>under a new name, and deprecategetDictionary(). Changing the return type in place would break binary compatibility. - Switch graphql-spring-boot to the new method.
- Remove
BiMapin the next major.
- Dominant language
- Kotlin
- Stars
- 824
- Forks
- 175
- Avg merge
- 2d 5h
- Merged PRs (30d)
- 52
Getting set up
This project ships no dev container, Dockerfile or contributing guide, so setting up is up to you: start from its README, and see our first-contribution guide for the general steps.
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from graphql-java-kickstart/graphql-java-tools
-
Difficulty 2/5 1-3 hours Newbie friendliness 85/100
graphql-java-kickstart/graphql-java-tools#893 ·
Maintainers usually reply within 1 day
-
bug
Difficulty 3/5 1-2 days Newbie friendliness 74/100
graphql-java-kickstart/graphql-java-tools#896 ·
Maintainers usually reply within 1 day
-
Difficulty 3/5 1-2 days Newbie friendliness 72/100
graphql-java-kickstart/graphql-java-tools#889 ·
Maintainers usually reply within 1 day
-
bug
Difficulty 3/5 1-2 days Newbie friendliness 55/100
graphql-java-kickstart/graphql-java-tools#886 ·
Maintainers usually reply within 1 day
-
Reject directives applied at the wrong location, without a required argument or twice in 15.0.0Openenhancement
Difficulty 3/5 1-2 days Newbie friendliness 72/100
graphql-java-kickstart/graphql-java-tools#884 ·
Maintainers usually reply within 1 day
All issues in graphql-java-kickstart/graphql-java-tools
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
auth0/Auth0.Android#1101 ·
Maintainers usually reply within 1 day
-
LiquidGlass / LiquidGlassContainer (iOS): padded content measured at the outer width and overflows by the paddingPossibly taken @aleclarson claimed this today. Open
Difficulty 2/5 1-3 hours Newbie friendliness 74/100
NativeScript/NativeScript#11561 ·
Maintainers usually reply within 1 day
-
bug
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
navikt/mock-oauth2-server#1129 ·
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 77/100
-
unconfirmed
Difficulty 2/5 1-3 hours Newbie friendliness 65/100
thunderbird/thunderbird-android#11778 · 2 comments ·
Maintainers usually reply within 1 day