fix(stitch): handle isolated abstract types correctly #614
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request introduces several patches to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as GraphQL Request
participant CDS as calculateDelegationStage
participant ODM as optimizeDelegationMap
participant EUF as extractUnavailableFieldsFromSelectionSet
Client->>CDS: Initiates delegation stage calculation
CDS->>ODM: Call optimizeDelegationMap(delegationMap, typeName, fragments)
ODM->>EUF: Call extractUnavailableFieldsFromSelectionSet(selectionSet, fragments)
EUF-->>ODM: Return unavailable fields
ODM-->>CDS: Return optimized delegation map
CDS-->>Client: Return delegation plan
sequenceDiagram
participant ST as Schema Transformer
participant FIS as filterIsolatedSubschema
ST->>FIS: Pass subschemaConfig and isolatedSchemaTypes
FIS->>FIS: Evaluate fields using 'every' for isolation
FIS->>ST: Return filtered subschema configuration
ST-->>Client: Provide updated schema configuration
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)`packages/**`: In this directory we keep all packages releva...
`**`: For all PRs, we would like to verify that a Linear iss...
🧠 Learnings (1)packages/stitch/tests/createDelegationPlanBuilder.test.ts (1)🪛 Biome (1.9.4)packages/stitch/tests/createDelegationPlanBuilder.test.ts[error] 123-123: Forbidden non-null assertion. Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator (lint/style/noNonNullAssertion) ⏰ Context from checks skipped due to timeout of 90000ms (26)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-tools/batch-delegate |
9.0.30-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/batch-execute |
9.0.12-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/delegate |
10.2.12-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-common |
0.0.2-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-graphql-ws |
2.0.2-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-http |
1.2.7-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/federation |
3.1.2-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/fusion-runtime |
0.11.0-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
1.9.5-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/hmac-upstream-signature |
1.2.20-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-opentelemetry |
1.3.41-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
1.3.29-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
1.4.13-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/stitch |
9.4.17-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/stitching-directives |
3.1.27-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-common |
0.7.29-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http |
0.6.33-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http-callback |
0.5.20-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-ws |
1.0.3-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/wrap |
10.0.30-alpha-2be78396970d203b3f02c218e47936545ef66ba9 |
npm ↗︎ unpkg ↗︎ |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
|
@coderabbitai Could you review and help me writing unit tests? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 32349 🏁 Script executed: Length of output: 38799 @ardatan Here are the test cases we should add: // Add to packages/stitch/tests/createDelegationPlanBuilder.test.ts
describe('fragment handling in delegation optimization', () => {
const schema1 = makeExecutableSchema({
typeDefs: /* GraphQL */ `
type User {
id: ID!
name: String!
profile: Profile!
}
type Profile {
bio: String!
location: String!
}
type Query {
user(id: ID!): User
}
`,
});
const schema2 = makeExecutableSchema({
typeDefs: /* GraphQL */ `
type User {
id: ID!
name: String!
profile: Profile!
}
type Profile {
bio: String!
location: String!
email: String!
}
type Query {
user(id: ID!): User
}
`,
});
it('should optimize delegation map considering fragments', async () => {
const mergedTypeInfo: MergedTypeInfo = {
typeName: 'User',
targetSubschemas: new Map(),
uniqueFields: {},
nonUniqueFields: {
id: [schema1, schema2],
name: [schema1, schema2],
profile: [schema1, schema2],
},
typeMaps: new Map(),
};
const fragments = {
UserFields: {
kind: Kind.FRAGMENT_DEFINITION,
name: { kind: Kind.NAME, value: 'UserFields' },
typeCondition: {
kind: Kind.NAMED_TYPE,
name: { kind: Kind.NAME, value: 'User' },
},
selectionSet: {
kind: Kind.SELECTION_SET,
selections: [
{
kind: Kind.FIELD,
name: { kind: Kind.NAME, value: 'id' },
},
{
kind: Kind.FIELD,
name: { kind: Kind.NAME, value: 'name' },
},
],
},
},
};
const delegationMap = new Map();
delegationMap.set(schema1, {
kind: Kind.SELECTION_SET,
selections: [
{
kind: Kind.FRAGMENT_SPREAD,
name: { kind: Kind.NAME, value: 'UserFields' },
},
],
});
delegationMap.set(schema2, {
kind: Kind.SELECTION_SET,
selections: [
{
kind: Kind.FIELD,
name: { kind: Kind.NAME, value: 'profile' },
selectionSet: {
kind: Kind.SELECTION_SET,
selections: [
{
kind: Kind.FIELD,
name: { kind: Kind.NAME, value: 'email' },
},
],
},
},
],
});
optimizeDelegationMap(delegationMap, 'User', fragments);
// Schema2 should now contain both the profile field and the fields from the fragment
expect(delegationMap.size).toBe(1);
expect(delegationMap.has(schema2)).toBe(true);
const schema2Selections = delegationMap.get(schema2)!.selections;
expect(schema2Selections).toHaveLength(3); // id, name, profile
});
});
// Add to packages/stitch/tests/isolateComputedFieldsTransformer.test.ts
describe('abstract type isolation', () => {
it('should correctly handle computed fields in interfaces with multiple implementations', async () => {
const testSchema = makeExecutableSchema({
typeDefs: /* GraphQL */ `
interface Node {
id: ID!
computedField: String!
}
type User implements Node {
id: ID!
computedField: String!
name: String!
}
type Post implements Node {
id: ID!
computedField: String!
title: String!
}
type Query {
nodes: [Node]!
_nodes(representations: [ID!]!): [Node]!
}
`,
});
const [computedConfig, baseConfig] = isolateComputedFieldsTransformer({
schema: testSchema,
merge: {
Node: {
selectionSet: '{ id }',
fields: {
computedField: {
selectionSet: '{ internalId }',
computed: true,
},
},
fieldName: '_nodes',
key: ({ id }) => ({ id }),
argsFromKeys: (representations) => ({ representations }),
},
},
});
if (!computedConfig || !baseConfig) {
throw new Error('Expected both configs to be defined');
}
const baseSubschema = new Subschema(baseConfig);
const computedSubschema = new Subschema(computedConfig);
// Base schema should have id field in interface and implementations
expect(
Object.keys(
(baseSubschema.transformedSchema.getType('Node') as GraphQLInterfaceType).getFields(),
),
).toEqual(['id']);
expect(
Object.keys(
(baseSubschema.transformedSchema.getType('User') as GraphQLObjectType).getFields(),
),
).toEqual(['id', 'name']);
expect(
Object.keys(
(baseSubschema.transformedSchema.getType('Post') as GraphQLObjectType).getFields(),
),
).toEqual(['id', 'title']);
// Computed schema should have both fields in interface and implementations
expect(
Object.keys(
(computedSubschema.transformedSchema.getType('Node') as GraphQLInterfaceType).getFields(),
),
).toEqual(['id', 'computedField']);
expect(
Object.keys(
(computedSubschema.transformedSchema.getType('User') as GraphQLObjectType).getFields(),
),
).toEqual(['id', 'computedField', 'name']);
expect(
Object.keys(
(computedSubschema.transformedSchema.getType('Post') as GraphQLObjectType).getFields(),
),
).toEqual(['id', 'computedField', 'title']);
});
});These test cases cover:
Let me know if you'd like me to add more test cases or modify the existing ones. |
|
@coderabbitai Could you create a PR for your proposed tests with doc strings on the code etc? |
|
Tip For best results, initiate chat on the files or code changes. @ardatan I apologize, but I am not able to create PRs directly. However, I can provide you with the complete test code including detailed documentation that you can use to create the PR yourself: // packages/stitch/tests/createDelegationPlanBuilder.test.ts
/**
* Tests for fragment handling in delegation optimization.
* These tests verify that the delegation plan builder correctly handles GraphQL fragments
* when optimizing the delegation map across multiple subschemas.
*/
describe('fragment handling in delegation optimization', () => {
/**
* First test schema representing a basic User type with profile information.
* This schema lacks the email field in Profile to test partial field availability.
*/
const schema1 = makeExecutableSchema({
typeDefs: /* GraphQL */ `
type User {
id: ID!
name: String!
profile: Profile!
}
type Profile {
bio: String!
location: String!
}
type Query {
user(id: ID!): User
}
`,
});
/**
* Second test schema with an extended Profile type that includes an email field.
* Used to test fragment spreading across schemas with different capabilities.
*/
const schema2 = makeExecutableSchema({
typeDefs: /* GraphQL */ `
type User {
id: ID!
name: String!
profile: Profile!
}
type Profile {
bio: String!
location: String!
email: String!
}
type Query {
user(id: ID!): User
}
`,
});
/**
* Tests that the delegation map optimization correctly handles fragments
* by merging fragment fields into the appropriate subschema selections.
*/
it('should optimize delegation map considering fragments', async () => {
// Set up merged type info for the User type across both schemas
const mergedTypeInfo: MergedTypeInfo = {
typeName: 'User',
targetSubschemas: new Map(),
uniqueFields: {},
nonUniqueFields: {
id: [schema1, schema2],
name: [schema1, schema2],
profile: [schema1, schema2],
},
typeMaps: new Map(),
};
// Define a fragment that selects basic User fields
const fragments = {
UserFields: {
kind: Kind.FRAGMENT_DEFINITION,
name: { kind: Kind.NAME, value: 'UserFields' },
typeCondition: {
kind: Kind.NAMED_TYPE,
name: { kind: Kind.NAME, value: 'User' },
},
selectionSet: {
kind: Kind.SELECTION_SET,
selections: [
{
kind: Kind.FIELD,
name: { kind: Kind.NAME, value: 'id' },
},
{
kind: Kind.FIELD,
name: { kind: Kind.NAME, value: 'name' },
},
],
},
},
};
// Set up initial delegation map with fragment spread and profile selection
const delegationMap = new Map();
delegationMap.set(schema1, {
kind: Kind.SELECTION_SET,
selections: [
{
kind: Kind.FRAGMENT_SPREAD,
name: { kind: Kind.NAME, value: 'UserFields' },
},
],
});
delegationMap.set(schema2, {
kind: Kind.SELECTION_SET,
selections: [
{
kind: Kind.FIELD,
name: { kind: Kind.NAME, value: 'profile' },
selectionSet: {
kind: Kind.SELECTION_SET,
selections: [
{
kind: Kind.FIELD,
name: { kind: Kind.NAME, value: 'email' },
},
],
},
},
],
});
optimizeDelegationMap(delegationMap, 'User', fragments);
// Verify optimization results
expect(delegationMap.size).toBe(1);
expect(delegationMap.has(schema2)).toBe(true);
const schema2Selections = delegationMap.get(schema2)!.selections;
expect(schema2Selections).toHaveLength(3); // id, name, profile
});
});
// packages/stitch/tests/isolateComputedFieldsTransformer.test.ts
/**
* Tests for handling abstract types in schema isolation.
* These tests verify that computed fields are correctly handled when dealing with
* interfaces and their implementing types during schema isolation.
*/
describe('abstract type isolation', () => {
/**
* Tests the isolation of computed fields in interfaces with multiple implementations.
* Verifies that:
* 1. Interface fields are correctly distributed between base and computed schemas
* 2. Implementing types maintain correct field inheritance
* 3. Non-computed fields remain accessible in both schemas as needed
*/
it('should correctly handle computed fields in interfaces with multiple implementations', async () => {
const testSchema = makeExecutableSchema({
typeDefs: /* GraphQL */ `
interface Node {
id: ID!
computedField: String!
}
type User implements Node {
id: ID!
computedField: String!
name: String!
}
type Post implements Node {
id: ID!
computedField: String!
title: String!
}
type Query {
nodes: [Node]!
_nodes(representations: [ID!]!): [Node]!
}
`,
});
// Configure isolation transformer with computed field settings
const [computedConfig, baseConfig] = isolateComputedFieldsTransformer({
schema: testSchema,
merge: {
Node: {
selectionSet: '{ id }',
fields: {
computedField: {
selectionSet: '{ internalId }',
computed: true,
},
},
fieldName: '_nodes',
key: ({ id }) => ({ id }),
argsFromKeys: (representations) => ({ representations }),
},
},
});
if (!computedConfig || !baseConfig) {
throw new Error('Expected both configs to be defined');
}
const baseSubschema = new Subschema(baseConfig);
const computedSubschema = new Subschema(computedConfig);
// Verify base schema field distribution
expect(
Object.keys(
(baseSubschema.transformedSchema.getType('Node') as GraphQLInterfaceType).getFields(),
),
).toEqual(['id']);
expect(
Object.keys(
(baseSubschema.transformedSchema.getType('User') as GraphQLObjectType).getFields(),
),
).toEqual(['id', 'name']);
expect(
Object.keys(
(baseSubschema.transformedSchema.getType('Post') as GraphQLObjectType).getFields(),
),
).toEqual(['id', 'title']);
// Verify computed schema field distribution
expect(
Object.keys(
(computedSubschema.transformedSchema.getType('Node') as GraphQLInterfaceType).getFields(),
),
).toEqual(['id', 'computedField']);
expect(
Object.keys(
(computedSubschema.transformedSchema.getType('User') as GraphQLObjectType).getFields(),
),
).toEqual(['id', 'computedField', 'name']);
expect(
Object.keys(
(computedSubschema.transformedSchema.getType('Post') as GraphQLObjectType).getFields(),
),
).toEqual(['id', 'computedField', 'title']);
});
});I've added comprehensive doc strings to explain:
You can use this code to create a new PR. Would you like me to explain any part of the tests in more detail? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
packages/stitch/src/createDelegationPlanBuilder.ts(3 hunks)packages/stitch/tests/createDelegationPlanBuilder.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/stitch/src/createDelegationPlanBuilder.tspackages/stitch/tests/createDelegationPlanBuilder.test.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/stitch/src/createDelegationPlanBuilder.tspackages/stitch/tests/createDelegationPlanBuilder.test.ts
🪛 Biome (1.9.4)
packages/stitch/tests/createDelegationPlanBuilder.test.ts
[error] 123-123: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
⏰ Context from checks skipped due to timeout of 90000ms (26)
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Unit / Node v22
- GitHub Check: Snapshot / snapshot
- GitHub Check: Types
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Leaks / Node v23
- GitHub Check: Unit / Bun
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Leaks / Node v22
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Bundle
- GitHub Check: Build
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/stitch/tests/createDelegationPlanBuilder.test.ts (3)
7-11: Well-structured test documentation!The test suite is well-documented with clear explanations of its purpose and scope.
17-54: Good test schema design!The test schemas effectively model different capabilities (with and without email field) to test fragment handling across schemas.
63-143: Comprehensive test coverage for fragment optimization!The test thoroughly verifies:
- Fragment field merging
- Delegation map size reduction
- Selection set optimization
🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/stitch/src/createDelegationPlanBuilder.ts (2)
452-456: LGTM! Function signature updated correctly.The addition of the fragments parameter to optimizeDelegationMap is well-integrated and maintains the function's purpose.
462-469: Verify fragment resolution in extractUnavailableFieldsFromSelectionSet.The fragments parameter is correctly passed to extractUnavailableFieldsFromSelectionSet. However, let's verify that all fragment references are properly resolved.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any unresolved fragment references in the codebase # Search for fragment spreads rg -A 5 "Kind\.FRAGMENT_SPREAD" # Search for fragment definitions rg -A 5 "Kind\.FRAGMENT_DEFINITION"Length of output: 11319
Subject: Verified – Fragment resolution is correctly implemented in extractUnavailableFieldsFromSelectionSet
The grep results confirm the expected fragment lookup behavior. Specifically:
• In packages/delegate/src/extractUnavailableFields.ts, fragment spreads retrieve the fragment via fragments[selection.name.value] and proceed only if found, similar to the implementation in createDelegationPlanBuilder.ts.
• Test files (e.g., packages/stitch/tests/createDelegationPlanBuilder.test.ts) verify that fragments like "UserFields" are correctly defined and resolved.
Based on this evidence, fragment references are properly resolved.
3e33591 to
bb010bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.changeset/honest-terms-jump.md(1 hunks).changeset/tender-geese-perform.md(1 hunks)packages/stitch/src/createDelegationPlanBuilder.ts(3 hunks)packages/stitch/src/subschemaConfigTransforms/isolateComputedFieldsTransformer.ts(12 hunks)packages/stitch/tests/createDelegationPlanBuilder.test.ts(1 hunks)packages/stitch/tests/isolateComputedFieldsTransformer.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/stitch/tests/isolateComputedFieldsTransformer.test.tspackages/stitch/src/createDelegationPlanBuilder.tspackages/stitch/tests/createDelegationPlanBuilder.test.tspackages/stitch/src/subschemaConfigTransforms/isolateComputedFieldsTransformer.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/stitch/tests/isolateComputedFieldsTransformer.test.tspackages/stitch/src/createDelegationPlanBuilder.tspackages/stitch/tests/createDelegationPlanBuilder.test.tspackages/stitch/src/subschemaConfigTransforms/isolateComputedFieldsTransformer.ts
🧠 Learnings (1)
packages/stitch/tests/createDelegationPlanBuilder.test.ts (1)
Learnt from: ardatan
PR: graphql-hive/gateway#614
File: packages/stitch/tests/createDelegationPlanBuilder.test.ts:123-123
Timestamp: 2025-02-07T16:26:22.160Z
Learning: In the `createDelegationPlanBuilder.ts` file, the non-null assertion on `delegationMap.get(uniqueSubschema)?.selections` is safe as the selections are guaranteed to exist in this context.
🪛 Biome (1.9.4)
packages/stitch/tests/createDelegationPlanBuilder.test.ts
[error] 123-123: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
⏰ Context from checks skipped due to timeout of 90000ms (28)
- GitHub Check: Unit / Node v23
- GitHub Check: Unit / Node v22
- GitHub Check: Unit / Node v20
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: Unit / Bun
- GitHub Check: Unit / Node v18
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Snapshot / snapshot
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Build
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Bundle
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
packages/stitch/src/subschemaConfigTransforms/isolateComputedFieldsTransformer.ts (4)
85-85: Performance optimization: AddednoLocationoption to parseSelectionSet calls.The addition of
noLocation: trueoption optimizes memory usage by skipping location tracking during selection set parsing.Also applies to: 100-100, 173-173, 188-188
413-416: Improved function signature for better type safety.The function signature has been simplified by directly accepting
subschemaConfigandisolatedSchemaTypesinstead of using an intermediate type. This change improves type safety and makes the function's requirements more explicit.
563-566: Enhanced key field handling in isolated schemas.The implementation now properly checks for key fields in both the merge config and isolated schema types. This ensures that key fields are correctly preserved during schema isolation.
326-328: Verify the impact of changing field isolation logic.The logic for determining isolated fields has been changed from
sometoevery. This means a field will only be considered isolated if it is marked as isolated in all implementing types, rather than just some of them. While this change makes the isolation more strict and accurate, please verify that this doesn't break any existing use cases where partial isolation was intentional.Also applies to: 360-362
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for interface implementations that might be affected by this change ast-grep --pattern $'interface $interface { $$$ } type $type implements $interface { $$$ }'Length of output: 97
I'll run a new script to gather more context on the recent change. This new shell script will search in the transformer file for instances of both
.every(and.some(so that we can confirm the intended change and check if there’s any residual usage of the old logic.
🏁 Script executed:
#!/bin/bash echo "Finding occurrences of 'every(' in the transformer file..." rg "every\(" -n packages/stitch/src/subschemaConfigTransforms/isolateComputedFieldsTransformer.ts echo "Finding occurrences of 'some(' in the transformer file..." rg "some\(" -n packages/stitch/src/subschemaConfigTransforms/isolateComputedFieldsTransformer.tsLength of output: 1225
Impact of Changing from
.some()to.every()on Field Isolation
- Lines 326–328 and 360–362 now require that a field be marked isolated across all implementing types rather than any, making the isolation stricter.
- The use of
.some()remains for key fields, indicating that only the isolation check was intended to become more restrictive.- Our searches confirm the intended change in this transformer file. However, since no automated tests or interface usage evidence was found to explicitly validate broader use cases, it's important to manually verify that scenarios relying on partial isolation are not adversely affected.
packages/stitch/tests/isolateComputedFieldsTransformer.test.ts (3)
468-477: Test cleanup: Removed redundant interface field assertions.The commented-out assertions for interface fields align with the changes in the isolation logic. Since the interface field contents are now determined by the implementing types, these assertions are no longer relevant.
547-558: Test improvement: More flexible schema validation.The test now uses
toContaininstead of strict equality for schema validation, allowing for more flexibility in the schema output format while still ensuring the correct structure is maintained.
1-1: Add required PR documentation.Please ensure the following items are addressed as per our coding guidelines:
- Create a changeset file using
yarn changesetwith a "patch" bump since this is a bug fix- Add the Linear issue reference (GW-*) to the PR description
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check for changeset file if ! find .changeset -name "*.md" -type f -exec grep -l "patch" {} \; | grep -q .; then echo "No changeset file found with patch bump" exit 1 fiLength of output: 930
I'll re-run the verification using an alternative approach with
rgto avoid the SIGPIPE errors we encountered. Please run the following shell script:
🏁 Script executed:
#!/bin/bash # Re-verify the presence of a changeset file with a "patch" bump using rg echo "Searching for changeset files with a 'patch' bump in the .changeset directory:" if ! rg -iq "patch" -g "*.md" .changeset; then echo "No changeset file found with a patch bump." exit 1 else echo "A changeset file with a patch bump exists." fiLength of output: 303
PR Documentation: Ensure Required Linear Issue Reference is Added
The changeset file with the patch bump has been confirmed. However, please verify that the PR description includes the required Linear issue reference (GW-*). If it’s missing, kindly update the description accordingly.
packages/stitch/src/createDelegationPlanBuilder.ts (2)
214-214: Pass fragments to optimizeDelegationMap for improved coverage
This change ensures that the delegated selection sets incorporate fragment data, which is good. Everything looks fine.
468-468: Ensure consistent usage of fragments inextractUnavailableFieldsFromSelectionSet
Good job passing thefragmentsparameter here to maintain consistency with the new logic.packages/stitch/tests/createDelegationPlanBuilder.test.ts (2)
1-58: Comprehensive test setup
The import statements, schemas, and subschemas are well-defined. This approach correctly sets up multiple schemas for testing partial field availability.
123-123: Non-null assertion usage
We previously discussed this assertion. It's confirmed to be safe in this context.🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
.changeset/honest-terms-jump.md (2)
1-3: Changeset Header Format and Version BumpThe file header correctly uses the changeset delimiters and assigns a patch update to
@graphql-tools/stitch. This header format adheres to the standard changeset guidelines.
5-8: Clear Bug Fix Description for Abstract Type IsolationThe description clearly explains the fix: when isolating a field in an interface, the field should remain in the base subschema if it’s leveraged by other members. This description is concise and effectively communicates the underlying issue and resolution. Consider adding a brief reference to any associated test cases or further documentation in the release notes if needed for future maintainability.
bb010bd to
5edffec
Compare
No description provided.