Skip to content

Fix conditional validator evaluation for Class subjects and lambda arity#249

Merged
johnnyshields merged 5 commits intomongoid:masterfrom
tablecheck:fix-4.2.0-conditional-validations
May 4, 2026
Merged

Fix conditional validator evaluation for Class subjects and lambda arity#249
johnnyshields merged 5 commits intomongoid:masterfrom
tablecheck:fix-4.2.0-conditional-validations

Conversation

@johnnyshields
Copy link
Copy Markdown
Member

@johnnyshields johnnyshields commented Apr 14, 2026

The 4.2.0 conditional validator support (commit bd9577e) was added with the intention of checking conditional validations on models.

This change was well-intentioned, but it was done in a way that broke semver. Specifically, it enforced conditional checks on all validation cases in a way that would cause specs which were passing on 4.1.0 to now fail.

To fix the fundamental semver issue, this PR puts the "conditional validator support" logic behind a gate method .check_conditions:

subject.validates_presence_of(:foobar).check_conditions

In addition, bd9577e had 3 subtler issues which are fixed in this PR:

  1. It introduced a hard error on Class subjects, which broke semver. This PR raises an informative error if you use .check_conditions on a Class subject (which one wouldn't do by default; it would be a clear sign of a miswritten spec).
  2. instance_exec broke lambdas with explicit args (->(obj) { ... }). This PR now uses filter.call(actual) for non-zero arity.
  3. It had no support for Array conditions; Rails wraps if/unless into arrays when using on:/except_on: options

The 4.2.0 conditional validator support (commit bd9577e) had three issues:
1. It introduced a hard error on Class subjects, which broke semver. This PR reverts it to skip conditionals for classes.
2. instance_exec broke lambdas with explicit args (->(obj) { ... }) now uses filter.call(actual) for non-zero arity
3. It had no support for Array conditions; Rails wraps if/unless into arrays when using on:/except_on: options

This PR resolves all three issues
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 14, 2026

Danger Report

No issues found.

View run

@johnnyshields
Copy link
Copy Markdown
Member Author

@dblock this PR is ready for your review

@johnnyshields
Copy link
Copy Markdown
Member Author

@dblock second thoughts on this. I think we should make a new modifier .with_conditions (or .check_conditions!) that explicitly enables the instance-level check.

expect(subject).to validate_presence_of(:name).with_conditions

The way this feature was implemented in mongoid-rspec 4.2.0 broke a bunch of my cases that were working fine in 4.1.0.

@dblock
Copy link
Copy Markdown
Collaborator

dblock commented Apr 15, 2026

@johnnyshields feel free to merge as is or wait and implement your suggestion

@johnnyshields
Copy link
Copy Markdown
Member Author

OK, let me have a think, I will probably do a re-implementation here later today.

johnnyshields and others added 3 commits May 4, 2026 18:40
By default, :if/:unless conditions are no longer evaluated during
matcher detection, restoring pre-4.2.0 behavior. Users can opt in
to condition evaluation with the .check_conditions modifier.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@johnnyshields johnnyshields merged commit 999803b into mongoid:master May 4, 2026
25 checks passed
@johnnyshields
Copy link
Copy Markdown
Member Author

johnnyshields commented May 4, 2026

@dblock I've gone ahead and merged this. In a nutshell, I've gated the "conditional valdiation" feature introduced in 4.2.0 (PR #239) behind a .check_conditions method, which preserves semver compatibility but allows users who want the feature to opt-into it.

To use it:

it "validates foobar field conditionally" do
  subject.validates_presence_of(:foobar).check_conditions
end

We could consider a more substantial breaking change in mongoid-rspec 5.0 if users really want this behavior, or perhaps a global flag such as Mongoid::Rspec.default_check_conditions = true. Personally, I think it is probably the wrong abstraction; .check_conditions feels a bit magical to me and I prefer to write more explicit RSpec cases to check validation conditions.

@knovoselic please note, you will need to update your tests to use the .check_conditions method where you want to use this.

@johnnyshields johnnyshields deleted the fix-4.2.0-conditional-validations branch May 4, 2026 11:53
@dblock
Copy link
Copy Markdown
Collaborator

dblock commented May 4, 2026

I think a major version bump and removing the magic makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants