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

Document how to configure custom ExecutionStrategy #832

Closed
gli-chwy opened this issue Oct 2, 2023 · 6 comments
Closed

Document how to configure custom ExecutionStrategy #832

gli-chwy opened this issue Oct 2, 2023 · 6 comments
Assignees
Labels
type: documentation A documentation task
Milestone

Comments

@gli-chwy
Copy link

gli-chwy commented Oct 2, 2023

org.springframework.graphql.execution.ExceptionResolversExceptionHandler is package private, and it's not exposed as a Bean. Clearly it's designed that way.

I'm trying to extend graphql.execution.AsyncExecutionStrategy and override

@Override
protected CompletableFuture<ExecutionResult> completeValueForEnum(ExecutionContext executionContext, ExecutionStrategyParameters parameters, GraphQLEnumType enumType, Object result) 
{
}

to ignore unknown enum values in a class called IgnoreUnknownEnumValueExecutionStrategy. But the difficulty, without duplicating ExceptionResolversExceptionHandler, is I could not inject the ExceptionResolversExceptionHandler instance (which is hidden within the framework) into my IgnoreUnknownEnumValueExecutionStrategy bean.

Please advise if my use case and question is legitimate. Thanks.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 2, 2023
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Oct 13, 2023

I think this is the same case as #552. There we provided the static factory method DataFetcherExceptionResolver#createExceptionHandler to create the ExceptionResolversExceptionHandler instance.

We could try to take this a step further by facilitating the registration of execution strategies with the the DataFetcherExceptionHandler that we create. Something like this on GraphQlSource.Builder:

Builder configureExecutionStrategies(BiConsumer<GraphQL.Builder, DataFetcherExceptionHandler> configurer);

Or perhaps even a dedicated ExecutionStrategyFactory to create an ExecutionStrategy given a DataFetcherExceptionHandler. Although something like that could belong in GraphQL Java too.

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Oct 13, 2023
@spring-projects-issues
Copy link
Collaborator

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Oct 20, 2023
@gli-chwy
Copy link
Author

Thank you @rstoyanchev for your response. I'll update our version to take advantage of DataFetcherExceptionResolver#createExceptionHandler since it is currently at 1.0.3.
You can decide on the further refinement you mentioned.

@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 status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Oct 25, 2023
@rstoyanchev
Copy link
Contributor

I'll update the title to reflect the actual goal to make it easier to configure a custom ExecutionStrategy. We need to consider whether to make a change here, or to request an enhancement in GraphQL Java's GraphQL.Builder. I'll leave this open for the time being.

@rstoyanchev rstoyanchev changed the title Question on if ExceptionResolversExceptionHandler can be made available to application code Make it easier to configure ExecutionStrategy Oct 27, 2023
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Oct 27, 2023
@rstoyanchev rstoyanchev added this to the 1.3 Backlog milestone Oct 27, 2023
@rstoyanchev rstoyanchev self-assigned this Apr 18, 2024
@rstoyanchev rstoyanchev modified the milestones: 1.3 Backlog, 1.3.0 Apr 18, 2024
@rstoyanchev
Copy link
Contributor

rstoyanchev commented May 18, 2024

On further thought, a callback on GraphQlSource.Builder limits the ability to declare an ExecutionStrategy as a Spring bean while at the same time there isn't much of a drawback to create DataFetcherExceptionHandler via DataFetcherExceptionResolver#createExceptionHandler. It's mostly about making it easier to discover.

I'm turning this into a documentation issue. I'll update the Javadoc and reference docs to make it easy to find.

@rstoyanchev rstoyanchev added type: documentation A documentation task and removed type: enhancement A general enhancement labels May 18, 2024
@rstoyanchev rstoyanchev changed the title Make it easier to configure ExecutionStrategy Update documentation on how to configure ExecutionStrategy May 18, 2024
@rstoyanchev rstoyanchev changed the title Update documentation on how to configure ExecutionStrategy Document how to configure custom ExecutionStrategy May 18, 2024
@rstoyanchev rstoyanchev modified the milestones: 1.3.0, 1.2.7 May 18, 2024
Copy link
Contributor

Fixed via aa1ee77

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation A documentation task
Projects
None yet
Development

No branches or pull requests

3 participants