graphql / graphql-spec

GraphQL is a query language and execution engine tied to any backend service.
https://spec.graphql.org
14.31k stars 1.12k forks source link

Covariance of arguments not possible for interface implementations #629

Open rdstn opened 5 years ago

rdstn commented 5 years ago

In the spec as it stands, it does not appear to be possible to make variations on an interface's argument in its implementation, even if the subclass's arguments are functionally conformant to the interface's. Assume the following schema:

interface Object
    children (filter: Object_Input_Object): Object

type SubObject implements Object
    children (filter: SubObject_Input_Object): SubObject

In spite of the specific SubObject type being a correct implementation of the Object interface and their input objects being functionally the same, the schema is invalid since their arguments are not exactly the same.

This appears to be caused by the fact that there is no inheritance mechanism for input objects. The GraphQL type system has no way of knowing that the SubObject_Input_Object is a implementation of Object_Input_Object.

This would be useful, for example, in a situation such as this: I define a WorkOfArt interface which can be filtered based on date, author and price. Then I define a Sculpture implementation. I want Scuplture to be filtered on material and school, but also based on all the WorkOfArt filters. I do not want the WorkOfArt interface to be filterable based on these fields since the majority of this interface's implementation do not have them defined.

benjie commented 5 years ago

We're currently discussing input unions in the input union RFC; but it seems that a more generic solution to the problem you raise would be an input interface. Input interfaces are also something that has been discussed in this repo. I encourage you to send a small PR to the input union RFC with your use case (you might want to wait for https://github.com/graphql/graphql-spec/pull/626 to be merged first); it may or may not impact on what ultimately becomes a polymorphic input type. Separately, if you have a particular solution on mind, I recommend first reviewing the previous discussions and then, if your solution is significantly different, raising an RFC PR which shows how you see it working so it can be discussed technically.

vitramir commented 4 years ago

Polymorphic input types don't help us with situation when we want to keep filter argument on interface's field and remove it on implementation's field. For example the only possible filter is children type. Nothing to filter on implementation.

Another issue with polymorphic input type (regarding to the most prioritised solution at the moment https://github.com/graphql/graphql-spec/blob/master/rfcs/InputUnion.md#-2-explicit-configurable-discriminator-field) for this case is that they should be objects. This makes impossible to make different enum or scalar args. For example orderBy argument which should list fields from linked implementation only.

Why is this validation important? Why not to allow usage of any arguments on implementations?

benjie commented 4 years ago

Polymorphic input types don't help us with situation when we want to keep filter argument on interface's field and remove it on implementation's field. For example the only possible filter is children type. Nothing to filter on implementation.

I'm confused: if you remove a field from the implementation then surely the implementation no longer implements the interface, and thus you cannot supply the field on the interface any more?

for this case is that they should be objects. This makes impossible to make different enum or scalar args.

If you think that allowing enum/scalars in input unions is important I suggest you let your opinion be heard here: https://github.com/graphql/graphql-spec/pull/677#discussion_r370726872

vitramir commented 4 years ago
  1. @rdstn's example shows covariant argument. It is not type safe with up-casting operation. I think the below fragment is equivalent of such operation.
interface Animal{}
type Cat implements Animal{}
type Dog implements Animal{}

InputInterface AnimalWhereInput{}
input CatWhereInput implements AnimalWhereInput{}
input DogWhereInput implements AnimalWhereInput{}

interface Human{
   pets(where: AnimalWhereInput):[Animal]
}
type CatLover implements Human{
   pets(where: CatWhereInput): [Cat]
}
type DogLover implements Human{
   pets(where: DogWhereInput): [Dog]
}
type Query{
   people:[Human]
   catLovers:[CatLover]
   dogLovers:[DogLover]
}
{
   catLovers{
        ... on Human{
             pets(where:<DOG INPUT>)
        }
   }
}
  1. Contravariant argument may be less useful in real world. It is useful with multiple inheritance but also it is not type safe with up-casting.
interface Animal{}
type Cat implements Animal{}
type Dog implements Animal{}

input CatWhereInput{}
input DogWhereInput{}
input AnimalWhereInput implements CatWhereInput & DogWhereInput{}

interface Human{
   pets(where: AnimalWhereInput):[Animal]
}
type CatLover implements Human{
   pets(where: CatWhereInput): [Cat]
}
type DogLover implements Human{
   pets(where: DogWhereInput): [Dog]
}
type Query{
   people:[Human]
   catLovers:[CatLover]
   dogLovers:[DogLover]
}
  1. if you remove a field from the implementation

Remove an argument, not field.

interface Object
    children (filter: ObjectChildrenFilterInput): [ObjectChildren]

type SubObject implements Object
    children: [SubObjectChildren]

If interface's argument is not required it is safe to remove it from the implementation.

  1. If you think that allowing enum/scalars in input unions is important

It is important, but this issue is not about polymorphic input. The main question: should arguments be covariant, contravariant, bivariant? Or there should be no validation at all?

VladimirAlexiev commented 4 years ago

@vitramir: input CatWhereInput implements AnimalWhereInput is not allowed since there are no input interfaces. And therein lies the problem.

vitramir commented 4 years ago

@VladimirAlexiev Yes, there are no interfaces for inputs at the moment. But I believe the solution will be selected soon. (You can check the input union RFC for more details).

This issue is about "Covariance of arguments". I try to show some examples why polymorphic inputs (including input interfaces or unions) is not enough to close this issue and it’s important to continue the discussion.

VladimirAlexiev commented 4 years ago

@vitramir Sorry I missed that. Can you rub my nose into your argument "why polymorphic inputs are not enough"? I still can't see it (not enough coffee this morning)

vitramir commented 4 years ago

@VladimirAlexiev Availability of polymeric inputs doesn't automatically tell us how to change arguments validation.

1)At least we can change it to covariant, contravariant, bivariant. 2)Why is removing of optional argument on implementation restricted? Can we allow it? 3)I have tested custom graphql build without validation at all. And I got a working solution. Can we remove this validation at all?

May be I miss something and invariant is the only right solution. What is your opinion on this topic?