Instantiate validators at definition time#2657
Conversation
Danger ReportNo issues found. |
5d145a5 to
f87920f
Compare
|
Missing UPGRADING notes. Working on it |
2ecb403 to
cf04c9d
Compare
|
Is there a tradeoff with things like |
e16efcf to
0b9e34b
Compare
a503556 to
2b16ba1
Compare
52b7862 to
adac9ed
Compare
2a39a4d to
2d51b32
Compare
177506d to
c4df5d3
Compare
Code reviewFound 2 issues:
Lines 1 to 5 in c4df5d3
Lines 208 to 217 in c4df5d3 🤖 Generated with Claude Code If this code review was useful, please react with 👍. Otherwise, react with 👎. |
0a19583 to
d1e35a1
Compare
dblock
left a comment
There was a problem hiding this comment.
Nice work. I think deep_freeze can't return on obj.frozen?.
|
|
||
| #### Custom validators: use `default_message_key` and `validation_error!` | ||
|
|
||
| Validators are now instantiated once at definition time and frozen. Any setup should happen in `initialize`, not in `validate_param!`. |
There was a problem hiding this comment.
So will users have to rewrite custom validators? Explain how if so with a simple example.
Validators are now instantiated once at route definition time and frozen, rather than per-request via ValidatorFactory. This removes repeated object allocation from the hot path and moves option parsing, converter building, and message formatting into initialize. - Remove ValidatorFactory; ParamsScope#validate instantiates validators directly - Freeze validator instances via Base.new override (super.freeze) - Add Grape::Util::DeepFreeze to recursively freeze Hash/Array/String options - Freeze ParamsScope at end of initialize; cache full_path as @full_path - Promote validate_param! to protected with NotImplementedError default - Extract validation_error!, hash_like?, option_value, scrub helpers in Base - Add default_message_key class macro for pre-computing exception messages - Eager-initialize all built-in validators (coerce, default, values, regexp, etc.) - ContractScopeValidator no longer inherits Base (standalone with freeze) - coerce_type receives only extracted coerce keys; callers skip manual deletes - Add DeepFreeze spec; add SameAsValidator and ExceptValuesValidator specs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| module Validations | ||
| module Validators | ||
| class ContractScopeValidator < Base | ||
| attr_reader :schema |
There was a problem hiding this comment.
@ericproulx Hi! Was there a specific reason to remove the public accessor, or was it just not needed internally anymore after switching to @schema.call?
I rely on it in grape-oas to extract the contract schema for OpenAPI spec generation. Without the accessor we have to fall back to instance_variable_get(:@schema), which feels fragile. If there's no objection, we'd be happy to open a small PR restoring attr_reader :schema.
There was a problem hiding this comment.
To add more context: for libraries that generate OpenAPI specs from Grape APIs, ContractScopeValidator is the only path to the Dry contract/schema when endpoints use the contract DSL. Unlike params do ... end which populates route.params, the contract DSL stores the schema exclusively inside the validator instance (via ContractScope pushing it into namespace_stackable[:validations]). There is no alternative public API to retrieve it.
With 3.2, the validator is now frozen and has no public accessor, so instance_variable_get(:@schema) is the only option. Restoring attr_reader :schema would give downstream libraries a stable interface for this.
This is how I see it now.
There was a problem hiding this comment.
Hello @numbata,
Thank you for addressing the situation. It makes total sense to restore the attr_reader. You can open a PR to restore it.
Thanks
There was a problem hiding this comment.
I opened the PR. Will make a release today
Summary
Validators are now instantiated once at route definition time rather than per-request via `ValidatorFactory`. This eliminates repeated object allocation on every request and moves expensive setup (option parsing, converter building, message formatting) out of the hot path.
Because validator instances are shared across all concurrent requests they are frozen after initialization, enforced by a `Validators::Base.new` override that calls `super.freeze`. All inputs (`options`, `opts`, `attrs`) arrive pre-frozen from the DSL boundary via `DeepFreeze` and `Array#freeze`, so subclass ivars derived from them are frozen by construction. A new `Grape::Util::DeepFreeze` helper recursively freezes Hash/Array/String values while intentionally leaving Procs, coercers, Classes, and other mutable objects unfrozen.
Fix #2519
Changes
Core
`Validators::Base`
`Grape::Util::DeepFreeze`
Validator-level eager initialization
Specs
Test plan