enhance GraphQL schema import error debugging with source locations #7437
Conversation
🦋 Changeset detectedLatest commit: b04b242 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughRefactors dependency tracking to map dependency names to NameNode references, updates extractDependencies return type, enables location-aware parsing, and switches error handling to GraphQLError with precise locations. Tests now assert GraphQLError and exact formatted messages; a new SDL fixture exercises multiple missing-type occurrences. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Importer as import/index.extractDependencies
participant Parser as parseGraphQLSDL
participant Visitor as AST Visitor
participant Resolver as Dependency Resolver
Caller->>Importer: extractDependencies(filePath, fileContents)
Importer->>Parser: parseGraphQLSDL(fileContents) (with locations enabled)
Parser-->>Importer: AST
Importer->>Visitor: visit(AST)
Visitor-->>Importer: dependenciesByDefinitionName (Map<defName, DependencySet>)
Importer->>Resolver: resolve dependencies (uses NameNode locations)
alt unresolved dependency found
Resolver-->>Importer: unresolved name + Set(NameNode)
Importer-->>Caller: throw GraphQLError(locations from NameNodes)
else all dependencies resolved
Importer-->>Caller: return { definitionsByName, dependenciesByDefinitionName }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/import/src/index.ts (1)
345-365: Bump major version and document breaking API change
- The return type of
extractDependencieshas changed fromMap<string, Set<string>>toDependenciesByDefinitionName(Map<string, Map<string, Set<NameNode>>>), which breaks existing type contracts.- In
packages/import/package.json, bump the MAJOR version and add a BREAKING CHANGE entry toCHANGELOG.mddescribing this signature change.
🧹 Nitpick comments (2)
packages/import/tests/schema/import-schema.spec.ts (1)
803-816: Consider using a more robust path comparison approach.The hardcoded
path.resolve(__dirname, './fixtures/type-not-found/a.graphql')in the expected error string may cause issues when tests run in different environments or with different path separators.Consider normalizing paths or using a more flexible assertion:
- expect(e.toString()).toBe( - ` -Couldn't find type Post in any of the schemas. - -${path.resolve(__dirname, './fixtures/type-not-found/a.graphql')}:2:9 -1 | type Dummy { -2 | test: Post - | ^ -3 | } -`.trim(), - ); + const expectedPath = path.resolve(__dirname, './fixtures/type-not-found/a.graphql'); + expect(e.toString()).toContain('Couldn\'t find type Post in any of the schemas.'); + expect(e.toString()).toContain(expectedPath); + expect(e.toString()).toContain('2 | test: Post'); + expect(e.toString()).toContain(' | ^');packages/import/src/index.ts (1)
238-264: Consider optimizing the dependency collection logic.The new dependency collection creates a fresh Map for each field, which could be optimized by reusing the dependency set when possible.
Consider this optimization to reduce object allocations:
- const newDependencySet = new Map<string, Set<NameNode>>(); + let newDependencySet = dependenciesByDefinitionName.get(fieldDefinitionName); + if (!newDependencySet) { + newDependencySet = new Map<string, Set<NameNode>>(); + } switch (field.kind) { case Kind.FIELD_DEFINITION: visitFieldDefinitionNode(field, newDependencySet, dependenciesByDefinitionName); break; case Kind.INPUT_VALUE_DEFINITION: visitInputValueDefinitionNode( field, newDependencySet, dependenciesByDefinitionName, ); break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/import/src/index.ts(19 hunks)packages/import/tests/schema/fixtures/type-not-found/h.graphql(1 hunks)packages/import/tests/schema/import-schema.spec.ts(8 hunks)
🔇 Additional comments (11)
packages/import/tests/schema/fixtures/type-not-found/h.graphql (1)
1-5: LGTM! Test fixture appropriately exercises multiple missing type references.This fixture effectively supports the new test case for validating multiple error locations when a type is missing multiple times within a single definition.
packages/import/tests/schema/import-schema.spec.ts (3)
4-4: LGTM! GraphQLError import supports enhanced error handling.The addition of GraphQLError import aligns with the refactored error handling that provides location-aware error reporting.
824-837: LGTM! Error assertions properly validate GraphQLError instances.The test correctly verifies both the error type and the detailed error message with location information.
943-974: Excellent test coverage for multiple missing type references!This new test case effectively validates that all occurrences of a missing type are reported with their respective locations, which greatly improves debugging experience.
packages/import/src/index.ts (7)
14-14: LGTM! Essential imports for location-aware error handling.The addition of GraphQLError and NameNode imports properly supports the enhanced error reporting with source locations.
Also applies to: 24-24
321-343: Well-documented type definitions for dependency tracking!The new DependencySet and DependenciesByDefinitionName types with clear documentation improve code maintainability and make the location-aware dependency tracking implementation easier to understand.
355-355: Location-aware parsing enabled as intended.Removing
noLocation: truefromparseGraphQLSDLenables location data in AST nodes, which is essential for the enhanced error reporting feature.
284-294: Excellent error reporting with precise location information!The GraphQLError with location nodes provides exactly the debugging improvement described in the PR objectives. This will significantly help developers identify where missing type references occur.
761-769: Well-implemented helper function for dependency tracking.The
addToDependencySethelper properly manages the Map of Sets structure and ensures efficient addition of NameNode references.
823-841: Ignore the interface dependency logic concern: bothnode.nameandnamedTypeNode.nameyieldNameNodeobjects, soallDependenciesremains type-consistent and no mixing with raw strings occurs.Likely an incorrect or invalid review comment.
553-553: Verify missing dependency handling when iterating dependency keysLine 553 in packages/import/src/index.ts: the loop now iterates solely over
dependenciesOfDefinition.keys(). No tests cover this path—please confirm definitions without dependencies are still handled as before.
packages/import/src/index.ts
Outdated
| !definitionsByName.has(dependencyName) | ||
| ) { | ||
| throw new Error(`Couldn't find type ${dependencyName} in any of the schemas.`); | ||
| throw new GraphQLError( |
There was a problem hiding this comment.
We usually use createGraphQLError helper from @graphql-tools/utils to fix the problems between GraphQLjs versions.
…7437) * plumb nodes * add locations * self review * update tests * one missing type multiple times in a single type * self review * Use createGraphQLError --------- Co-authored-by: Arda TANRIKULU <ardatanrikulu@gmail.com>
2 participants
Description
This PR enhances the developer experience when debugging GraphQL schema import errors by transforming basic error messages into proper
GraphQLErrorinstances with accurate source locations. For example:Missing type output before this PR
Missing type output after this PR
This significantly improves the developer experience by immediately pinpointing where missing type references occur, reducing debugging time and making schema issues much easier to resolve.
Fixes#7443
Risks
This PR just changes the error format and leaves all business logic intact so I expect it to have low risk. However, it does enable parsing graphql with locations. This will require more memory (though I expect negligible) and could result in graphql source code to be exposed through error messages. That said, a graphql server with schema reflection already exposes the graphql source code and any graphql queries on the client also expose the schema. So while there is risk of exposing raw source code, it's only schema which is often already exposed. If this is a concern we could consider plumbing an option to remove locations ( with the trade off of worsening debug messages).
Type of change
Please delete options that are not relevant.
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
GraphQLErrorresponsesGraphQLErrorin a way which wasn't previously (shows debug output for each reference).To run:
npm testTest Environment:
@graphql-tools/...: importChecklist:
CONTRIBUTING doc and the
style guidelines of this project