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

Cant use interface as input #1013

Closed
robp94 opened this issue Sep 1, 2021 · 16 comments
Closed

Cant use interface as input #1013

robp94 opened this issue Sep 1, 2021 · 16 comments

Comments

@robp94
Copy link
Contributor

robp94 commented Sep 1, 2021

When I use an interface as an input, I get an exception:

2021-09-01 16:43:53,480 ERROR [io.qua.dep.dev.IsolatedDevModeMain] (main) Failed to start quarkus: java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.smallrye.graphql.deployment.SmallRyeGraphQLProcessor#buildExecutionService threw an exception: java.lang.IllegalArgumentException: Class org.acme.Eatable is used as input, but does neither have a public default constructor nor a JsonbCreator method
...

code-with-quarkus-graphql.zip
#939

@phillip-kruger
Copy link
Member

Thanks @robp94 . I'll have a look a.s.a.p

@phillip-kruger
Copy link
Member

@robp94 - I did not forget about this... just been busy, I'll get to it soon.

@robp94
Copy link
Contributor Author

robp94 commented Sep 8, 2021

No problem, take your time.

@jmartisk
Copy link
Member

It seems the same issue arises when you try to use a Java 14 record as input.
But I'm not sure how to resolve this, because records are immutable and they can't have a no-arg constructor that does not fully initialize the instance.... Or am I getting something wrong?

@jmartisk
Copy link
Member

For records, we'll need a way to locate the suitable constructor with all fields as arguments, and build an instance using that (rather than use a no-arg constructor and then set each field one by one). I'm trying to do this for the client side now (when typesafe client consumes something that is a record, eg. it is output from the server side), - that actually falls rather under #971. Then if I am successful, I can try the same on server side as input.

@phillip-kruger
Copy link
Member

@jmartisk - your records fix would not have solved this, right ? Or would it ?

@jmartisk
Copy link
Member

jmartisk commented Sep 16, 2021

@jmartisk - your records fix would not have solved this, right ? Or would it ?

No.
This certainly needs more thinking and clarification.

  • How should we extract fields from an interface, if Java interfaces don't have fields? If the interface contains things that look like getters and setters, then we might use them as fields of the resulting GraphQL type. If there are other methods that don't look like a getter+setter pair, just ignore them?

  • The GraphQL library will need to create instances of something that implements the interface, so we will need a concrete class to exist. We could try to find a suitable one using the index, but what if there's none, or more than one? A solution could be to, instead of trying to find one between the application classes, generate one dynamically. With Quarkus and bytecode generation this should somehow be doable, but it is more difficult in other environments.

I'm not sure if it's worth the hassle - if usage of interfaces is really so important that we can't simply restrict to "concrete classes only".

@phillip-kruger
Copy link
Member

We can maybe find an implementation in the code ? And if it exist, use that, else throw an error ?

@jmartisk
Copy link
Member

What if there are multiple suitable implementations, we won't know which one to pick. Throw an error too?

If we require only one implementation of the interface to exist in the code, then that's somewhat against the intended usage of interfaces in Java. And people will get surprised that the application doesn't work just because they have two implementations of an interface.

@robp94
Copy link
Contributor Author

robp94 commented Sep 16, 2021

I would like to use interfaces like this:

interface Character {
  id: ID!
  name: String!
  friends: [Character]
  appearsIn: [Episode]!
}

type Human implements Character {
  id: ID!
  name: String!
  friends: [Character]
  appearsIn: [Episode]!
  starships: [Starship]
  totalCredits: Int
}

type Droid implements Character {
  id: ID!
  name: String!
  friends: [Character]
  appearsIn: [Episode]!
  primaryFunction: String
}

https://graphql.org/learn/schema/#interfaces they would be not dynamic, and all types would be known at build type.

@phillip-kruger
Copy link
Member

@jmartisk or pick the first one we find ?

@jmartisk
Copy link
Member

So if you have an operation like @Mutation void createCharacter(Character character)
your Java code would contain interface Character, class Human implements Character and class Droid implements Character
and somebody executes that mutation, how will the library know whether it should unmarshal it into a Droid or a Human?

Does the GraphQL spec allow interfaces as inputs at all? How should that look in the schema definition?
Input types should be prepended with the word "input", like

input ReviewInput {
  stars: Int!
  commentary: String
}

while interfaces should be prepended with "interface", it is legal at all to do input interface?

@phillip-kruger
Copy link
Member

Well, Input (and output) should be plain java object (or interfaces in this case) and should not contain logic. So it should not matter if you create Human or Droid, as your code only care about the interface. Right ? But agree, maybe we should check the spec...

@robp94
Copy link
Contributor Author

robp94 commented Sep 16, 2021

Hm seems like it actually is not supported https://spec.graphql.org/June2018/#sec-Interfaces Would be good if they would write it on their site and not only in the spec.
They plan something similar to union types for inputs. graphql/graphql-spec#733.

@phillip-kruger
Copy link
Member

https://download.eclipse.org/microprofile/microprofile-graphql-1.1.0/microprofile-graphql-spec-1.1.0.html#_limitations
I think we can close this ?

@jmartisk
Copy link
Member

Yeah agreed..

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

No branches or pull requests

3 participants