smallrye-graphql icon indicating copy to clipboard operation
smallrye-graphql copied to clipboard

Don't repeat `Input`

Open t1 opened this issue 4 years ago • 26 comments

If an input parameter type already ends with Input, e.g. SuperHeroInput, the generated schema shouldn't repeat it to become SuperHeroInputInput.

t1 avatar Aug 09 '20 05:08 t1

I agree with this. Is this in the spec though ? If not we might want to add that rule there too.

phillip-kruger avatar Aug 09 '20 12:08 phillip-kruger

I hadn't expected this to be ambiguous, but after re reading: yes, the spec should be clarified, too.

t1 avatar Aug 09 '20 15:08 t1

Just created https://github.com/eclipse/microprofile-graphql/issues/285

t1 avatar Aug 10 '20 04:08 t1

@t1 - Ok, So if this object is also use for an output, should we remove the (already there) Input ? If not we will have an error as both in- and output will have the same name ....

phillip-kruger avatar Aug 26 '20 11:08 phillip-kruger

I assume so, yes. If the application has only a SuperHeroInput class, it should either become SuperHero when used as an output, or it should be reported as an error... maybe the latter would be less confusing?

t1 avatar Oct 04 '20 16:10 t1

@phillip-kruger : Did you have time to think about this?

t1 avatar Oct 16 '20 09:10 t1

Maybe in this case, the user should just use @Name to correct the name ?

phillip-kruger avatar Oct 16 '20 09:10 phillip-kruger

Okay, that would work. It's just not really intuitive to write @Name("FooInput") class FooInput {...}, is it? OTOH it would be even stranger to have a class TeamInput turn into a return type Team.

So the rules would have to be:

  1. If a class is used as a return value, the GraphQL type is exactly the simple class name.
  2. If a class is used as parameter as well as a return type, the GraphQL type is exactly the simple class name and the GraphQL input is the simple class name plus Input.
  3. If a class is used only as a return type, and it ends with Input, the GraphQL input is exactly the simple class name.
  4. If a class is used only as a return type, and it doesn't end with Input, the GraphQL input is the simple class name plus Input.

The only downside I can see is this situation: you start with TeamInput as parameter only; so the schema contains an input TeamInput. Then you also return it somewhere; so the schema gets a type TeamInput, which is okay; but your input suddenly changes to input TeamInputInput.

BTW: I checked the GraphQL tutorial as well as the GraphQL spec: maybe I'm just blind, but I didn't see either to mention that an input and a type can't use the same name! So it should be possible to use TeamInput as a type as well as an input. But the graphql.schema.GraphQLTypeCollectingVisitor complains if I try. If this was true, this would be an issue for graphql-java, wouldn't it?

t1 avatar Oct 16 '20 13:10 t1

Why would you use ***Input as a classname ? We currently would control what is input and what is output via annotations on a normal class.

phillip-kruger avatar Oct 17 '20 17:10 phillip-kruger

My examples weren't clear enough, I guess.

FooInput was meant to be an example where the Foo class is used only for the output, and a different class FooInput for the input.

I used TeamInput as an example of a class where Input is not an indicator that this is an input type, but rather the input of a team. This can be output as well and TeamInputInput would make perfect sense.

t1 avatar Oct 18 '20 17:10 t1

Is this a duplicate of #650?

t1 avatar Mar 21 '21 12:03 t1

Just stumbled upon this.

I have a complex entity Foo:

@Entity
public class Foo {
    // ...
}

And a dedicated Input for it as a record:

public record FooInput(
    // ...
) {}

The Foo is nowhere used as input of @Mutation queries and yet a FooInput is generated for Foo and FooInputInput is generated for FooInput.

How do I disable generation of FooInput for Foo? How do I keep the input name of FooInput the same as the class/record? Shouldn't GraphQL be smart enough to detect that there already exist a FooInput type (detected in @Mutation queries) and skip the generation of the one for Foo?

Using GraphQL version associated with Quarkus 2.13.3.Final.

Primary reason for having dedicated input is mainly for updates, so that the user isn't forced to re-fill out all properties which are marked @NotNull in complex entity even though they are not modified. I.e. only the ones actually present in GraphQL arguments will be mapped. Another reason is being able to specify just the ID of the nested entity instead of the whole nested entity. Yet another reason is that the entity superclass can't necessarily be modified (e.g. 3rd party lib) to add @Ignore to e.g. id, version, created, lastModified properties because we absolutely don't want them to be updatable by GraphQL input.

BalusC avatar Dec 01 '22 09:12 BalusC

@phillip-kruger any thoughts on the above?

kito99 avatar Dec 01 '22 13:12 kito99

I'm not sure, but it's worth a try to annotate your FooInput as @Input("Foo").

The problem is that we can't just look at the type and not duplicate the Input suffix. E.g. a UserInput type could be the input for a User class, or it could be the input that a user has generated, so it's feasible to have a UserInputInput.

t1 avatar Dec 01 '22 13:12 t1

@t1 can you think of a reason why his non-input Foo would be generating a FooInput? Should that happen if it's not used in a @Mutation?

kito99 avatar Dec 01 '22 13:12 kito99

This has nothing to do with @Mutation vs. @Query. If it's a parameter (also in a nested method, BTW), then it's an Input. If it's being returned somewhere, it's (also) a Type.

t1 avatar Dec 01 '22 13:12 t1

If it's a parameter (also in a nested method, BTW), then it's an Input.

These entities are indeed used as @Source parameter in @Query methods. But that should still not make them a Input.

BalusC avatar Dec 01 '22 13:12 BalusC

@t1

I'm not sure, but it's worth a try to annotate your FooInput as @Input("Foo").

This throws the following exception:

Caused by: graphql.AssertException: All types within a GraphQL schema must have unique names. No two provided types may have the same name.
No provided type may have a name which conflicts with any built in types (including Scalar and Introspection types).
You have redefined the type 'Foo' from being a 'GraphQLObjectType' to a 'GraphQLInputObjectType'
	at graphql.schema.impl.GraphQLTypeCollectingVisitor.assertUniqueTypeObjects(GraphQLTypeCollectingVisitor.java:158)
	at graphql.schema.impl.GraphQLTypeCollectingVisitor.assertTypeUniqueness(GraphQLTypeCollectingVisitor.java:150)
	at graphql.schema.impl.GraphQLTypeCollectingVisitor.visitGraphQLInputObjectType(GraphQLTypeCollectingVisitor.java:71)
	at graphql.schema.impl.MultiReadOnlyGraphQLTypeVisitor.lambda$visitGraphQLInputObjectType$9(MultiReadOnlyGraphQLTypeVisitor.java:106)
	at java.base/java.util.Arrays$ArrayList.forEach(Arrays.java:4204)
	at graphql.schema.impl.MultiReadOnlyGraphQLTypeVisitor.visitGraphQLInputObjectType(MultiReadOnlyGraphQLTypeVisitor.java:106)
	at graphql.schema.GraphQLInputObjectType.accept(GraphQLInputObjectType.java:162)
	at graphql.schema.SchemaTraverser$TraverserDelegateVisitor.enter(SchemaTraverser.java:109)
	at graphql.util.Traverser.traverse(Traverser.java:144)
	at graphql.schema.SchemaTraverser.doTraverse(SchemaTraverser.java:96)
	at graphql.schema.SchemaTraverser.depthFirst(SchemaTraverser.java:86)
	at graphql.schema.SchemaTraverser.depthFirst(SchemaTraverser.java:79)
	at graphql.schema.impl.SchemaUtil.visitPartiallySchema(SchemaUtil.java:61)
	at graphql.schema.GraphQLSchema$Builder.buildImpl(GraphQLSchema.java:917)
	at graphql.schema.GraphQLSchema$Builder.build(GraphQLSchema.java:894)
	at io.smallrye.graphql.bootstrap.Bootstrap.generateGraphQLSchema(Bootstrap.java:186)

Interestingly it already considered Foo a GraphQLObjectType in the beginning?

BalusC avatar Dec 01 '22 13:12 BalusC

Sorry, my fault. It should be @Input("FooInput").

And you're right: using something as a @Source parameter shouldn't make it an Input.

t1 avatar Dec 01 '22 14:12 t1

Sorry, my fault. It should be @Input("FooInput").

I've tried this before. It throws the following exception:

Caused by: io.smallrye.graphql.schema.SchemaBuilderException: Classes com.example.model.input.FooInput and com.example.model.Foo map to the same GraphQL type 'FooInput', consider using the @Name annotation or a different naming strategy to distinguish between them
	at io.smallrye.graphql.schema.creator.ReferenceCreator.putIfAbsent(ReferenceCreator.java:472)
	at io.smallrye.graphql.schema.creator.ReferenceCreator.createReference(ReferenceCreator.java:276)
	at io.smallrye.graphql.schema.creator.ReferenceCreator.getReference(ReferenceCreator.java:372)
	at io.smallrye.graphql.schema.creator.ReferenceCreator.createReferenceForPojoField(ReferenceCreator.java:182)
	at io.smallrye.graphql.schema.creator.FieldCreator.createFieldForPojo(FieldCreator.java:93)
	at io.smallrye.graphql.schema.creator.type.InputTypeCreator.addFields(InputTypeCreator.java:158)
	at io.smallrye.graphql.schema.creator.type.InputTypeCreator.create(InputTypeCreator.java:71)
	at io.smallrye.graphql.schema.creator.type.InputTypeCreator.create(InputTypeCreator.java:36)
	at io.smallrye.graphql.schema.SchemaBuilder.createAndAddToSchema(SchemaBuilder.java:266)
	at io.smallrye.graphql.schema.SchemaBuilder.addTypesToSchema(SchemaBuilder.java:160)
	at io.smallrye.graphql.schema.SchemaBuilder.generateSchema(SchemaBuilder.java:125)
	at io.smallrye.graphql.schema.SchemaBuilder.build(SchemaBuilder.java:89)
	at io.quarkus.smallrye.graployment.SmallRyeGraphQLProcessor.buildExecutionService(SmallRyeGraphQLProcessor.java:267)

Moreover, ideally the schema.graphql should also NOT contain the generated input for Foo. Currently it contains both FooInput of Foo and FooInputInput of FooInput.

BalusC avatar Dec 01 '22 14:12 BalusC

It's difficult to investigate this without seeing the code. Do you happen to have a reproducer you can share?

t1 avatar Dec 01 '22 14:12 t1

Okay, I'll have to prep one first.

BalusC avatar Dec 01 '22 14:12 BalusC

^^

While prepping the reproducer I discovered that having a @Source is indeed the cause of observable issue. See the README at https://github.com/BalusC/smallrye-graphql-issue-360

BalusC avatar Dec 01 '22 15:12 BalusC

It's because the method with the Source parameter also contains a query annotation, so we will tread that is a query and a source. If you only want to attach that as a field, you can remove the query annotation and that should then not create the input

phillip-kruger avatar Dec 01 '22 20:12 phillip-kruger

That worked. Thank you very much :)

BalusC avatar Dec 01 '22 20:12 BalusC

Pleasure :)

phillip-kruger avatar Dec 01 '22 20:12 phillip-kruger