Refactor SchemaValidator caches to be per-resolver#1185
Open
p1c2u wants to merge 1 commit into
Open
Conversation
The class-level _needs_state_cache was keyed on SchemaPath, but SchemaPath equality (inherited from pathable.BasePath) is path-only: two distinct OpenAPI specs that share a JSON-pointer path collide, returning stale answers across specs. The bug is silent in production because validation typically runs against a single spec at a time, but bites in any host that loads more than one spec, and in test suites where fresh SchemaPath.from_dict() calls produce short colliding paths. Replace the cache with a per-resolver registry (one cache per loaded spec), keyed on the resolver's identity and evicted via weakref.finalize when the resolver is garbage-collected. Inner cache keys are id(content_dict), which is safe within a single spec (the cache only lives as long as the resolver does, so id() reuse cannot cross spec boundaries). Perf-neutral on the bench (357 vs 348 ops/sec, within noise) because _schema_needs_state is only consulted during state-building, not on the per-value hot path. Adds a regression test that two specs with colliding paths return correct independent answers, and a GC test that the per-resolver cache slot is released when the spec is collected.
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
1 participant
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.
The class-level
_needs_state_cachewas keyed onSchemaPath, butSchemaPathequality (inherited frompathable.BasePath) is path-only: two distinct OpenAPI specs that share a JSON-pointer path collide, returning stale answers across specs. The bug is silent in production because validation typically runs against a single spec at a time, but bites in any host that loads more than one spec, and in test suites where freshSchemaPath.from_dict()calls produce short colliding paths.Replace the cache with a per-resolver registry (one cache per loaded spec), keyed on the resolver's identity and evicted via weakref.finalize when the resolver is garbage-collected. Inner cache keys are id(content_dict), which is safe within a single spec (the cache only lives as long as the resolver does, so id() reuse cannot cross spec boundaries).
Perf-neutral on the bench (357 vs 348 ops/sec, within noise) because
_schema_needs_stateis only consulted during state-building, not on the per-value hot path.Adds a regression test that two specs with colliding paths return correct independent answers, and a GC test that the per-resolver cache slot is released when the spec is collected.