Fix scalar overrides not applied without assertValid()#1886
Merged
Conversation
3 tasks
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: c32a6ba | Previous: 6702705 | Ratio |
|---|---|---|---|
DeferredBench::benchManyNestedDeferreds |
18.651 ms |
12.17 ms |
1.53 |
This comment was automatically generated by workflow using github-action-benchmark.
Collaborator
Author
|
@spawnia wdyt? |
Schema::getType() fell back to global built-in scalar singletons without checking config->types for scalar overrides. The override logic only ran inside getTypeMap(), triggered by assertValid(). Extract scalar override detection into a lazy cached getScalarOverrides() method used by both getType() and getTypeMap(). #1886 🤖 Generated with Claude Code
schema->assertValid is called
Collaborator
|
Added a fix, ready to go from my perspective. |
Collaborator
Author
|
Thanks. Let's try this out. I hope we refactor the whole Type system one day so that this becomes easier. |
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
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.
Summary
Schema::getType()fell back to global built-in scalar singletons without checkingconfig->typesfor scalar overrides.The override logic only ran inside
getTypeMap(), which is triggered byassertValid().Extracted scalar override detection into a lazy cached
getScalarOverrides()method used by bothgetType()andgetTypeMap(), so overrides work regardless of whetherassertValid()was called.Test plan
testTypesOverrideWorksWithTypeLoadernow passes withoutassertValid()testTypesOverrideWorksWithTypeLoaderAndAssertValidto cover theassertValid()+ typeLoader pathtestGetTypeReturnsScalarOverrideWithTypeLoaderto verifygetType()returns the override directlytestTypesOverrideWorksWithCallableTypesConfigfor callabletypesconfigtestTypesOverrideWorksWithGeneratorTypesConfigfor generatortypesconfig with subsequentassertValid()testGetTypeThenAssertValidBothWorkWithTypeLoaderto verify both code paths work in sequence