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

Improve how Querydsl and Query by Example auto registration fits in the hierarchy of DataFetcher registrations #244

Closed
dugenkui03 opened this issue Jan 3, 2022 · 6 comments
Assignees
Labels
in: data Issues related to working with data type: enhancement A general enhancement
Milestone

Comments

@dugenkui03
Copy link
Contributor

I think the goal of AutoRegistrationTypeVisitor#hasDataFetcher is that: determine whether the field bind customized DataFetcher, instead of default DataFetcher.

return (fetcher != null && !(fetcher instanceof PropertyDataFetcher));

But user can change the default DataFetcher of GraphQL Java by GraphQLCodeRegistry#defaultDataFetcherFactory. It will be more accurate to replace PropertyDataFetcher to DefaultDataFetcher.

@bclozel bclozel added the status: waiting-for-triage An issue we've not yet triaged label Jan 3, 2022
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 5, 2022

What is DefaultDataFetcher? I don't find such a type.

Keep in mind, we expect DataFetcher registrations to be made via RuntimeWiringConfigurer and likewise GraphQlSource.Builder supports a default TypeResolver registration, so I'd expect there to be no reason to set GraphQLCodeRegistry.

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Jan 5, 2022
@dugenkui03
Copy link
Contributor Author

Maybe I haven't make things clear. default DataFetcher is not a concrete Java Class: If schema field not register DataFetcher manully, the GraphQL Java will register default DataFetcher for this field.

Details in code sample, and I think change like that will avoid this error:

	private boolean hasDataFetcher(
			GraphQLCodeRegistry.Builder registry, GraphQLFieldsContainer parent,
			GraphQLFieldDefinition fieldDefinition) {

		DataFetcher<?> defaultDataFetcher = registry.getDefaultDataFetcherFactory().get(null);

		DataFetcher<?> fieldFetcher = registry.getDataFetcher(parent, fieldDefinition);
		return (fieldFetcher != null && !(defaultDataFetcher.getClass().isInstance(fieldFetcher)));
	}

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 12, 2022
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 13, 2022

Thanks for clarifying.

We've already run into some challenges with the way auto-registration works, related to extended validation (see #240). I'm going to experiment with replacing the AutoRegistrationTypeVisitor with a RuntimeWiringConfigurer that sets a defaultDataFetcherForType for each type that it needs to auto-register. This allows an application to override it through a regular registration but will also take precedence over a defaultDataFetcher as you have shown.

@rstoyanchev rstoyanchev changed the title The logic of AutoRegistrationTypeVisitor#hasDataFetcher Improve how Querydsl and Query by Example auto registration fits in the hierarchy of DataFetcher registrations Jan 13, 2022
@rstoyanchev rstoyanchev self-assigned this Jan 13, 2022
@rstoyanchev rstoyanchev added in: data Issues related to working with data and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Jan 13, 2022
@rstoyanchev rstoyanchev added this to the 1.0.0-M5 milestone Jan 13, 2022
rstoyanchev added a commit that referenced this issue Jan 14, 2022
CombinedWiringFactory makes use of multiple WiringFactory instances
possible, but RuntimeWiring.Builder does not use it by default.

This change enables use of multiple WiringFactory instances through
an extra callback on RuntimeWiringConfigurer that allows multiple
parties to add their own WiringFactory.

See gh-244
@rstoyanchev
Copy link
Contributor

This ended up a little different, in two changes. The first enables the ability to add any number of WiringFactory instances through an additional configure method on RuntimeWiringConfigurer. The second switches auto-registrating to use a WiringFactory instead of a TypeVisitor.

Note that we'll need a small follow-up change in the Boot starter before this works out of the box. So if you want to try it, please hold off until the Boot change is in place. I'll update here.

bclozel added a commit to spring-projects/spring-boot that referenced this issue Jan 14, 2022
rstoyanchev added a commit that referenced this issue Jan 17, 2022
Missed one deprecation and doc updates.

See gh-244
@rstoyanchev
Copy link
Contributor

@dugenkui03 the changes in the Boot starter are now also ready, if you'd like to give it a try with 1.0.0-SNAPSHOT and Boot 2.7.0-SNAPSHOT for the starter.

@rstoyanchev rstoyanchev added the type: enhancement A general enhancement label Jan 18, 2022
@dugenkui03
Copy link
Contributor Author

Thanks, the bad logic is still exist in AutoRegistrationTypeVisitor, and I make a pr for this #302.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues related to working with data type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants