Skip to content

fix: properly cache GraphQL object/interfaces/union types #291

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

Merged
merged 1 commit into from
Aug 5, 2019

Conversation

dariuszkuc
Copy link
Collaborator

@dariuszkuc dariuszkuc commented Aug 3, 2019

graphql-java requires all GraphQLObjectType, GraphQLInterfaceType and GraphQLUnionType types to have unique names within a schema. While generating the schemas based on the reflections we have to ensure that each one of those objects is build only once. Since underlying objects do not implement hashcode it leads to problems if we attempt to update the fields through some hooks (e.g. use newBuilder(existingObject)). In order to make sure we only build those objects once we implemented basic caching logic in our builders - when we start building the target object we put it under construction and after it is build we add it to the cache and mark it completed.

Unfortunately, existing logic was flawed - all objects were being put under construction but only GraphQLObjectTypes generated when processing their GraphQLInterfaceType were marked as completed and added to the cache. This change fixes the caching logic to properly complete all GraphQLObjectType, GraphQLInterfaceType and GraphQLUnionType types and add them to the underlying cache.

@codecov
Copy link

codecov bot commented Aug 3, 2019

Codecov Report

Merging #291 into master will decrease coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #291      +/-   ##
============================================
- Coverage     64.69%   64.57%   -0.13%     
+ Complexity      198      195       -3     
============================================
  Files           103      103              
  Lines          1065     1067       +2     
  Branches        151      153       +2     
============================================
  Hits            689      689              
- Misses          363      364       +1     
- Partials         13       14       +1
Impacted Files Coverage Δ Complexity Δ
...ia/graphql/generator/state/SchemaGeneratorState.kt 100% <ø> (ø) 4 <0> (-2) ⬇️
...xpedia/graphql/generator/types/InterfaceBuilder.kt 100% <100%> (ø) 2 <0> (ø) ⬇️
...n/com/expedia/graphql/generator/SchemaGenerator.kt 97.5% <100%> (ø) 18 <0> (ø) ⬇️
.../com/expedia/graphql/generator/state/TypesCache.kt 100% <100%> (ø) 20 <3> (-1) ⬇️
...om/expedia/graphql/generator/types/UnionBuilder.kt 80% <0%> (-10%) 2% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d31ae62...3138717. Read the comment docs.

@dariuszkuc dariuszkuc marked this pull request as ready for review August 3, 2019 23:27
`graphql-java` requires all `GraphQLObjectType`, `GraphQLInterfaceType` and `GraphQLUnionType` types to have unique names within a schema. While generating the schemas based on the reflections we have to ensure that each one of those objects is build only once. Since underlying objects do not implement hashcode it leads to problems if we attempt to update the fields through some hooks (e.g. use `newBuilder(existingObject)`). In order to make sure we only build those objects once we implemented basic caching logic in our builders - when we start building the target object we put it under construction and after it is build we add it to the cache and mark it completed.

Unfortunately, existing logic was flawed - all objects were being put under construction but only `GraphQLObjectType`s generated when processing their `GraphQLInterfaceType` were marked as completed and added to the cache. This change fixes the caching logic to properly complete all `GraphQLObjectType`, `GraphQLInterfaceType` and `GraphQLUnionType` types and add them to the underlying cache.
@smyrick smyrick added changes: patch Changes require a patch version type: bug Something isn't working labels Aug 5, 2019
@smyrick smyrick merged commit 10da005 into ExpediaGroup:master Aug 5, 2019
@dariuszkuc dariuszkuc deleted the type_cache branch August 5, 2019 15:38
smyrick pushed a commit to smyrick/graphql-kotlin that referenced this pull request Sep 11, 2019
…up#291)

`graphql-java` requires all `GraphQLObjectType`, `GraphQLInterfaceType` and `GraphQLUnionType` types to have unique names within a schema. While generating the schemas based on the reflections we have to ensure that each one of those objects is build only once. Since underlying objects do not implement hashcode it leads to problems if we attempt to update the fields through some hooks (e.g. use `newBuilder(existingObject)`). In order to make sure we only build those objects once we implemented basic caching logic in our builders - when we start building the target object we put it under construction and after it is build we add it to the cache and mark it completed.

Unfortunately, existing logic was flawed - all objects were being put under construction but only `GraphQLObjectType`s generated when processing their `GraphQLInterfaceType` were marked as completed and added to the cache. This change fixes the caching logic to properly complete all `GraphQLObjectType`, `GraphQLInterfaceType` and `GraphQLUnionType` types and add them to the underlying cache.
dariuszkuc added a commit to dariuszkuc/graphql-kotlin that referenced this pull request Aug 5, 2022
…up#291)

`graphql-java` requires all `GraphQLObjectType`, `GraphQLInterfaceType` and `GraphQLUnionType` types to have unique names within a schema. While generating the schemas based on the reflections we have to ensure that each one of those objects is build only once. Since underlying objects do not implement hashcode it leads to problems if we attempt to update the fields through some hooks (e.g. use `newBuilder(existingObject)`). In order to make sure we only build those objects once we implemented basic caching logic in our builders - when we start building the target object we put it under construction and after it is build we add it to the cache and mark it completed.

Unfortunately, existing logic was flawed - all objects were being put under construction but only `GraphQLObjectType`s generated when processing their `GraphQLInterfaceType` were marked as completed and added to the cache. This change fixes the caching logic to properly complete all `GraphQLObjectType`, `GraphQLInterfaceType` and `GraphQLUnionType` types and add them to the underlying cache.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes: patch Changes require a patch version type: bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants