use model.erorrs.any? instead of model.invalid? #8

nyrf · 2020-05-19T06:27:32Z

https://github.com/leastbad/optimism/blob/master/lib/optimism.rb#L73

before

  Post Exists? (0.2ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = ? AND "posts"."id" != ? LIMIT ?  [["name", "222"], ["id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE Post Exists? (0.0ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = ? AND "posts"."id" != ? LIMIT ?  [["name", "222"], ["id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE Post Exists? (0.0ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = ? AND "posts"."id" != ? LIMIT ?  [["name", "222"], ["id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE Post Exists? (0.0ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = ? AND "posts"."id" != ? LIMIT ?  [["name", "222"], ["id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE Post Exists? (0.0ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = ? AND "posts"."id" != ? LIMIT ?  [["name", "222"], ["id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
[ActionCable] Broadcasting to OptimismChannel: {"cableReady"=>true, "operations"=>{"addCssClass"=>[{"selector"=>"#post_age_container", "name"=>"error"}, {"selector"=>"#post_form", "name"=>"invalid"}], "removeCssClass"=>[{"selector"=>"#post_name_container", "name"=>"error"}, {"selector"=>"#post_body_container", "name"=>"error"}, {"selector"=>"#post_consent_container", "name"=>"error"}], "textContent"=>[{"selector"=>"#post_name_error", "text"=>""}, {"selector"=>"#post_age_error", "text"=>"Age must be greater than or equal to 18"}, {"selector"=>"#post_body_error", "text"=>""}, {"selector"=>"#post_consent_error", "text"=>""}]}}

after

  Post Exists? (0.2ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = ? AND "posts"."id" != ? LIMIT ?  [["name", "222"], ["id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
[ActionCable] Broadcasting to OptimismChannel: {"cableReady"=>true, "operations"=>{"addCssClass"=>[{"selector"=>"#post_age_container", "name"=>"error"}, {"selector"=>"#post_form", "name"=>"invalid"}], "removeCssClass"=>[{"selector"=>"#post_name_container", "name"=>"error"}, {"selector"=>"#post_body_container", "name"=>"error"}, {"selector"=>"#post_consent_container", "name"=>"error"}], "textContent"=>[{"selector"=>"#post_name_error", "text"=>""}, {"selector"=>"#post_age_error", "text"=>"Age must be greater than or equal to 18"}, {"selector"=>"#post_body_error", "text"=>""}, {"selector"=>"#post_consent_error", "text"=>""}]}}

nyrf · 2020-05-19T06:35:25Z

If it's a nested form, it'll run more times like this

 ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  TaskItem Load (0.1ms)  SELECT "task_items".* FROM "task_items" WHERE "task_items"."post_id" = ?  [["post_id", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  TaskItem Exists? (0.1ms)  SELECT 1 AS one FROM "task_items" WHERE "task_items"."name" = ? AND "task_items"."id" != ? AND "task_items"."post_id" = ? LIMIT ?  [["name", "11112"], ["id", 1], ["post_id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE TaskItem Exists? (0.0ms)  SELECT 1 AS one FROM "task_items" WHERE "task_items"."name" = ? AND "task_items"."id" != ? AND "task_items"."post_id" = ? LIMIT ?  [["name", "11112"], ["id", 1], ["post_id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE TaskItem Exists? (0.0ms)  SELECT 1 AS one FROM "task_items" WHERE "task_items"."name" = ? AND "task_items"."id" != ? AND "task_items"."post_id" = ? LIMIT ?  [["name", "11112"], ["id", 1], ["post_id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE TaskItem Exists? (0.0ms)  SELECT 1 AS one FROM "task_items" WHERE "task_items"."name" = ? AND "task_items"."id" != ? AND "task_items"."post_id" = ? LIMIT ?  [["name", "11112"], ["id", 1], ["post_id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  TaskItem Exists? (0.1ms)  SELECT 1 AS one FROM "task_items" WHERE "task_items"."name" = ? AND "task_items"."id" != ? AND "task_items"."post_id" = ? LIMIT ?  [["name", "11112"], ["id", 5], ["post_id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE TaskItem Exists? (0.0ms)  SELECT 1 AS one FROM "task_items" WHERE "task_items"."name" = ? AND "task_items"."id" != ? AND "task_items"."post_id" = ? LIMIT ?  [["name", "11112"], ["id", 5], ["post_id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE TaskItem Exists? (0.0ms)  SELECT 1 AS one FROM "task_items" WHERE "task_items"."name" = ? AND "task_items"."id" != ? AND "task_items"."post_id" = ? LIMIT ?  [["name", "11112"], ["id", 5], ["post_id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE TaskItem Exists? (0.0ms)  SELECT 1 AS one FROM "task_items" WHERE "task_items"."name" = ? AND "task_items"."id" != ? AND "task_items"."post_id" = ? LIMIT ?  [["name", "11112"], ["id", 5], ["post_id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  TaskItem Exists? (0.1ms)  SELECT 1 AS one FROM "task_items" WHERE "task_items"."name" = ? AND "task_items"."id" != ? AND "task_items"."post_id" = ? LIMIT ?  [["name", "11122"], ["id", 8], ["post_id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE TaskItem Exists? (0.0ms)  SELECT 1 AS one FROM "task_items" WHERE "task_items"."name" = ? AND "task_items"."id" != ? AND "task_items"."post_id" = ? LIMIT ?  [["name", "11122"], ["id", 8], ["post_id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE TaskItem Exists? (0.0ms)  SELECT 1 AS one FROM "task_items" WHERE "task_items"."name" = ? AND "task_items"."id" != ? AND "task_items"."post_id" = ? LIMIT ?  [["name", "11122"], ["id", 8], ["post_id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE TaskItem Exists? (0.0ms)  SELECT 1 AS one FROM "task_items" WHERE "task_items"."name" = ? AND "task_items"."id" != ? AND "task_items"."post_id" = ? LIMIT ?  [["name", "11122"], ["id", 8], ["post_id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE Post Exists? (0.0ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = ? AND "posts"."id" != ? LIMIT ?  [["name", "222"], ["id", 1], ["LIMIT", 1]]

leastbad · 2020-05-19T12:36:40Z

Could you please add some context? I am not in your brain. Please use your words.

leastbad · 2020-05-19T13:32:38Z

Are you saying that I should use model.errors.any? instead of model.invalid?

You never actually explain yourself, and your comments are assumed to be self-evident. I sincerely appreciate feedback, but I need a lot more information from you to put this to work.

Are you working with the currently published gem or this master branch? They are quite different, which is part of the reason I'm frustrated by your lack of context.

nyrf · 2020-05-19T15:06:42Z

Sorry for my weak english, I use the master branch and base on the demo code https://github.com/leastbad/optimism-demo, i found when the post model invalid because of unique name, the log likes:

Post Exists? (0.2ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = ? AND "posts"."id" != ? LIMIT ?  [["name", "222"], ["id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE Post Exists? (0.0ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = ? AND "posts"."id" != ? LIMIT ?  [["name", "222"], ["id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE Post Exists? (0.0ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = ? AND "posts"."id" != ? LIMIT ?  [["name", "222"], ["id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE Post Exists? (0.0ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = ? AND "posts"."id" != ? LIMIT ?  [["name", "222"], ["id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
  CACHE Post Exists? (0.0ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = ? AND "posts"."id" != ? LIMIT ?  [["name", "222"], ["id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
[ActionCable] Broadcasting to OptimismChannel: {"cableReady"=>true, "operations"=>{"addCssClass"=>[{"selector"=>"#post_age_container", "name"=>"error"}, {"selector"=>"#post_form", "name"=>"invalid"}], "removeCssClass"=>[{"selector"=>"#post_name_container", "name"=>"error"}, {"selector"=>"#post_body_container", "name"=>"error"}, {"selector"=>"#post_consent_container", "name"=>"error"}], "textContent"=>[{"selector"=>"#post_name_error", "text"=>""}, {"selector"=>"#post_age_error", "text"=>"Age must be greater than or equal to 18"}, {"selector"=>"#post_body_error", "text"=>""}, {"selector"=>"#post_consent_error", "text"=>""}]}}

after i modify the code https://github.com/leastbad/optimism/blob/master/lib/optimism.rb#L73

if model.invalid? && model.errors.messages.map(&:first).include?(attribute.to_sym) to if model.errors.any? && model.errors.messages.map(&:first).include?(attribute.to_sym)

 Post Exists? (0.2ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = ? AND "posts"."id" != ? LIMIT ?  [["name", "222"], ["id", 1], ["LIMIT", 1]]
  ↳ app/controllers/posts_controller.rb:59:in `block (2 levels) in update'
[ActionCable] Broadcasting to OptimismChannel: {"cableReady"=>true, "operations"=>{"addCssClass"=>[{"selector"=>"#post_age_container", "name"=>"error"}, {"selector"=>"#post_form", "name"=>"invalid"}], "removeCssClass"=>[{"selector"=>"#post_name_container", "name"=>"error"}, {"selector"=>"#post_body_container", "name"=>"error"}, {"selector"=>"#post_consent_container", "name"=>"error"}], "textContent"=>[{"selector"=>"#post_name_error", "text"=>""}, {"selector"=>"#post_age_error", "text"=>"Age must be greater than or equal to 18"}, {"selector"=>"#post_body_error", "text"=>""}, {"selector"=>"#post_consent_error", "text"=>""}]}}

nyrf · 2020-05-19T15:18:23Z

Especially with nested form, eg:

class Post < ApplicationRecord
  validates :name, :age, :body, presence: true
  validates :name, uniqueness: true
  validates :age, numericality: { only_integer: true, greater_than_or_equal_to: 18 }
  validates :body, length: { minimum: 10 }
  validates :consent, acceptance: { message: "must be given" }


  has_many :task_items 

  accepts_nested_attributes_for :task_items, allow_destroy: true
end

class TaskItem < ApplicationRecord
  validates :name, presence: true, uniqueness: true
  belongs_to :post
end

when save post fail because of duplicate task_item name, the log likes #8 (comment), use if model.errors.any? should solve this problem.

leastbad · 2020-05-19T17:25:34Z

Thanks for following up, @nyrf. I didn't mean to imply that there was anything wrong with your English. I actually needed more of it! :)

I'm working to figure out what's going on right now. If you look in the gem version, you'll actually see that I used to have model.errors.any? and then I received a very good PR from @joshleblanc that gave a compelling reason to change it. I am now working hard to verify how it was vs. how it works now. Keeping it all in my head is honestly a bit stressful but I really want to get this right for everyone. You can see the PR in question at #7.

joshleblanc · 2020-05-19T17:46:18Z

invalid? (and valid?) perform the server-side validations on the model. (And therefor populate errors).

Without the invalid? call, the model is never validated, and the errors object will be empty (unless some method is called that performs validation, like save).

Is the problem that it's running the validations multiple times? That would be possible. We could just document that validations aren't run by optimism, and you have to do that manually.

All of those additional logs look like cache hits though, so it doesn't actually look like a performance problem.

Sorry, I'm not 100% clear on what the actual issue is 😄

leastbad · 2020-05-20T00:26:05Z

@nyrf still working on this. Could you share your relevant models? Or at least tell me if you are validating uniqueness?

validates :name, uniqueness: true

All of those Post exists? queries are uniqueness validations.

What I can confirm right now, at least in the optimism-demo app, is that something really weird is going on.

@joshleblanc the actual issue is that the library is behaving differently in subtle ways. It's cool that it's working for you, and I genuinely love the tweaks you made, but... you aren't the only user.

leastbad · 2020-05-20T01:41:36Z

Okay @joshleblanc @nyrf I think I have a reasonable, working compromise that will ensure validation and cut down on CACHE log spam.

9711063

What @joshleblanc helped me understand is that if validations are never called, the errors collection will always be empty. However, there's no need to validate the entire model over and over again, either.

So what I did was add a line: model.valid? if model.errors.empty? before the processing begins. This should properly populate the collection, allowing me to switch back to model.errors.any? This will potentially add one additional validation pass, but I think this is an acceptable tradeoff because I don't want to have to tell people to call valid on the model just to use this library.

I did keep the guarding check on line 73/74, but changed it to model.errors.any? as well. @joshleblanc I am working on the assumption here that the nested models have already had valid? called on them when the parent model is validated... so the error collection should already be built.

I also changed the Optimism.channel lambda to return "OptimismChannel" as a default. I will be giving detail instructions for authenticated configuration in the docs.

Does this work for you guys?

nyrf · 2020-05-20T02:12:12Z

@leastbad Thanks, that works.

nyrf · 2020-05-20T02:39:53Z

@leastbad I found when i use https://github.com/toptal/database_validations to validate nested model uniqueness, seems can't show nested model unique error message. this is the demo code : https://github.com/nyrf/optimism-demo/tree/nested

leastbad · 2020-05-20T03:27:24Z

@nyrf thanks for spotting this. I was just writing you a note to explain that the optimism-demo repo needs to be updated to work with the latest gem which hasn't been published yet. However, it seems like you're seeing an entirely different problem. I'm looking into it.

I am unfamiliar with the database_validations gem. Can you verify that you're still having this problem when not using the gem?

nyrf · 2020-05-20T03:56:50Z

@leastbad I found that because of the database_validations gem seems not trigger nested model invalid when use db_uniqueness validates :body, presence: true, db_uniqueness: { scope: :post_id }, this is my debug code:

You can see that nested model comment has errors, but it invalid? is false. i'll try to modify the database_validations gem, thanks.

leastbad · 2020-05-20T04:18:02Z

Just so I understand, are you saying that when you removed the database_validations gem from the test scenario, nested model validation reporting behaved correctly? I don't want to ship this if you are seeing weird problems.

nyrf · 2020-05-20T04:36:36Z

I use database_validations to validate comment model body column uniqueness when create or update at once. eg:

post = Post.new({name: 'name', age: 19, body: 'bodybodybody'})
comment = Comment.new(body: 'body')
comment2 = Comment.new(body: 'body')
post.comments = [comment, comment2]
post.save

19] pry(main)> post.errors
=> #<ActiveModel::Errors:0x00007fe0ca6045e8
 @base=
  #<Post:0x00007fe0ccb73590
   id: nil,
   name: "name",
   age: 19,
   body: "bodybodybody",
   consent: nil,
   created_at: Wed, 20 May 2020 04:27:18 UTC +00:00,
   updated_at: Wed, 20 May 2020 04:27:18 UTC +00:00>,
 @details={},
 @messages={}>

pry(main)> post.comments.last.errors
=> #<ActiveModel::Errors:0x00007fe0cbc714e8
 @base=
  #<Comment:0x00007fe0ca5483c0
   id: nil,
   post_id: 30,
   body: "body",
   created_at: Wed, 20 May 2020 04:27:18 UTC +00:00,
   updated_at: Wed, 20 May 2020 04:27:18 UTC +00:00>,
 @details={:body=>[{:error=>:taken, :value=>"body"}]},
 @messages={:body=>["has already been taken"]}>

pry(main)> post.invalid?
  Post Exists? (0.3ms)  SELECT 1 AS one FROM "posts" WHERE "posts"."name" = $1 LIMIT $2  [["name", "name"], ["LIMIT", 1]]
  Comment Exists? (0.3ms)  SELECT 1 AS one FROM "comments" WHERE "comments"."body" = $1 AND "comments"."post_id" = $2 LIMIT $3  [["body", "body"], ["post_id", 30], ["LIMIT", 1]]
  Comment Exists? (0.3ms)  SELECT 1 AS one FROM "comments" WHERE "comments"."body" = $1 AND "comments"."post_id" = $2 LIMIT $3  [["body", "body"], ["post_id", 30], ["LIMIT", 1]]
=> false
[24] pry(main)> post.comments.last.invalid?
  Comment Exists? (0.4ms)  SELECT 1 AS one FROM "comments" WHERE "comments"."body" = $1 AND "comments"."post_id" = $2 LIMIT $3  [["body", "body"], ["post_id", 30], ["LIMIT", 1]]
=> false

If not use the database_validations gem. can't valid nested model uniqueness at once, see rails/rails#20676 . other validates works fine.

leastbad · 2020-05-20T04:44:56Z

Okay, thank you. Unfortunately, I have to slept but I'll be back on this tomorrow. Thanks to both of you.

nyrf · 2020-05-20T06:27:05Z

Thanks for your great work, I found an ugly way to make accepts_nested_attributes_for method can works with uniqueness and optimism.

class MyValidator < ActiveModel::Validator
  def validate(record)
    invalid = false
    names = record.comments.map(&:body)
    record.comments.each_with_index do |item, index|
      if names.count(item.body) > 1
        item.errors.add(:body, :taken, value: item.body, message: "unique invalid")
        invalid = true
      end
    end
    record.errors.add("comments.body", "invalid") if invalid
  end
end

class Post < ApplicationRecord
  validates :name, :age, :body, presence: true
  validates :name, uniqueness: true
  validates :age, numericality: {only_integer: true, greater_than_or_equal_to: 18}
  validates :body, length: {minimum: 10}
  validates :consent, acceptance: {message: "must be given"}

  has_many :comments, dependent: :destroy
  accepts_nested_attributes_for :comments, allow_destroy: true
  validates_with MyValidator
end

leastbad · 2020-05-20T20:29:29Z

Hello @nyrf, I hope that you missed me.

I have just pushed a significant update of the optimism-demo project to master on GH.

What I recommend is that you clone/pull optimism and run bundle config set local.optimism ~/optimism (adjusting for where you have the code). https://bundler.io/v2.0/guides/git.html#local

Then pull the latest changes for optimism-demo and update the Gemfile so that it reads: gem "optimism", github: "leastbad/optimism", branch: "master" while you are testing.

There is a migration to run but otherwise it should be ready to run if you already had it working.

I believe that it demonstrates everything working as expected, without any ugly hacks. It could be that you can pull some techniques from the models and views in the project. Let me know if you have any suggestions - I don't want it to be too complex, though.

Please give this a try while I work on documentation for the new release. If everything looks good to you, I will close this issue and you can expect a formal release soon, probably tonight.

nyrf · 2020-05-21T00:42:25Z

@leastbad Yes, it works, but if i want to validate uniqueness of comment model's opinion column, i've to use the ugly code, that is a bug of rails not optimism, you can close the issue, thanks for your help.

leastbad · 2020-05-21T00:52:03Z

I understand. I did a quick Google search and saw this: https://stackoverflow.com/questions/24416599/rails-validate-nested-attributes-uniqueness-with-scope-parent-of-parent

Thanks for sticking with this and let me know if you notice anything else.

nyrf · 2020-05-21T01:12:43Z

Yes, i've, this is a bug of rails for many years, the only way is to write custom ugly codes to do that now. i used #8 (comment) and it works.

leastbad closed this as completed May 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use model.erorrs.any? instead of model.invalid? #8

use model.erorrs.any? instead of model.invalid? #8

nyrf commented May 19, 2020

nyrf commented May 19, 2020

leastbad commented May 19, 2020

leastbad commented May 19, 2020

nyrf commented May 19, 2020

nyrf commented May 19, 2020

leastbad commented May 19, 2020

joshleblanc commented May 19, 2020 •

edited

Loading

leastbad commented May 20, 2020

leastbad commented May 20, 2020 •

edited

Loading

nyrf commented May 20, 2020

nyrf commented May 20, 2020

leastbad commented May 20, 2020

nyrf commented May 20, 2020

leastbad commented May 20, 2020

nyrf commented May 20, 2020

leastbad commented May 20, 2020

nyrf commented May 20, 2020

leastbad commented May 20, 2020

nyrf commented May 21, 2020

leastbad commented May 21, 2020

nyrf commented May 21, 2020

use model.erorrs.any? instead of model.invalid? #8

use model.erorrs.any? instead of model.invalid? #8

Comments

nyrf commented May 19, 2020

nyrf commented May 19, 2020

leastbad commented May 19, 2020

leastbad commented May 19, 2020

nyrf commented May 19, 2020

nyrf commented May 19, 2020

leastbad commented May 19, 2020

joshleblanc commented May 19, 2020 • edited Loading

leastbad commented May 20, 2020

leastbad commented May 20, 2020 • edited Loading

nyrf commented May 20, 2020

nyrf commented May 20, 2020

leastbad commented May 20, 2020

nyrf commented May 20, 2020

leastbad commented May 20, 2020

nyrf commented May 20, 2020

leastbad commented May 20, 2020

nyrf commented May 20, 2020

leastbad commented May 20, 2020

nyrf commented May 21, 2020

leastbad commented May 21, 2020

nyrf commented May 21, 2020

joshleblanc commented May 19, 2020 •

edited

Loading

leastbad commented May 20, 2020 •

edited

Loading