ActiveRecord/ActiveModel '#validate' alias for 'valid?'

[henrik]

It's unintuitive to call '#valid?' when you want to run validations but don't care about the return value, e.g. in a test:

x.name  = ""
x.valid?
assert_not_empty x.errors_on(:name)

Compare that to

x.name  = ""
x.validate
assert_not_empty x.errors_on(:name)

The alias in ActiveRecord isn't strictly necessary (the ActiveModel alias is still in effect), but it clarifies.

@dhh was positive when I asked a few months ago: https://twitter.com/henrik/status/390205394305703936

An alternative implementation would be to actually implement valid? in terms of validate, but I figured an alias would be the simplest thing that works.

[chancancode]

While I understand that it reads nicer in certain less common cases, but I'm not sure if the benefits are worth it.

The reason I'm on the cautious side is because every method we add to the Active Record API increases comes with the risk of conflict with user code. If we add this method, you will no longer be able to have db columns called validate or define custom methods on your model called validate.

While I agree the former is quite unlikely, I think it is quite possible that we will cause problems for existing apps/gems that defines a validate method on their models with different signature/semantics/side-effects.

For example, some apps might have defined BillingMethod#validate that hits an external service. If this becomes part of the public API and future framework code or gems starts calling this method, their app might suddenly become very slow because of those unintended service calls.

This is not a particular high risk name for us to use, but the benefits seems pretty insignificant either. Personally I'd err on the cautious side, but I'll defer to @dhh to weigh the pros/cons.

[dhh]

I don't think it's a problem to add this as an alias because it happens to the base class. So if you have your own validate method, you'll just be overwriting the method. As long as we are not using the method internally.

On Mar 23, 2014, at 13:11, Godfrey Chan [email protected] wrote:

While I understand that it reads nicer in certain less common cases, but I'm not sure if the benefits are worth it.

The reason I'm on the cautious side is because every method we add to the Active Record API increases comes with the risk of conflict with user code. If we add this method, you will no longer be able to have db columns called validate or define custom methods on your model called validate.

While I agree the former is quite unlikely, I think it is quite possible that we will cause problems for existing apps/gems that defines a validate method on their models with different signature/semantics/side-effects.

For example, some apps might have defined BillingMethod#validate that hits an external service. If this becomes part of the public API and future framework code or gems starts calling this method, their app might suddenly become very slow because of those unintended service calls.

This is not a particular high risk name for us to use, but the benefits seems pretty insignificant either. Personally I'd err on the cautious side, but I'll defer to @dhh to weigh the pros/cons.

—
Reply to this email directly or view it on GitHub.

[carlosantoniodasilva]

There's assert_not_empty I guess you could use, we try to avoid refuteusage.

[henrik]

Excellent; I like that better. Pushing a test cleanup.

[dhh]

Actually, I'm starting to have second thoughts if the only use case we can think for this is tests. These tests would have been just as well if you had a assert_not r.valid? before your error check. Do you have a non-test example?

On Mar 23, 2014, at 16:45, Henrik Nyh [email protected] wrote:

In activerecord/test/cases/validations_test.rb:

@@ -52,6 +52,21 @@ def test_error_on_given_context
assert r.save(:context => :special_case)
end

• def test_validate
• r = WrongReply.new(:title => "Valid title")

• r.validate
• assert_empty r.errors[:author_name]

• r.validate(:special_case)
• refute_empty r.errors[:author_name]
  Excellent; I like that better. Pushing a test cleanup.

—
Reply to this email directly or view it on GitHub.

[carlosantoniodasilva]

Just as a side note, thinking about your example, in general I add an assertion to the valid? call too, so it looks just fine: assert r.valid?.

[henrik]

I got a comment by email (that I don't see on this pull request for some reason) – whether I have more use cases than testing validations per above.

Grepping through one of our apps for uses of valid? without a conditional:

I see some non-test code that collects errors on several records by calling the_record.valid? on each and concatenating.

There's also some non-test code where one controller (as part of an unconventional flow) is passed a set of attributes and shows a form – it validates to show errors, if any, but doesn't do anything conditionally based on the validation result.

I also see several cases of testing validation callbacks: foo.valid?; expect(foo).to show_the_effect_of_the_callback. These are callbacks that e.g. normalize or cache some value before validation. The tests would communicate their intent a lot better if they did foo.validate instead of foo.valid? or assert foo.valid? (the latter actually distracts from the intent of the test).

Examples aside, I think a method like this makes a lot of sense in the API. I know I've wanted this method several times in the past.

[billhorsman]

I too dislike having to call valid? when I don't care about the result. I think there is a risk that someone could come back and look at that code and think it's redundant. A name that describes the intention would be nicer.

As a side issue, I don't usually use assert r.valid? because if that fails then I don't know why. I prefer to use my own custom (MiniTest) validation:

  def assert_valid(model)
    model.valid?
    assert model.errors.to_a == [], "Unexpected errors: #{model.errors.to_a}"
  end

I also have:

  def refute_valid(model)
    model.valid?
    refute model.errors.to_a == [], "Expected errors but found none"
  end

  def assert_errors(messages, model)
    model.valid?
    assert model.errors.to_a.sort == messages.sort, "Expected: #{messages.sort.inspect}\nActual: #{model.errors.to_a.sort.inspect}"
  end

  def assert_includes_error(message, model)
    model.valid?
    assert model.errors.to_a.include?(message), "Expected #{model.errors.to_a.inspect} to include #{message}"
  end

All of them would be improved by using model.validate instead. +1

[dhh]

Aight, I buy the collection case.

On Mar 23, 2014, at 7:59 PM, Henrik Nyh [email protected] wrote:

I got a comment by email (that I don't see on this pull request for some reason) – whether I have more use cases than testing validaitons.

Grepping through one of our apps for uses of valid? without a conditional:

I see some code that collects errors on several records by calling the_record.valid? on each and concatenating.

I also see a test for a validation callback: foo.valid?; expect(foo).to show_the_effect_of_the_callback. E.g. normalizing or caching some value before validation. The tests would communicate their intent a lot better if they did foo.validate instead of foo.valid? or assert foo.valid? (the latter actually distracts from the intent of the test).

Examples aside, I think a method like this makes a lot of sense in the API. I know I've wanted this method several times in the past.

—
Reply to this email directly or view it on GitHub.

[henrik]

Let me know if there's anything else I can do towards getting this merged.

[egilburg]

Semantically it would seem to me that validate should run the validation and then return self rather than true or false

[henrik]

I implemented something along those lines first, but I figured it might be better to do something minimal.

Also consider that e.g. 'save' returns a bool, so the current behavior has precedents.

Sent from my iPhone

On 27 Mar 2014, at 08:23, Eugene Gilburg [email protected] wrote:

Semantically it would seem to me that validate should run the validation and then return self rather than true or false

—
Reply to this email directly or view it on GitHub.

[rafaelfranca]

Could you add a CHANGELOG entry?

[rafaelfranca]

And also please squash your commits

[henrik]

Will do! squash and then force push this branch, right?

Sent from my iPhone

On 27 Mar 2014, at 14:36, Rafael Mendonça França [email protected] wrote:

And also please squash your commits

—
Reply to this email directly or view it on GitHub.

[rafaelfranca]

Right

[henrik]

Added CHANGELOG entry, rebased against upstream master, squashed.

The ActiveModel CHANGELOG just says "Please check 4-1-stable for previous changes." so I'm not sure what to do there. Left it alone until I hear differently.

[rafaelfranca]

Merged. Thank you so much

[henrik]

@rafaelfranca Thanks so much for the feedback and for merging!

[bogdan]

but don't care about the return value

It would be so cool if this method would return self if you don't care about returning value. I would use it for a different purpose - not just tests

[henrik]

@bogdan What would you use it for?

[bogdan]

I've written about it here: #8639
Method you've build is a little different, so suppose we have a method that builds an AR::Base object and returns it without saving in the database.

In this case method could look like this:

def build_user
  user = User.new
  # additional manipulations
  user.validate
end

[henrik]

Aha, I think I understand.

We probably shouldn't continue this discussion here (since the pullreq is merged and notifications may go out to commenters), but I suggest making your own pull request and/or starting a discussion on the rubyonrails-core mailing list. Then post the link here for interested parties.