Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
64 changes: 52 additions & 12 deletions composition-js/src/compose.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,30 @@ export interface CompositionFailure {
schema?: undefined;
supergraphSdl?: undefined;
hints?: undefined;
subgraphSdl?: { [subgraph: string]: string}
}

export interface CompositionSuccess {
schema: Schema;
supergraphSdl: string;
hints: CompositionHint[];
errors?: undefined;
subgraphSdl?: { [subgraph: string]: string}
}

export interface CompositionOptions {
sdlPrintOptions?: PrintOptions;
allowedFieldTypeMergingSubtypingRules?: SubtypingRule[];
/// Flag to toggle if satisfiability should be performed during composition
runSatisfiability?: boolean;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this would be a breaking change.... but ideally this should move under new pipelineOptions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It would be a breaking change to add an extra optional field to the return type?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if was thinking about moving this over but yeah if we also add extra option there then yeah that should be fine (and IMHO more consistent, maybe we could deprecate the old field as well)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, which old field do you mean?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

move runSatisfiability?: boolean; under new pipelineOptions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see my other comment though as I think this PR might not be that useful from hybrid composition perspective

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thought this PR would make it possible to support the future HybridComposition. Can federation-rs call composition with the piplineOptions to achieve the goals, no? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Kind of -> using pipeline options would enable us to skip first or last stages but will not allow pick and choose style of integrations.

i.e. given following stages

  pipelineOptions?: {
    runUpgradeSubgraphs?: boolean;
    runValidateSubgraphs?: boolean;
    runComposition?: boolean;
    runSatisfiability?: boolean;
    outputUpgradedSubgraphs?: boolean;
  }

How would you integrate it with hybrid composition if you only want to replace subgraph validations with Rust version? Or if you just want to compare single phases?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could move it, but I was thinking that runSatisfiability will be more long lived, but the other ones are really only for our use and will be temporary.


// note that pipeline options are temporary and will be removed in the future
pipelineOptions?: {
runUpgradeSubgraphs?: boolean;
runValidateSubgraphs?: boolean;
runComposition?: boolean;
outputUpgradedSubgraphs?: boolean;
}
}

function validateCompositionOptions(options: CompositionOptions) {
Expand All @@ -63,6 +73,24 @@ export function compose(subgraphs: Subgraphs, options: CompositionOptions = {}):
if (mergeResult.errors) {
return { errors: mergeResult.errors };
}

const subgraphSdl: { [name: string]: string } | undefined = !!options.pipelineOptions?.outputUpgradedSubgraphs ? {} : undefined;
if (subgraphSdl !== undefined && mergeResult.subgraphs) {
for (const subgraph of mergeResult.subgraphs.values()) {
subgraphSdl[subgraph.name] = printSchema(subgraph.schema, sdlPrintOptions ?? shallowOrderPrintedDefinitions(defaultPrintOptions));
}
}

// if we don't have a supergraph, we're using the `runComposition`=false option.
if (!mergeResult.supergraph) {
return {
schema: undefined,
supergraphSdl: undefined,
hints: undefined,
subgraphSdl,
errors: [],
}
}

let satisfiabilityResult;
if (runSatisfiability) {
Expand All @@ -84,11 +112,12 @@ export function compose(subgraphs: Subgraphs, options: CompositionOptions = {}):
} catch (err) {
return { errors: [err] };
}

return {
schema: mergeResult.supergraph,
supergraphSdl,
hints: [...mergeResult.hints, ...(satisfiabilityResult?.hints ?? [])],
subgraphSdl,
};
}

Expand Down Expand Up @@ -136,25 +165,36 @@ export function validateSatisfiability({ supergraphSchema, supergraphSdl} : Sati
return validateGraphComposition(supergraph.schema, supergraph.subgraphNameToGraphEnumValue(), supergraphQueryGraph, federatedQueryGraph);
}

type ValidateSubgraphsAndMergeResult = MergeResult | { errors: GraphQLError[] };
type ValidateSubgraphsAndMergeResult = MergeResult | { errors: GraphQLError[] } | { subgraphs: Subgraphs, errors?: undefined, supergraph?: undefined };

/**
* Upgrade subgraphs if necessary, then validates subgraphs before attempting to merge
*
* @param subgraphs
* @returns ValidateSubgraphsAndMergeResult
*/
function validateSubgraphsAndMerge(subgraphs: Subgraphs) : ValidateSubgraphsAndMergeResult {
const upgradeResult = upgradeSubgraphsIfNecessary(subgraphs);
if (upgradeResult.errors) {
return { errors: upgradeResult.errors };
function validateSubgraphsAndMerge(subgraphs: Subgraphs, options: CompositionOptions = {}) : ValidateSubgraphsAndMergeResult {
let toMerge: Subgraphs;

if (options.pipelineOptions?.runUpgradeSubgraphs !== false) {
const upgradeResult = upgradeSubgraphsIfNecessary(subgraphs);
if (upgradeResult.errors) {
return { errors: upgradeResult.errors };
}
toMerge = upgradeResult.subgraphs;
} else {
toMerge = subgraphs;
}

const toMerge = upgradeResult.subgraphs;
const validationErrors = toMerge.validate();
if (validationErrors) {
return { errors: validationErrors };
if (options.pipelineOptions?.runValidateSubgraphs !== false) {
const validationErrors = toMerge.validate();
if (validationErrors) {
return { errors: validationErrors };
}
}

return mergeSubgraphs(toMerge);

if (options.pipelineOptions?.runComposition !== false) {
return mergeSubgraphs(toMerge);
}
return { subgraphs: toMerge };
}
4 changes: 3 additions & 1 deletion composition-js/src/merging/merge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ export interface MergeSuccess {
supergraph: Schema;
hints: CompositionHint[];
errors?: undefined;
subgraphs?: Subgraphs,
}

export interface MergeFailure {
Expand Down Expand Up @@ -769,7 +770,8 @@ class Merger {
} else {
return {
supergraph: this.merged,
hints: this.hints
hints: this.hints,
subgraphs: this.subgraphs,
}
}
}
Expand Down