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

Inline fragment from an interface to an interface that extends it is invalid #3949

Closed
u9g opened this issue Aug 11, 2023 · 1 comment
Closed

Comments

@u9g
Copy link

u9g commented Aug 11, 2023

Schema for more reference

So here's the problem. When putting our inline fragment for NumberLiteral in the value edge. [1] We are shown an error [2] stating the following:

Fragment cannot be spread here as objects of type "Expression" can never be of type "NumberLiteral".

This is a bug because NumberLiteral implements Expression, so it would be entirely correct to convert from an Expression to a NumberLiteral.

However, this check is implemented with this code, inside the InlineFragment function (as this is a syntax tree visitor I believe):

// <snip>
{
    InlineFragment(node) {
      const fragType = context.getType();
      const parentType = context.getParentType();
      if (
        isCompositeType(fragType) &&
        isCompositeType(parentType) &&
        !doTypesOverlap(context.getSchema(), fragType, parentType)
      ) {
        const parentTypeStr = inspect(parentType);
        const fragTypeStr = inspect(fragType);
        context.reportError(
          new GraphQLError(
            `Fragment cannot be spread here as objects of type "${parentTypeStr}" can never be of type "${fragTypeStr}".`,
            { nodes: node },
          ),
        );
      }
    },
}
// <snip>

in this function, doTypesOverlap(context.getSchema(), fragType, parentType) is doing the heavy lifting of whether NumberLiteral is a suitable subtype of Expression.

Let's take a look at doTypesOverlap()'s implemenation:

export function doTypesOverlap(
  schema: GraphQLSchema,
  typeA: GraphQLCompositeType,
  typeB: GraphQLCompositeType,
): boolean {
  // Equivalent types overlap
  if (typeA === typeB) {
    return true;
  }

  if (isAbstractType(typeA)) {
    if (isAbstractType(typeB)) {
      // If both types are abstract, then determine if there is any intersection
      // between possible concrete types of each.
      return schema
        .getPossibleTypes(typeA)
        .some((type) => schema.isSubType(typeB, type));
    }
    // Determine if the latter type is a possible concrete type of the former.
    return schema.isSubType(typeA, typeB);
  }

  if (isAbstractType(typeB)) {
    // Determine if the former type is a possible concrete type of the latter.
    return schema.isSubType(typeB, typeA);
  }

  // Otherwise the types do not overlap.
  return false;
}

To help not paste the entire codebase here, isAbstractType(X) returns true if X is an interface type or a union type, false otherwise.

For further context, NumberLiteral and Expression are both interfaces.

So we would have for if (isAbstractType(typeA)) { and then into the if (isAbstractType(typeB)) { path.

Which brings us to the schema.getPossibleTypes(typeA), so what is schema.getPossibleTypes()? Oh, and typeA will be fragType from the InlineFragment function which ends up being NumberLiteral in our case.

  getPossibleTypes(
    abstractType: GraphQLAbstractType,
  ): ReadonlyArray<GraphQLObjectType> {
    return isUnionType(abstractType)
      ? abstractType.getTypes()
      : this.getImplementations(abstractType).objects;
  }

Aha, so we just get the objects, which means the subtypes that are declared with the type keyword. Since NumberLiteral is an interface, we don't return it from this getPossibleTypes() function, so we never have the opportunity to check if NumberLiteral overlaps in the doTypesOverlap(). There is another property on the return type of this.getImplementations(abstractType) called implements which houses all the interfaces, including NumberLiteral, so really this getPossibleTypes() should be returning types and interfaces, when it only returns objects for now.

After writing all this, I read the comment here:

// If both types are abstract, then determine if there is any intersection
// between possible concrete types of each.
and I am thinking that maybe this behavior is correct, so if someone can confirm or deny this is a bug, I am willing to make a pull request to fix this.

Reference:

1

image

2

image

@yaacovCR
Copy link
Contributor

yaacovCR commented Oct 6, 2024

I think you have encountered this error because within the linked schema, the interface NumberLiteral does not actually have any concrete types which implement it, such that at runtime, there are no object types that intersect both NumberLiteral and Expression, the error message being technically correct.

We do not want to directly simply check to see if one interface implements the other, as we want to allow for a case in which they don't, but there are still concrete runtime types that implement both.

Pull requests improving the error reporting in this area are welcome, but I am closing this for now.

@yaacovCR yaacovCR closed this as completed Oct 6, 2024
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
@yaacovCR @u9g and others