Fastbuilder - shadow PR #1
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…er check Replace Pattern.matcher().matches() with direct character validation in Assert.assertValidName(). This method is called in the constructor of every GraphQL type (objects, interfaces, unions, enums, inputs, scalars), field, argument, and directive during schema construction. For large schemas (18k+ types), this eliminates tens of thousands of regex compilations and matches, providing significant performance improvement: - FastBuilder: 25% faster (364ms → 272ms on 18,837-type schema) - Standard Builder: 4% faster (1967ms → 1883ms on same schema) The character-by-character check maintains identical validation semantics while avoiding regex Pattern compilation and Matcher object allocation overhead on every name validation.
Add GraphQLSchema.FastBuilder, a high-performance schema builder that avoids full-schema traversals. This initial implementation includes: - FastBuilder class with constructor accepting code registry builder and root types (query, mutation, subscription) - New private GraphQLSchema constructor for FastBuilder - ShallowTypeRefCollector stub class for future type reference handling - Support for adding scalar types via additionalType() - Automatic addition of built-in directives - Optional validation support (disabled by default) - Comprehensive test suite in FastBuilderTest.groovy 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Implement directive handling in FastBuilder with support for type references in directive arguments: - Move ShallowTypeRefCollector to graphql.schema package (for access to package-private replaceType methods) - Implement additionalDirective() with duplicate detection - Implement ShallowTypeRefCollector.handleDirective() to scan arguments - Implement type reference resolution for directive arguments - Support List and NonNull wrapped type references - Throw AssertException for missing type references - Add comprehensive tests for directive type reference resolution 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Enumeration types work with the existing additionalType() implementation. Add tests to verify: - Enum types can be added to schema - Enum type matches standard builder output - Directive arguments can reference enum types via GraphQLTypeReference 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Extend ShallowTypeRefCollector to handle GraphQLInputObjectType: - Scan input object fields for type references - Implement replaceInputFieldType() for field type resolution - Support nested input types via type references - Support List and NonNull wrapped type references in input fields - Add comprehensive tests for input object type handling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Extend ShallowTypeRefCollector to handle applied directives: - Add scanAppliedDirectives() method for scanning applied directive args - Scan applied directives on directive container types (enum, scalar, etc.) - Scan applied directives on input object fields - Implement replaceAppliedDirectiveArgumentType() for type resolution - Update FastBuilder.withSchemaAppliedDirective() to scan for type refs - Add comprehensive tests for applied directive type reference resolution 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Extended ShallowTypeRefCollector to handle GraphQLObjectType: - Scans field types for type references - Scans field arguments for type references - Scans applied directives on fields - Scans interfaces for type references - Added resolveOutputType() for output type reference resolution - Added replaceFieldType() for field type replacement - Added ObjectInterfaceReplaceTarget wrapper and replaceObjectInterfaces() - Updated FastBuilder.additionalType() to maintain interface→implementations map - Added comprehensive tests for object type handling: - Field type reference resolution (direct, NonNull, List) - Interface type reference resolution - Interface→implementations map building - Field argument type reference resolution - Applied directive on fields - Error cases for missing types/interfaces 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Extended ShallowTypeRefCollector to handle GraphQLInterfaceType: - Scans field types for type references - Scans field arguments for type references - Scans applied directives on fields - Scans extended interfaces for type references - Added handleInterfaceType() method - Added InterfaceInterfaceReplaceTarget wrapper class - Added replaceInterfaceInterfaces() for interface extension resolution - Updated FastBuilder.additionalType() to wire type resolvers from interfaces - Added comprehensive tests for interface type handling: - Basic interface type addition - Field type reference resolution - Interface extending interface via type reference - Type resolver wiring from interface - Field argument type reference resolution - Applied directive on interface fields - Error cases for missing extended interfaces 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Extended ShallowTypeRefCollector to handle GraphQLUnionType: - Scans possible types for type references - Applied directives already handled by directive container scan - Added handleUnionType() method - Added UnionTypesReplaceTarget wrapper class - Added replaceUnionTypes() for union member type resolution - Updated FastBuilder.additionalType() to wire type resolvers from unions - Added comprehensive tests for union type handling: - Basic union type addition - Union member type reference resolution - Type resolver wiring from union - Error cases for missing member types - Applied directive on union types 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fixed additionalTypes not being set in FastBuilder path: - Added additionalTypes parameter to FastBuilder private constructor - Added buildAdditionalTypes() method to compute additionalTypes from typeMap - Validation now properly traverses all types in the schema - Added comprehensive tests for validation and edge cases: - withValidation(false) skips validation - withValidation(true) runs validation and catches errors - Circular type reference resolution - Deeply nested type reference resolution (NonNull + List + NonNull) - Complex schema with interfaces, unions, input types - Null handling for types/directives - Built-in directives are added automatically 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add BuildSchemaBenchmark to measure schema construction performance in isolation from SDL parsing. This benchmark compares standard SchemaGenerator against FastSchemaGenerator using the large-schema-4.graphqls test schema (~18,800 types). Changes: - Add FastSchemaGenerator that uses FastBuilder for optimized schema construction from pre-parsed TypeDefinitionRegistry - Add BuildSchemaBenchmark.java with side-by-side comparison of standard vs fast schema generation, parsing SDL once in @setup to isolate build performance - Add jmhProfilers gradle property support for GC profiling (e.g., -PjmhProfilers="gc") - Add FastSchemaGeneratorTest to verify equivalence with standard SchemaGenerator Usage: ./gradlew jmh -PjmhInclude=".*BuildSchemaBenchmark.*" ./gradlew jmh -PjmhInclude=".*BuildSchemaBenchmark.*" -PjmhProfilers="gc"
A reviewer pointed out an unnecessary check was being performed, the commit removes it. Also, in re-reading that code I noticed that the behavior of `Builder.additionalType` and `FastBuilder.additionalType` were subtly different. To prevent potential confusion, this commit renames it to `addType` and documents its behavior more explicitly.
Clean up test files by removing development phase comments like "==================== Phase X: ... ====================" that were used during incremental implementation.
- Fix reference to non-existent addAdditionalType(s) method - Correct additionalTypes documentation to reflect actual behavior - Simplify type unwrapping code
- addType() and addTypes() now take GraphQLNamedType instead of GraphQLType - Remove unnecessary unwrapping logic since only named types are accepted - Remove null checks from builder methods (fail fast on null) - Update FastSchemaGenerator to use GraphQLNamedType - Update FastBuilderComparisonTest to use GraphQLNamedType
A reviewer noted that the dispirate ways that the classic builder and the new "fast" builder handled built-in directives created two parallel lists, which is a maintenance burden. This commit consolidates the lists.
…erences The batch method was adding applied directives directly without scanning their arguments for GraphQLTypeReference instances. This left unresolved type references in the built schema. Fix by delegating to the singular withSchemaAppliedDirective() method which already performs the scan, matching the pattern used by the standard Builder. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fixed a bug after resolving conflicts but forgot to add it to the merge commit.
Test Results 335 files + 8 335 suites +8 5m 3s ⏱️ -8s Results for commit 5bcba4a. ± Comparison against base commit 51d16e7. This pull request removes 196 and adds 293 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 719f91f7ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+80
to
+83
| // Use a dummy builder to trigger type building (this populates buildCtx) | ||
| GraphQLSchema.Builder tempBuilder = GraphQLSchema.newSchema(); | ||
| schemaGeneratorHelper.buildOperations(buildCtx, tempBuilder); | ||
|
|
There was a problem hiding this comment.
FastSchemaGenerator only calls buildOperations/buildAdditionalTypes and never calls SchemaGeneratorHelper.buildSchemaDirectivesAndExtensions, which is where schema-level directives and schema extensions are gathered and applied in the standard generator. As a result, SDL like schema @auth { query: Query } or extend schema @foo will be silently dropped (e.g., getSchemaAppliedDirectives() stays empty and schema extensions are not preserved), so schemas generated via FastSchemaGenerator diverge from SchemaGenerator for any SDL that uses schema directives/extensions.
Useful? React with 👍 / 👎.
Comment on lines
133
to
137
|
|
||
| // Disable validation for performance | ||
| fastBuilder.withValidation(false); | ||
|
|
||
| return fastBuilder.build(); |
There was a problem hiding this comment.
Unlike SchemaGenerator, FastSchemaGenerator returns the schema immediately without running SchemaDirectiveWiringSchemaGeneratorPostProcessing when directive wiring is present. That means any SchemaDirectiveWiring registered in RuntimeWiring (e.g., directives that wrap fields or alter types) will never be applied, so the generated schema’s behavior will differ from the standard path whenever directive wiring is used.
Useful? React with 👍 / 👎.
- Remove unused Pattern import from Assert.java - Add @NullMarked annotation to ShallowTypeRefCollector - Expand FastBuilder class documentation with clear guidance on: - When to use FastBuilder (large schemas, known types, pre-validated) - When NOT to use FastBuilder (type discovery, type reuse, dynamic construction) - Key differences from standard Builder - Example usage code - Add mutation warnings to addType() and build() methods - Document type mutation behavior in ShallowTypeRefCollector.replaceTypes() Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Mark assertValidName parameter as @nullable since callers may pass null - Keep null check in assertValidName (required for @nullable contract) - Remove redundant empty check that was duplicated in isValidName - Use String.valueOf() for null-safe error message formatting Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
andimarek
force-pushed
the
fastbuilder
branch
from
1411c38 to
cd32b5c
Compare
- Change from @internal to @experimentalapi, making it part of the public API - Add @NullMarked and @nullable annotations for null safety - Enable schema validation by default (previously skipped for performance) - Add withValidation option to SchemaGenerator.Options for explicit opt-out - Update documentation to reference FastBuilder limitations - Add tests for validation behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
FastBuilder now validates that all interfaces and unions have type resolvers, matching the behavior of the standard GraphQLSchema.Builder. This validation is always performed regardless of the withValidation() setting, which only controls GraphQL spec validation via SchemaValidator. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Created to trigger CI when I make changes.