Skip to content

Fixes composition issues with @interfaceObject #2318

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 10, 2023
Merged

Fixes composition issues with @interfaceObject #2318

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50676b6
Fixes composition issues with @interfaceObject
Jan 9, 2023
ef321a4
Simplify implem
Jan 9, 2023
ce94b76
changelogs
Jan 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 107 additions & 0 deletions composition-js/src/__tests__/compose.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3750,5 +3750,112 @@ describe('composition', () => {
'[subgraphB] Interface type "I" is defined as an @interfaceObject in subgraph "subgraphB" so that subgraph should not define any of the implementation types of "I", but it defines type "A"',
]]);
});

it('composes references to @interfaceObject', () => {
// Ensures that we have no issue merging a shared field whose is an interface in a subgraph, but an interfaceObject (so an object type)
// in another.
const subgraphA = {
typeDefs: gql`
type Query {
i: I @shareable
}

interface I @key(fields: "id") {
id: ID!
x: Int
}

type A implements I @key(fields: "id") {
id: ID!
x: Int
}

type B implements I @key(fields: "id") {
id: ID!
x: Int
}
`,
name: 'subgraphA',
};

const subgraphB = {
typeDefs: gql`
type Query {
i: I @shareable
}

type I @interfaceObject @key(fields: "id") {
id: ID!
y: Int
}
`,
name: 'subgraphB',
};

const result = composeAsFed2Subgraphs([subgraphA, subgraphB]);
assertCompositionSuccess(result);
});

it('do not error when optimizing unecessary loops', () => {
// This test is built so that reaching `t { i { ... on A { u { v } } } }` flows from `subgraphB` to `subgraphA` (for `t`),
// then changes types (with `i`), have a down cast to an implementation (`... A`) and then switch back subgraph (back to
// `subgraphB` for `u { v }`). The reason is that the underlying code will check for some optimisation in that case (more
// precisely, when switching back to `subgraphB` at the end, it will double-check if there wasn't a direct path in
// `subgraphA` achieving the same), and there was an early issue when `@interfaceObject` are involved for that optimization.
const subgraphA = {
typeDefs: gql`
type T @key(fields: "id") {
id: ID!
i: I
}

interface I @key(fields: "id") {
id: ID!
x: Int
}

type A implements I @key(fields: "id") {
id: ID!
x: Int
u: U
}

type B implements I @key(fields: "id") {
id: ID!
x: Int
}

type U @key(fields: "id") {
id: ID!
}
`,
name: 'subgraphA',
};

const subgraphB = {
typeDefs: gql`
type Query {
t: T
}

type T @key(fields: "id") {
id: ID!
}

type I @interfaceObject @key(fields: "id") {
id: ID!
}

type U @key(fields: "id") {
id: ID!
v: Int
}
`,
name: 'subgraphB',
};

const result = composeAsFed2Subgraphs([subgraphA, subgraphB]);
assertCompositionSuccess(result);
});
});
});
81 changes: 44 additions & 37 deletions composition-js/src/validate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ export class ValidationState {

const updatedState = new ValidationState(newPath, newSubgraphPaths);

// When handling a @shareable field, we also ensure compare the set of runtime types from each subgraph involves.
// When handling a @shareable field, we also compare the set of runtime types for each subgraphs involved.
// If there is no common intersection between those sets, then we record an error: a @shareable field should resolve
// the same way in all the subgraphs in which it is resolved, and there is no way this can be true if each subgraph
// returns runtime objects that we know can never be the same.
Expand All @@ -490,51 +490,58 @@ export class ValidationState {
// some runtime types intersection and the field resolvers only return objects of that intersection, then this
// could be a valid implementation. And this case can in particular happen temporarily as subgraphs evolve (potentially
// independently), but it is well worth warning in general.

// Note that we ignore any path when the type is not an abstract type, because in practice this means an @interfaceObject
// and this should not be considered as an implementation type. Besides @interfaceObject always "stand-in" for every
// implementations so they never are a problem for this check and can be ignored.
let hint: CompositionHint | undefined = undefined;
if (
newSubgraphPaths.length > 1
&& transition.kind === 'FieldCollection'
&& isAbstractType(newPath.tail.type)
&& context.isShareable(transition.definition)
) {
// We start our intersection by using all the supergraph types, both because it's a convenient "max" set to start our intersection,
// but also because that means we will ignore @inaccessible types in our checks (which is probably not very important because
// I believe the rules of @inacessible kind of exclude having some here, but if that ever change, it makes more sense this way).
const allRuntimeTypes = possibleRuntimeTypeNamesSorted(newPath);
let intersection = allRuntimeTypes;

const runtimeTypesToSubgraphs = new MultiMap<string, string>();
const runtimeTypesPerSubgraphs = new MultiMap<string, string>();
let hasAllEmpty = true;
for (const path of newSubgraphPaths) {
const subgraph = path.path.tail.source;
const typeNames = possibleRuntimeTypeNamesSorted(path.path);
runtimeTypesPerSubgraphs.set(subgraph, typeNames);
// Note: we're formatting the elements in `runtimeTYpesToSubgraphs` because we're going to use it if we display an error. This doesn't
// impact our set equality though since the formatting is consistent betweeen elements and type names syntax is sufficiently restricted
// in graphQL to not create issues (no quote or weird character to escape in particular).
let typeNamesStr = 'no runtime type is defined';
if (typeNames.length > 0) {
typeNamesStr = (typeNames.length > 1 ? 'types ' : 'type ') + joinStrings(typeNames.map((n) => `"${n}"`));
hasAllEmpty = false;
}
runtimeTypesToSubgraphs.add(typeNamesStr, subgraph);
intersection = intersection.filter((t) => typeNames.includes(t));
}

// If `hasAllEmpty`, then it means that none of the subgraph defines any runtime types. Typically, all subgraphs defines a given interface,
// but none have implementations. In that case, the intersection will be empty but it's actually fine (which is why we special case). In
// fact, assuming valid graphQL subgraph servers (and it's not the place to sniff for non-compliant subgraph servers), the only value to
// which each subgraph can resolve is `null` and so that essentially guaranttes that all subgraph do resolve the same way.
if (!hasAllEmpty) {
if (intersection.length === 0) {
return { error: shareableFieldNonIntersectingRuntimeTypesError(updatedState, transition.definition, runtimeTypesToSubgraphs) };
const filteredPaths = newSubgraphPaths.map((p) => p.path).filter((p) => isAbstractType(p.tail.type));
if (filteredPaths.length > 1) {
// We start our intersection by using all the supergraph types, both because it's a convenient "max" set to start our intersection,
// but also because that means we will ignore @inaccessible types in our checks (which is probably not very important because
// I believe the rules of @inacessible kind of exclude having some here, but if that ever change, it makes more sense this way).
const allRuntimeTypes = possibleRuntimeTypeNamesSorted(newPath);
let intersection = allRuntimeTypes;

const runtimeTypesToSubgraphs = new MultiMap<string, string>();
const runtimeTypesPerSubgraphs = new MultiMap<string, string>();
let hasAllEmpty = true;
for (const path of newSubgraphPaths) {
const subgraph = path.path.tail.source;
const typeNames = possibleRuntimeTypeNamesSorted(path.path);
runtimeTypesPerSubgraphs.set(subgraph, typeNames);
// Note: we're formatting the elements in `runtimeTYpesToSubgraphs` because we're going to use it if we display an error. This doesn't
// impact our set equality though since the formatting is consistent betweeen elements and type names syntax is sufficiently restricted
// in graphQL to not create issues (no quote or weird character to escape in particular).
let typeNamesStr = 'no runtime type is defined';
if (typeNames.length > 0) {
typeNamesStr = (typeNames.length > 1 ? 'types ' : 'type ') + joinStrings(typeNames.map((n) => `"${n}"`));
hasAllEmpty = false;
}
runtimeTypesToSubgraphs.add(typeNamesStr, subgraph);
intersection = intersection.filter((t) => typeNames.includes(t));
}

// As said, we accept it if there is an intersection, but if the runtime types are not all the same, we still emit a warning to make it clear that
// the fields should not resolve any of the types not in the intersection.
if (runtimeTypesToSubgraphs.size > 1) {
hint = shareableFieldMismatchedRuntimeTypesHint(updatedState, transition.definition, intersection, runtimeTypesPerSubgraphs);
// If `hasAllEmpty`, then it means that none of the subgraph defines any runtime types. Typically, all subgraphs defines a given interface,
// but none have implementations. In that case, the intersection will be empty but it's actually fine (which is why we special case). In
// fact, assuming valid graphQL subgraph servers (and it's not the place to sniff for non-compliant subgraph servers), the only value to
// which each subgraph can resolve is `null` and so that essentially guaranttes that all subgraph do resolve the same way.
if (!hasAllEmpty) {
if (intersection.length === 0) {
return { error: shareableFieldNonIntersectingRuntimeTypesError(updatedState, transition.definition, runtimeTypesToSubgraphs) };
}

// As said, we accept it if there is an intersection, but if the runtime types are not all the same, we still emit a warning to make it clear that
// the fields should not resolve any of the types not in the intersection.
if (runtimeTypesToSubgraphs.size > 1) {
hint = shareableFieldMismatchedRuntimeTypesHint(updatedState, transition.definition, intersection, runtimeTypesPerSubgraphs);
}
}
}
}
Expand Down
27 changes: 12 additions & 15 deletions internals-js/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,10 @@ import {
InterfaceType,
isInterfaceType,
isListType,
isNamedType,
isNonNullType,
isObjectType,
isUnionType,
ListType,
NamedType,
NonNullType,
ObjectType,
Type,
UnionType
Expand All @@ -32,26 +30,25 @@ export type SubtypingRule = typeof ALL_SUBTYPING_RULES[number];
export const DEFAULT_SUBTYPING_RULES = ALL_SUBTYPING_RULES.filter(r => r !== "list_upgrade");

/**
* Tests whether 2 types are the same type.
* Tests whether 2 types are the "same" type.
*
* To be the same type, for this method, is defined as having the same "kind"
* and either the same name, for named types or, for wrapper types, the same
* wrapped type (applying this method recursively).
* To be the same type, for this method, is defined as having the samee name for named types
* or, for wrapper types, the same wrapper type and recursively same wrapped one.
*
* This method does not check that both types are from the same schema and does
* not validate that the structure of named types is the same.
* This method does not check that both types are from the same schema and does not validate
* that the structure of named types is the same. Also note that it does not check the "kind"
* of the type, which is actually relied on due to @interfaceObject (where the "same" type
* can be an interface in one subgraph but an object type in another, while fundamentally being
* the same type).
*/
export function sameType(t1: Type, t2: Type): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The kind was previously just used as a short-circuit to not have to recurse, but this won't change any existing behavior in theory, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly didn't consider when writing this that comparing type of different kind could ever matter, so yes that was mostly a short-circuit to ensure the recursion worked. But yes, I reviewed the call site of this and I'm almost certain this won't change any behavior in an intended way (I mean, if we compare types from the same subgraph, this change makes no difference since a given name uniquely identify a type object in a given schema; so this only matter where comparing types from different schema where we check the kind of types between subgraph independently anyway, so this can't really allow things we don't want).

if (t1.kind !== t2.kind) {
return false;
}
switch (t1.kind) {
case 'ListType':
return sameType(t1.ofType, (t2 as ListType<any>).ofType);
return isListType(t2) && sameType(t1.ofType, t2.ofType);
case 'NonNullType':
return sameType(t1.ofType, (t2 as NonNullType<any>).ofType);
return isNonNullType(t2) && sameType(t1.ofType, t2.ofType);
default:
return t1.name === (t2 as NamedType).name;
return isNamedType(t2) && t1.name === t2.name;
}
}

Expand Down
36 changes: 33 additions & 3 deletions query-graphs-js/src/graphPath.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ type PathProps<TTrigger, RV extends Vertex = Vertex, TNullEdge extends null | ne
/** If the last edge (the one getting to tail) was a DownCast, the runtime types before that edge. */
readonly runtimeTypesBeforeTailIfLastIsCast?: readonly ObjectType[],

readonly lastIsInterfaceObjectFakeCastAfterNonCollecting: boolean,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Writing such a comment, it occurred to me that there was no need to add a new field, and that I could just reuse the subgraphEnteringEdge field we already have to implement, so update slightly. It saves the field but it's also probably a bit clearer because more explicit.


readonly deferOnTail?: DeferDirectiveArgs,
}

Expand Down Expand Up @@ -206,7 +208,8 @@ export class GraphPath<TTrigger, RV extends Vertex = Vertex, TNullEdge extends n
edgeConditions: [],
ownPathIds: [],
overriddingPathIds: [],
runtimeTypesOfTail: runtimeTypes
runtimeTypesOfTail: runtimeTypes,
lastIsInterfaceObjectFakeCastAfterNonCollecting: false,
});
}

Expand Down Expand Up @@ -283,6 +286,10 @@ export class GraphPath<TTrigger, RV extends Vertex = Vertex, TNullEdge extends n
return this.props.runtimeTypesOfTail;
}

lastIsIntefaceObjectFakeDownCastAfterNonCollecting(): boolean {
return this.props.lastIsInterfaceObjectFakeCastAfterNonCollecting;
}

/**
* Creates the new path corresponding to appending to this path the provided `edge`.
*
Expand Down Expand Up @@ -341,6 +348,7 @@ export class GraphPath<TTrigger, RV extends Vertex = Vertex, TNullEdge extends n
edgeConditions: withReplacedLastElement(this.props.edgeConditions, conditionsResolution.pathTree ?? null),
edgeToTail: updatedEdge,
runtimeTypesOfTail: runtimeTypesWithoutPreviousCast,
lastIsInterfaceObjectFakeCastAfterNonCollecting: false,
// We know the edge is a DownCast, so if there is no new `defer` taking precedence, we just inherit the
// prior version.
deferOnTail: defer ?? this.props.deferOnTail,
Expand Down Expand Up @@ -375,9 +383,11 @@ export class GraphPath<TTrigger, RV extends Vertex = Vertex, TNullEdge extends n
edgeTriggers: withReplacedLastElement(this.props.edgeTriggers, trigger),
edgeIndexes: withReplacedLastElement(this.props.edgeIndexes, edge.index),
edgeConditions: withReplacedLastElement(this.props.edgeConditions, conditionsResolution.pathTree ?? null),
subgraphEnteringEdge,
edgeToTail: edge,
runtimeTypesOfTail: updateRuntimeTypes(this.props.runtimeTypesOfTail, edge),
runtimeTypesBeforeTailIfLastIsCast: undefined, // we know last is not a cast
lastIsInterfaceObjectFakeCastAfterNonCollecting: false,
deferOnTail: defer,
});
}
Expand All @@ -394,6 +404,7 @@ export class GraphPath<TTrigger, RV extends Vertex = Vertex, TNullEdge extends n
edgeToTail: edge,
runtimeTypesOfTail: updateRuntimeTypes(this.props.runtimeTypesOfTail, edge),
runtimeTypesBeforeTailIfLastIsCast: edge?.transition?.kind === 'DownCast' ? this.props.runtimeTypesOfTail : undefined,
lastIsInterfaceObjectFakeCastAfterNonCollecting: edge?.transition.kind === 'InterfaceObjectFakeDownCast' && !!this.props.edgeToTail?.changesSubgraph(),
// If there is no new `defer` taking precedence, and the edge is downcast, then we inherit the prior version. This
// is because we only try to re-enter subgraphs for @defer on concrete fields, and so as long as we add downcasts,
// we should remember that we still need to try re-entering the subgraph.
Expand Down Expand Up @@ -436,7 +447,7 @@ export class GraphPath<TTrigger, RV extends Vertex = Vertex, TNullEdge extends n
});
}

checkDirectPathFomPreviousSubgraphTo(
checkDirectPathFromPreviousSubgraphTo(
typeName: string,
triggerToEdge: (graph: QueryGraph, vertex: Vertex, t: TTrigger) => Edge | null | undefined
): Vertex | undefined {
Expand Down Expand Up @@ -1116,6 +1127,25 @@ function advancePathWithNonCollectingAndTypePreservingTransitions<TTrigger, V ex
convertTransitionWithCondition: (transition: Transition, context: PathContext) => TTrigger,
triggerToEdge: (graph: QueryGraph, vertex: Vertex, t: TTrigger) => Edge | null | undefined
): IndirectPaths<TTrigger, V, TNullEdge, TDeadEnds> {
// If we're asked for indirect paths after an "@interfaceObject fake down cast" but that down cast comes just after a non-collecting edges, then
// we can ignore it (skip indirect paths from there). The reason is that the presence of the non-collecting just before the fake down-cast means
// we add looked at indirect paths just before that down cast, but that fake downcast really does nothing in practice with the subgraph it's on,
// so any indirect path from that fake down cast will have a valid indirect path _before_ it, and so will have been taken into account independently.
if (path.lastIsIntefaceObjectFakeDownCastAfterNonCollecting()) {
// Note: we need to register a dead-end for every subgraphs we "could" be going to, or the code calling this may try to infer a reason on its own
// and we'll run into some assertion.
const reachableSubgraphs = new Set(path.nextEdges().filter((e) => !e.transition.collectOperationElements && e.tail.source !== path.tail.source).map((e) => e.tail.source));
return {
paths: [],
deadEnds: new Unadvanceables(Array.from(reachableSubgraphs).map((s) => ({
sourceSubgraph: path.tail.source,
destSubgraph: s,
reason: UnadvanceableReason.IGNORED_INDIRECT_PATH,
details: `ignoring moving from "${path.tail.source}" to "${s}" as a more direct option exists`,
}))) as TDeadEnds,
};
}

const isTopLevelPath = path.isOnTopLevelQueryRoot();
const typeName = isFederatedGraphRootType(path.tail.type) ? undefined : path.tail.type.name;
const originalSource = path.tail.source;
Expand Down Expand Up @@ -1251,7 +1281,7 @@ function advancePathWithNonCollectingAndTypePreservingTransitions<TTrigger, V ex
// loop when calling `hasValidDirectKeyEdge` in that case without additional care and it's not useful because this
// very method already ensure we don't create unnecessary chains of keys for the "current type"
if (subgraphEnteringEdge && edge.transition.kind === 'KeyResolution' && subgraphEnteringEdge.edge.tail.type.name !== typeName) {
const prevSubgraphVertex = toAdvance.checkDirectPathFomPreviousSubgraphTo(edge.tail.type.name, triggerToEdge);
const prevSubgraphVertex = toAdvance.checkDirectPathFromPreviousSubgraphTo(edge.tail.type.name, triggerToEdge);
const backToPreviousSubgraph = subgraphEnteringEdge.edge.head.source === edge.tail.source;
const maxCost = toAdvance.subgraphEnteringEdge.cost + (backToPreviousSubgraph ? 0 : conditionResolution.cost);
if (prevSubgraphVertex
Expand Down
Loading