Skip to content

Conversation

@igdianov
Copy link
Collaborator

@igdianov igdianov commented Oct 19, 2019

This PR is a fork from #205 that adds the following changes to original commit:

  • refactor: added DataFetchingEnvironmentBuilder wrapper class …
  • refactor(JavaScalars): Renamed method toScalarType to newScalarType
  • fix: Resolved NPE with nullable arguments in GraphQLJpaExecutor.execute
  • polish: Replaced NullValue builder() with NullValue.Nullinstance
  • refactor: Removed ExecutionResult implementation from GraphQLJpaQueryStarterIT
  • feat: added support to merge CodeRegistries
  • feat: added test example for graphql-java-annotations library

@igdianov igdianov self-assigned this Oct 19, 2019
@codecov
Copy link

codecov bot commented Oct 19, 2019

Codecov Report

Merging #209 into master will decrease coverage by 0.04%.
The diff coverage is 78.32%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #209      +/-   ##
===========================================
- Coverage     73.55%   73.5%   -0.05%     
- Complexity      850     863      +13     
===========================================
  Files            49      50       +1     
  Lines          3668    3722      +54     
  Branches        607     612       +5     
===========================================
+ Hits           2698    2736      +38     
- Misses          706     719      +13     
- Partials        264     267       +3
Impacted Files Coverage Δ Complexity Δ
...phql/jpa/query/schema/impl/GraphQLJpaExecutor.java 100% <100%> (ø) 3 <1> (ø) ⬇️
...ventures/graphql/jpa/query/schema/JavaScalars.java 46.22% <100%> (+0.71%) 6 <1> (+1) ⬆️
...query/schema/impl/GraphQLJpaSimpleDataFetcher.java 90% <100%> (ø) 8 <3> (+1) ⬆️
...jpa/query/schema/impl/GraphQLJpaSchemaBuilder.java 91.43% <100%> (ø) 113 <0> (ø) ⬇️
...ry/schema/impl/DataFetchingEnvironmentBuilder.java 25% <25%> (ø) 1 <1> (?)
...a/query/schema/impl/QraphQLJpaBaseDataFetcher.java 73.29% <70%> (+0.76%) 149 <1> (ø) ⬇️
.../query/autoconfigure/GraphQLSchemaFactoryBean.java 68.8% <74.19%> (-5.34%) 15 <15> (+6)
.../query/schema/impl/GraphQLJpaQueryDataFetcher.java 87.96% <87.5%> (-0.61%) 31 <10> (+4)

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 5ce95b5...6a59e79. Read the comment docs.

@molexx
Copy link
Contributor

molexx commented Oct 28, 2019

Thanks, this is mostly working for me.

I'm having problems when merging with the schema generated by graphql-java-annotations though. The schema appears correct but the DataFetchers aren't executed. It looks like the schema merging code in GraphQLSchemaFactoryBean.java doesn't account for CodeRegistries, do you think it ought to?

I've confirmed that it works ok if I declare its GraphQLSchema as the primary Spring bean and so disabling the rest of the autoconfiguring and merging.

Thanks

@igdianov
Copy link
Collaborator Author

@molexx Thanks! I will add support for code registry merge to enable merging different schemas via factory bean. Stay tuned...

@negesti
Copy link
Contributor

negesti commented Oct 29, 2019

@igdianov as you mentioned in #205 adding the CodeRegistry to the SchemaFactory Bean would require changes in v0.3 not only v.0.4

One option would be, to add an additional GraphQLSchemaFactoryBean that extends the default one and adds support for the CodeRegistry. Imho this would only require some minor changes in the existing FactoryBean (mostly changing private to protected) and makes "back-porting" changes from 0.4 to 0.3 easier.
Adding an empty function like "mergeAdditionalSchemaData" would be great for #163 to. Or maybe a flexible way to add SchemaMergers that get both schemas and can merge/copy definitions

@igdianov
Copy link
Collaborator Author

igdianov commented Nov 3, 2019

@negesti @molexx Please, see the proposed support for handling merge of code registries in this commit: 77e8497

It should make it possible to do transparently when you want to instrument your schema with third party instrumentation tools or by hand.

In my opinion, the code registry is a poorly designed feature that makes field to code bindings with static coordinates(type, field) instead of object tree. There is no validation of any kind in GraphQL, so you can make a mistake in coordinate names and add code binding to non-existing field for example. May be we should have validation to fail fast?

@igdianov
Copy link
Collaborator Author

igdianov commented Nov 3, 2019

@molexx I have added example tests for graphql-java-annotations library: 744dddf

Feel free to add any other test case that you may want to cover with code registry

@molexx
Copy link
Contributor

molexx commented Nov 4, 2019

Yes this works for me now, thanks! :-)

@molexx
Copy link
Contributor

molexx commented Nov 5, 2019

RE validation, yes it would be good I think. There is validation elsewhere - non matching or duplicate types for instance - so it would be consistent.

This functionality is creeping away from JPA. I wonder if it might make sense to split this out into a separate stitching library for graphql-java - and optionally Spring - that graphql-jpa-query-autoconfigure can depend on?

@igdianov igdianov force-pushed the upgrade-to-graphql-java-13 branch 2 times, most recently from 706b7ab to 518b248 Compare December 1, 2019 09:15
@igdianov igdianov force-pushed the upgrade-to-graphql-java-13 branch 2 times, most recently from 7ec278c to 67d2e55 Compare December 16, 2019 06:45
clemens and others added 12 commits December 15, 2019 23:00
* replace custom NullValue with graphql.language.NullValue (DataFetcher)

Deprecated in 13.0
* Replace new GraphQLScalarType with newScalar Builder
* use Field.transform to change arguments (all field properties are immutable)
* replace DataFetchingEnvironmentBuilder with DataFetchingEnvironmentImpl (graphql internal class!)
* variables have been moved from DataFetchingEnvironment.Environment directly to DataFetchingEnvironment.variables
in order to bridge changes in GraphQL-java 13
@igdianov igdianov force-pushed the upgrade-to-graphql-java-13 branch from 67d2e55 to 6a59e79 Compare December 16, 2019 07:00
@igdianov igdianov merged commit 09dd88c into master Dec 29, 2019
@igdianov igdianov deleted the upgrade-to-graphql-java-13 branch December 29, 2019 19:36
@igdianov igdianov restored the upgrade-to-graphql-java-13 branch December 29, 2019 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants