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

ConnectionDataFetcher should handle data wrapped with DataFetcherResult #835

Closed
Empatixx opened this issue Oct 9, 2023 · 1 comment
Closed
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@Empatixx
Copy link

Empatixx commented Oct 9, 2023

Hello,

First and foremost, I'd like to express my gratitude for this incredible library. I'm reaching out to report a bug related to the ConnectionAdapter and the results from DataFetcher as represented by DataFetcherResult.

When I retrieve a result from DataFetcher that's an instance of DataFetcherResult with a specific local context and associated data that has its specific ConnectionAdapter, an error is generated. The system seems unable to locate the adapter. It appears to be looking for an adapter for the overall result rather than the specific data type of DataFetcherResult. Currently, there isn't an alternative method to pass the local context and the corresponding data that should be Connection and mapped by the ConnectionAdapter. This leads me to believe that there's a bug in the process.

I've managed to identify a potential solution...

In ConnectionFieldTypeVisitor in record ConnectionDataFetcher

        @Override
        public Object get(DataFetchingEnvironment environment) throws Exception {
            Object result = this.delegate.get(environment);
            if (result instanceof DataFetcherResult dataFetcherResult){
                Object data = dataFetcherResult.getData();
                return DataFetcherResult.newResult()
                        .data(map(data))
                        .localContext(dataFetcherResult.getLocalContext())
                        .errors(dataFetcherResult.getErrors())
                        .build();
            }

            return map(result);
        }

        private Object map(Object result) {
            if (result instanceof Mono<?> mono) {
                return mono.map(this::adapt);
            }
            else if (result instanceof CompletionStage<?> stage) {
                return stage.thenApply(this::adapt);
            }
            else {
                return adapt(result);
            }
        }

I think it should check if result is DataFetcherResult and map only its data.
My controller is like this:

    @SchemaMapping(typeName = "Parent")
    public DataFetcherResult<Connection<Item>> items(final Parent parent,
                                                final OffsetPagination pagination) {
        final List<Item> items = service.getItems(parent.getId(), pagination.offset(), pagination.limit());
        final Connection<Item> connection = Connection.create(items, pagination);
        return DataFetcherResult.<Connection<Item>>newResult()
                .localContext(GraphQLContext.newContext()
                        .of(LocalContextValues.PARENT, parent.id())
                        .build())
                .data(connection)
                .build();  
}

The Connection class functions correctly with its ConnectionAdapter. When returning entire Connection, everything operates as expected. However, when there's a need to pass a localContext, I utilize DataFetcherResult. At this point, an issue arises because it cannot handle this specific type.

type Query {
    parent: Parent!
}
type Parent {
    id: ID!
    items(after: String, before: String, first: NonNegativeInt, last: NonNegativeInt): ItemConnection!
}
type Item{
    id: ID!
}

If needed more code or more information, tell me. Thank you for library!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 9, 2023
@rstoyanchev rstoyanchev changed the title ConnectionAdapter - DataFetcherResult ConnectionDataFetcher should handle data wrapped with DataFetcherResult Oct 13, 2023
@rstoyanchev rstoyanchev self-assigned this Oct 13, 2023
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 13, 2023
@rstoyanchev rstoyanchev added this to the 1.2.4 milestone Oct 13, 2023
@rstoyanchev
Copy link
Contributor

Thank you for the report. It makes sense to do the right thing when the data is wrapped with DataFetcherResult.

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

No branches or pull requests

3 participants