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

bug: @DgsEntityFetcher annotation does not prevent compilation when method arguments are incorrect #1618

Open
ghost opened this issue Aug 30, 2023 · 5 comments
Assignees
Labels
backlog enhancement New feature or request

Comments

@ghost
Copy link

ghost commented Aug 30, 2023

Expected behavior

WHEN an entity fetcher is defined as such:

@DgsEntityFetcher(name = DgsConstants.STORE.TYPE_NAME)
public Store storeFetcher(final DgsDataFetchingEnvironment dfe, final Map<String, Object> values) {
    return Store.newBuilder().name("myStore").build();
}

THEN compilation should fail because the parameters are in the wrong order

Actual behavior

Compilation is successful and the service starts. When making this query:

query {
  _entities(representations: [{__typename: "Store", id: "123"}]) {
    ... on Store {
      name
    }
  }
}

the returned error states Exception while fetching data (/_entities) : argument type mismatch. This is a misleading error that appears to come from GraphQL, but actually comes from Java.

Steps to reproduce

# schema
type Store @key(fields: "id") {
  id: ID!
  name: String
}
  1. Create an entityfetcher with the signature described above.
  2. Attempt to invoke it with the query described above.
@ghost ghost added the bug Something isn't working label Aug 30, 2023
@ghost ghost changed the title bug: DgsEntityFetcher does not prevent compilation when method arguments are incorrect bug: @DgsEntityFetcher annotation does not prevent compilation when method arguments are incorrect Sep 4, 2023
@kailyak
Copy link
Contributor

kailyak commented Sep 6, 2023

Hi @aogilvie-indeed thanks for the well documented issue. What version of dgs-framework are you using? On latest (version 7.5.1) with no custom exception handler I get this error message:

{
  "errors": [
    {
      "message": "java.lang.IllegalArgumentException: argument type mismatch",
      "locations": [],
      "path": [
        "_entities"
      ],
      "extensions": {
        "errorType": "INTERNAL"
      }
    }
  ],
  "data": {
    "_entities": [
      null
    ]
  }
}

You may be using a custom exception handler. The message on the latest version 7.5.1 with the default handler is not misleading and includes the java lang exception. Added a ticket internally for the compilation behavior.

@ghost
Copy link
Author

ghost commented Sep 12, 2023

Hi @kailyak !

I'm using 5.4.1. However, my error message is identical to yours.

My suggestion is that we catch this function signature problem at compile time and not at run time. Why build something that cannot be executed?

Also, it's not clear that "argument type mismatch" is a Java problem with the entityfetcher and not a GraphQL problem with the type of one of the arguments sent in the query.

@bcole
Copy link

bcole commented Oct 18, 2023

I just want to +1 this issue, as I also faced the same error output and couldn't figure out why. Thankfully I found this post to resolve my issue and swap argument order, but this issue is very valid and should be addressed in two ways as @aogilvie-indeed is mentioning:

  1. Documentation on Federation (here, here) does not mention anywhere how to inject the DataFetchingEnvironment for a DgsEntityFetcher. It is important to have access to the DataFetchingEnvironment, by the way, so that the Data Loader pattern can be used:

    In nearly every situation, reference resolvers should use a dataloader.
    Apollo docs
    ("reference resolver" is the Apollo term for DgsEntityFetcher)

    So on this point, we should enhance documentation to show how a DataFetchingEnvironment can be injected to the DgsEntityFetcher (as this post points out, only allowed as the second argument, fails if injected as the first argument).

  2. Upon simply trying to add the DataFetchingEnvironment argument as the first argument, compilation succeeds and runtime fails with the ambiguous error as shared above.

    So on this point, we should fail at compile time (with a clear error message) since the method argument order is incorrect.

@n614cd
Copy link

n614cd commented Jan 8, 2024

The OP Title is correct. The java method param names, and the schema names do not generate errors when they do not match.
If compile time validation is not possible (I am not sure it is, based on the quick look at the code), it should be done as part of the runtime wiring and through an exception.

Tim

@paulbakker
Copy link
Collaborator

It's not possible to implement this kind of check compile time with Java, but we could at least add a hint in the Intellij plugin. Also we could verify this at startup.

@paulbakker paulbakker added enhancement New feature or request backlog and removed bug Something isn't working labels Oct 17, 2024
@paulbakker paulbakker self-assigned this Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants