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 @@
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
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
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
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
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
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
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
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
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
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 @@
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()) {

Check warning on line 79 in composition-js/src/compose.ts

View check run for this annotation

Codecov / codecov/patch

composition-js/src/compose.ts#L79

Added line #L79 was not covered by tests
subgraphSdl[subgraph.name] = printSchema(subgraph.schema, sdlPrintOptions ?? shallowOrderPrintedDefinitions(defaultPrintOptions));

Check warning on line 80 in composition-js/src/compose.ts

View check run for this annotation

Apollo SecOps / Static App Security Check

rules.providers.gitlab.security.eslint.detect-object-injection

Bracket object notation with user input is present, this might allow an attacker to access all properties of the object and even it's prototype, leading to possible code execution.
}
}

// if we don't have a supergraph, we're using the `runComposition`=false option.
if (!mergeResult.supergraph) {
return {

Check warning on line 86 in composition-js/src/compose.ts

View check run for this annotation

Codecov / codecov/patch

composition-js/src/compose.ts#L86

Added line #L86 was not covered by tests
schema: undefined,
supergraphSdl: undefined,
hints: undefined,
subgraphSdl,
errors: [],
}
}

let satisfiabilityResult;
if (runSatisfiability) {
Expand All @@ -84,11 +112,12 @@
} catch (err) {
return { errors: [err] };
}

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

Expand Down Expand Up @@ -136,25 +165,36 @@
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;

Check warning on line 186 in composition-js/src/compose.ts

View check run for this annotation

Codecov / codecov/patch

composition-js/src/compose.ts#L185-L186

Added lines #L185 - L186 were not covered by tests
}

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 };

Check warning on line 192 in composition-js/src/compose.ts

View check run for this annotation

Codecov / codecov/patch

composition-js/src/compose.ts#L192

Added line #L192 was not covered by tests
}
}

return mergeSubgraphs(toMerge);

if (options.pipelineOptions?.runComposition !== false) {
return mergeSubgraphs(toMerge);
}
return { subgraphs: toMerge };

Check warning on line 199 in composition-js/src/compose.ts

View check run for this annotation

Codecov / codecov/patch

composition-js/src/compose.ts#L199

Added line #L199 was not covered by tests
}
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