show print not working #7786
show print not working #7786
Conversation
|
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds/updates tests that exercise printing and merging of schemas with duplicate and repeatable directives, and adds a dev dependency for Apollo subgraph to support those tests. No production code changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/utils/tests/print-schema-with-directives.spec.ts (2)
406-416: Simplify the test schema and align formatting with existing tests.The
extend schemablock with the@linkdirective (lines 407-413) doesn't appear directly related to testing duplicate directives on a scalar. If it's required for the@tagdirective to function, please add a comment explaining this dependency. Otherwise, consider simplifying the schema to focus only on the duplicate directives:it('should allow duplicate directives', () => { - const typeDefs = ` - extend schema - @link( - url: "https://specs.apollo.dev/federation/v2.5" - import: [ - "@tag" - ] - ) - - scalar DateTimeISO @tag(name: "nameA") @tag(name: "nameB") - `; + const typeDefs = /* GraphQL */ ` + directive @tag(name: String!) repeatable on SCALAR + + scalar DateTimeISO @tag(name: "nameA") @tag(name: "nameB") + `;Additionally, using the
/* GraphQL */comment tag aligns with the formatting style used in other tests (e.g., lines 25, 54, 286).
422-422: Add a newline at the end of the file.The test block should be followed by a blank line before the closing brace of the
describeblock, consistent with the formatting of other tests in this file.- }) + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/utils/tests/print-schema-with-directives.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/utils/tests/print-schema-with-directives.spec.ts (1)
packages/utils/src/print-schema-with-directives.ts (1)
printSchemaWithDirectives(119-125)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Bun
| expect(printedTransformedSchema).toContain('nameA'); | ||
| expect(printedTransformedSchema).toContain('nameB'); |
There was a problem hiding this comment.
Strengthen assertions to verify complete directive syntax.
The current assertions only check for the presence of "nameA" and "nameB" strings, which could produce false positives if these values appear elsewhere in the schema (e.g., in type names, field names, or other directive arguments).
Consider verifying the full directive syntax to ensure both @tag directives are correctly printed:
const printedTransformedSchema = printSchemaWithDirectives(schema);
- expect(printedTransformedSchema).toContain('nameA');
- expect(printedTransformedSchema).toContain('nameB');
+ expect(printedTransformedSchema).toContain('@tag(name: "nameA")');
+ expect(printedTransformedSchema).toContain('@tag(name: "nameB")');
+ // Optionally verify both appear on the same scalar
+ expect(printedTransformedSchema).toMatch(
+ /scalar DateTimeISO[^\n]*@tag\(name: "nameA"\)[^\n]*@tag\(name: "nameB"\)/
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(printedTransformedSchema).toContain('nameA'); | |
| expect(printedTransformedSchema).toContain('nameB'); | |
| expect(printedTransformedSchema).toContain('@tag(name: "nameA")'); | |
| expect(printedTransformedSchema).toContain('@tag(name: "nameB")'); | |
| // Optionally verify both appear on the same scalar | |
| expect(printedTransformedSchema).toMatch( | |
| /scalar DateTimeISO[^\n]*@tag\(name: "nameA"\)[^\n]*@tag\(name: "nameB"\)/ | |
| ); |
🤖 Prompt for AI Agents
In packages/utils/tests/print-schema-with-directives.spec.ts around lines 419 to
420, the two assertions only check for the bare strings "nameA" and "nameB"
which can yield false positives; update them to assert the full directive syntax
(e.g., check for the exact printed directive tokens such as '@tag(name:
"nameA")' and '@tag(name: "nameB")' or use a regex that matches the directive
and argument structure) so the test verifies the directives themselves rather
than just the substring.
|
Could you give more context about the PR? |
|
i'm suspecting the bug is from buildSubgraphSchema at the moment. i'll reopen if i discover more |
|
ok I think i got to the root of the bug, but I'm not entirely clear on what the correct workaround or fix should be i'm using the repeatable federation directive and I have a conflicting set of scalars (in this case DateTimeISO) In one schema (which is generated by typeGraphQL) the scalar has no directives i.e. Because the scalars conflict, you can't just pass both schemas into buildSubgraphSchema, you need to use mergeTypeDefs first and here's the rub, because the fed 2 definitions haven't been unpacked yet. (which is done by buildSubgraphSchema) I can work around this by adding the unpacked definition of tag manually, but should that definition quietly change in some future update to fed 2 that could get a bit weird. It might be a "won't fix" bug, but at least we know the cause. |
|
pushed an updated which hopefully demonstrates the issue. As i mentioned i don't think it's expressly a graphql-tools bug, but it's an interesting chicken, egg scenario |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(1 hunks)packages/merge/tests/merge-typedefs.spec.ts(2 hunks)packages/utils/tests/print-schema-with-directives.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/merge/tests/merge-typedefs.spec.ts (2)
packages/merge/src/typedefs-mergers/merge-typedefs.ts (1)
mergeTypeDefs(118-146)packages/utils/src/print-schema-with-directives.ts (1)
printSchemaWithDirectives(119-125)
🪛 GitHub Check: Type Check on GraphQL v15
packages/utils/tests/print-schema-with-directives.spec.ts
[failure] 20-20:
'mergeTypeDefs' is declared but its value is never read.
[failure] 19-19:
'buildSubgraphSchema' is declared but its value is never read.
[failure] 18-18:
'gql' is declared but its value is never read.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Bun
- GitHub Check: Full Check on GraphQL v16
🔇 Additional comments (1)
package.json (1)
53-53: The @apollo/subgraph version is current and appropriate.Version 2.12.2 is the latest stable release (released December 8, 2025), and the caret constraint (^2.12.2) is correctly specified for the dev dependency.
| it('should allow duplicate directives', () => { | ||
| const schemaWithFedDeclaration = gql` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.5" | ||
| import: ["@key", "@shareable", "@tag"] | ||
| ) | ||
| scalar DateTimeISO @tag(name: "nameA") @tag(name: "nameB") | ||
| `; | ||
| const schemaWithConflictingScalar = gql` | ||
| scalar DateTimeISO | ||
| `; | ||
|
|
||
| const schemaWithExpandedFederatedTagDirective = gql` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.5" | ||
| import: ["@key", "@shareable", "@tag"] | ||
| ) | ||
| directive @tag( | ||
| name: String! | ||
| ) repeatable on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION | SCHEMA | ||
| scalar DateTimeISO @tag(name: "nameA") @tag(name: "nameB") | ||
| `; | ||
|
|
||
| const schemaWithoutExpandedTagDirective = buildSubgraphSchema({ | ||
| typeDefs: mergeTypeDefs([schemaWithFedDeclaration, schemaWithConflictingScalar]), | ||
| }); | ||
| const schemaWithExpandedTagDirective = buildSubgraphSchema({ | ||
| typeDefs: mergeTypeDefs([ | ||
| schemaWithExpandedFederatedTagDirective, | ||
| schemaWithConflictingScalar, | ||
| ]), | ||
| }); | ||
|
|
||
| const problematicSchema = printSchemaWithDirectives(schemaWithoutExpandedTagDirective); | ||
| const successfulSchema = printSchemaWithDirectives(schemaWithExpandedTagDirective); | ||
|
|
||
| expect(successfulSchema).toContain('nameA'); | ||
| expect(successfulSchema).toContain('nameB'); | ||
| console.log('firstSchemaPasses'); | ||
|
|
||
| expect(problematicSchema).toContain('nameB'); | ||
| // should fail, as only the last directive was kept by mergeTypeDefs | ||
| console.log( | ||
| 'test will fail, nameA not found, due to mergeTypeDefs stripping the first tag directive', | ||
| ); | ||
| expect(problematicSchema).toContain('nameA'); | ||
| }); |
There was a problem hiding this comment.
Fix test implementation issues.
This test has several critical problems that will cause CI failures and code quality issues:
- Console.log statements (lines 1893, 1897-1899): Debug logging should not be committed to test code.
- Intentionally failing assertion (line 1900): The test expects
'nameA'to be present when comments explicitly state it won't be found, causing the test to fail. - Unclear test intent: The test name "should allow duplicate directives" suggests testing desired behavior, but the implementation demonstrates a bug.
If this test is meant to document a known bug:
- Use
.skipto prevent CI failures:it.skip('should allow duplicate directives', () => { ... }) - Or convert to a
.todowith a description:it.todo('should preserve duplicate repeatable directives during merge') - Add a clear comment referencing the issue/bug being demonstrated
If this test is meant to verify a fix:
- Remove all
console.logstatements - Remove or fix the failing assertion on line 1900
- Clarify expected vs actual behavior
Apply this diff to remove debug logging:
expect(successfulSchema).toContain('nameA');
expect(successfulSchema).toContain('nameB');
- console.log('firstSchemaPasses');
expect(problematicSchema).toContain('nameB');
- // should fail, as only the last directive was kept by mergeTypeDefs
- console.log(
- 'test will fail, nameA not found, due to mergeTypeDefs stripping the first tag directive',
- );
- expect(problematicSchema).toContain('nameA');
+ // Known issue: mergeTypeDefs strips the first @tag directive because it's not aware
+ // that @tag is repeatable until buildSubgraphSchema unpacks federation definitions
+ expect(problematicSchema).not.toContain('nameA');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should allow duplicate directives', () => { | |
| const schemaWithFedDeclaration = gql` | |
| extend schema | |
| @link( | |
| url: "https://specs.apollo.dev/federation/v2.5" | |
| import: ["@key", "@shareable", "@tag"] | |
| ) | |
| scalar DateTimeISO @tag(name: "nameA") @tag(name: "nameB") | |
| `; | |
| const schemaWithConflictingScalar = gql` | |
| scalar DateTimeISO | |
| `; | |
| const schemaWithExpandedFederatedTagDirective = gql` | |
| extend schema | |
| @link( | |
| url: "https://specs.apollo.dev/federation/v2.5" | |
| import: ["@key", "@shareable", "@tag"] | |
| ) | |
| directive @tag( | |
| name: String! | |
| ) repeatable on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION | SCHEMA | |
| scalar DateTimeISO @tag(name: "nameA") @tag(name: "nameB") | |
| `; | |
| const schemaWithoutExpandedTagDirective = buildSubgraphSchema({ | |
| typeDefs: mergeTypeDefs([schemaWithFedDeclaration, schemaWithConflictingScalar]), | |
| }); | |
| const schemaWithExpandedTagDirective = buildSubgraphSchema({ | |
| typeDefs: mergeTypeDefs([ | |
| schemaWithExpandedFederatedTagDirective, | |
| schemaWithConflictingScalar, | |
| ]), | |
| }); | |
| const problematicSchema = printSchemaWithDirectives(schemaWithoutExpandedTagDirective); | |
| const successfulSchema = printSchemaWithDirectives(schemaWithExpandedTagDirective); | |
| expect(successfulSchema).toContain('nameA'); | |
| expect(successfulSchema).toContain('nameB'); | |
| console.log('firstSchemaPasses'); | |
| expect(problematicSchema).toContain('nameB'); | |
| // should fail, as only the last directive was kept by mergeTypeDefs | |
| console.log( | |
| 'test will fail, nameA not found, due to mergeTypeDefs stripping the first tag directive', | |
| ); | |
| expect(problematicSchema).toContain('nameA'); | |
| }); | |
| it('should allow duplicate directives', () => { | |
| const schemaWithFedDeclaration = gql` | |
| extend schema | |
| @link( | |
| url: "https://specs.apollo.dev/federation/v2.5" | |
| import: ["@key", "@shareable", "@tag"] | |
| ) | |
| scalar DateTimeISO @tag(name: "nameA") @tag(name: "nameB") | |
| `; | |
| const schemaWithConflictingScalar = gql` | |
| scalar DateTimeISO | |
| `; | |
| const schemaWithExpandedFederatedTagDirective = gql` | |
| extend schema | |
| @link( | |
| url: "https://specs.apollo.dev/federation/v2.5" | |
| import: ["@key", "@shareable", "@tag"] | |
| ) | |
| directive @tag( | |
| name: String! | |
| ) repeatable on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION | SCHEMA | |
| scalar DateTimeISO @tag(name: "nameA") @tag(name: "nameB") | |
| `; | |
| const schemaWithoutExpandedTagDirective = buildSubgraphSchema({ | |
| typeDefs: mergeTypeDefs([schemaWithFedDeclaration, schemaWithConflictingScalar]), | |
| }); | |
| const schemaWithExpandedTagDirective = buildSubgraphSchema({ | |
| typeDefs: mergeTypeDefs([ | |
| schemaWithExpandedFederatedTagDirective, | |
| schemaWithConflictingScalar, | |
| ]), | |
| }); | |
| const problematicSchema = printSchemaWithDirectives(schemaWithoutExpandedTagDirective); | |
| const successfulSchema = printSchemaWithDirectives(schemaWithExpandedTagDirective); | |
| expect(successfulSchema).toContain('nameA'); | |
| expect(successfulSchema).toContain('nameB'); | |
| expect(problematicSchema).toContain('nameB'); | |
| // Known issue: mergeTypeDefs strips the first @tag directive because it's not aware | |
| // that @tag is repeatable until buildSubgraphSchema unpacks federation definitions | |
| expect(problematicSchema).not.toContain('nameA'); | |
| }); |
🤖 Prompt for AI Agents
In packages/merge/tests/merge-typedefs.spec.ts around lines 1851 to 1901 the
test contains stray console.log calls (lines ~1893 and ~1897-1899), an
intentionally failing assertion (line ~1900) and unclear intent; remove all
console.log debug statements, and either mark the test as demonstrating a known
bug (change it(...) to it.skip(...) or it.todo(...) and add a one-line comment
referencing the tracking issue) or, if the test should verify correct behavior
after a fix, remove the failing assertion and update expectations to match the
intended behavior (e.g., assert that nameA is present only when the expanded
directive is provided), ensuring the final test has no debug logs and its
assertions match the declared intent.
| import { gql } from '@apollo/client'; | ||
| import { buildSubgraphSchema } from '@apollo/subgraph'; | ||
| import { mergeTypeDefs } from '@graphql-tools/merge'; |
There was a problem hiding this comment.
Remove unused imports.
Static analysis confirms that gql, buildSubgraphSchema, and mergeTypeDefs are imported but never used in this test file. These should be removed to keep the codebase clean.
Apply this diff to remove the unused imports:
-import { gql } from '@apollo/client';
-import { buildSubgraphSchema } from '@apollo/subgraph';
-import { mergeTypeDefs } from '@graphql-tools/merge';
import { makeExecutableSchema } from '@graphql-tools/schema';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { gql } from '@apollo/client'; | |
| import { buildSubgraphSchema } from '@apollo/subgraph'; | |
| import { mergeTypeDefs } from '@graphql-tools/merge'; | |
| import { makeExecutableSchema } from '@graphql-tools/schema'; |
🧰 Tools
🪛 GitHub Check: Type Check on GraphQL v15
[failure] 20-20:
'mergeTypeDefs' is declared but its value is never read.
[failure] 19-19:
'buildSubgraphSchema' is declared but its value is never read.
[failure] 18-18:
'gql' is declared but its value is never read.
🤖 Prompt for AI Agents
In packages/utils/tests/print-schema-with-directives.spec.ts around lines 18 to
20, the imports "gql" from '@apollo/client', "buildSubgraphSchema" from
'@apollo/subgraph', and "mergeTypeDefs" from '@graphql-tools/merge' are unused;
remove these three import specifiers/lines from the file header so only actually
used imports remain, run tests/lint to confirm no other references, and commit
the change.
2 participants
🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant
motivation and context. List any dependencies that are required for this change.
Related # (issue)
Type of change
Please delete options that are not relevant.
expected)
Screenshots/Sandbox (if appropriate/relevant):
Adding links to sandbox or providing screenshots can help us understand more about this PR and take
action on it as appropriate
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can
reproduce. Please also list any relevant details for your test configuration
Test Environment:
@graphql-tools/...:Checklist:
CONTRIBUTING doc and the
style guidelines of this project
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose
the solution you did and what alternatives you considered, etc...