These are chat archives for ManageIQ/manageiq/rails5

31st
May 2016
Jason Frey
@Fryguy
May 31 2016 16:28
I did not see that...I really have to fix my gmail settings so these kinds of @mentions pop out from the crowd :(
Joe Rafaniello
@jrafanie
May 31 2016 16:29
this feels like a blocker bug for us and should be for rails 5 if rails 4.2 allowed you do update attributes you didn't include in your select
Jason Frey
@Fryguy
May 31 2016 16:30
yeah, interesting
Matthew Draper
@matthewd
May 31 2016 16:40
But.. you did include it in your select? :confused:
Jason Frey
@Fryguy
May 31 2016 16:41
@matthewd You're like a wizard o_O
Joe Rafaniello
@jrafanie
May 31 2016 16:41
so, two bugs @matthewd, apparently, .select("id).first.update_attributes(other_columns) worked in rails 4.2, haven't confirmed yet
Jason Frey
@Fryguy
May 31 2016 16:41
I didn't expect you to appear :sparkles:
Joe Rafaniello
@jrafanie
May 31 2016 16:41
it doesn't now, you need to add other_columns to the select
it also looks like updated_on gets magically added as required to the select for models with that column
trying to write some tests on AR
Matthew Draper
@matthewd
May 31 2016 16:42
Ah, for the same reason
Joe Rafaniello
@jrafanie
May 31 2016 16:43
yeah, both columns get .changed? called on them
Matthew Draper
@matthewd
May 31 2016 16:43
Because update_attributes(foo: 1) is "really" update_attributes(foo: 1, updated_at: now)
Chris Arcand
@chrisarcand
May 31 2016 16:55
I thought if you just said "Rails 5” three times @matthewd just magically appears.
Only once!
Joe Rafaniello
@jrafanie
May 31 2016 17:01
I have a feeling @matthewd realizes we're one of the only ones using rails 5 on each commit basis ;-)
Or :sob:
Oleg Barenboim
@chessbyte
May 31 2016 17:11
the latter
wishing those Rails guys ;-) can just ship Rails5 already
Matthew Draper
@matthewd
May 31 2016 17:12
Real Soon Now™
Joe Rafaniello
@jrafanie
May 31 2016 17:13
:sob:
ok, got it
Joe Rafaniello
@jrafanie
May 31 2016 17:18
this is :poop: awesome
will gist it and take lunch
tl;dr: rails 4.2 didn't blow up if you didn't select a column you want to later update, it silently didn't save it
unless I did something wrong, one second, gist coming
Joe Rafaniello
@jrafanie
May 31 2016 17:30
I think that's a good test demonstrating the problems (and there are a few)
Matthew Draper
@matthewd
May 31 2016 19:02
rails/rails@c4cb686
Joe Rafaniello
@jrafanie
May 31 2016 19:03
:clap:
5-0-stable ??? :bow:
@matthewd what do you think of that gist? :point_up: May 31, 2016 1:30 PM
Joe Rafaniello
@jrafanie
May 31 2016 19:08
I can't tell how far up the stack we need to go to tell the mutation tracker to only check dirty on a subset of the attributes
Matthew Draper
@matthewd
May 31 2016 19:09
Sean's leaning towards an exception being the better choice (but a more informative one than NotImplementedError)
Joe Rafaniello
@jrafanie
May 31 2016 19:09
4.2 didn't care if you did .select(:name) and then did object.update(other_name: "blah")
it didn't throw an exception at all
Matthew Draper
@matthewd
May 31 2016 19:09
didn't care = ignored, or didn't care = wrote the value?
If it silently ate your data, that doesn't seem like a feature we'd want to retain
I can definitely see an argument that we should just write it, skipping the dirty check, on the basis that it's clearly dirty-by-definition… but yeah, that would be contingent on there being a sane place to apply such a decision
Joe Rafaniello
@jrafanie
May 31 2016 19:12
AFAIK, 4.2 for object = Model.select(:name).first; object.update(other_name: "blah"), it didn't save "blah", but didn't raise an error
5.0 doesn't save "blah" and raise an error
Jason Frey
@Fryguy
May 31 2016 19:13

didn't save "blah", but didn't raise an error

seems wrong...I would not expect rails to keep that "feature"

Joe Rafaniello
@jrafanie
May 31 2016 19:13
on top of that, if you have updated_on or updated_at, you also have to include them in the select clause to avoid the NotImplementedError
Jason Frey
@Fryguy
May 31 2016 19:13
@jrafanie Do we know where this affects us?
Matthew Draper
@matthewd
May 31 2016 19:14
updated_on/_at is definitely an edge case… it's arguably less unreasonable for us to just drop that change. But I'd still much prefer we kept it and actually wrote it, if we're going to do something non-errory.
Joe Rafaniello
@jrafanie
May 31 2016 19:15
@Fryguy I'm still trying to understand if we 4.2-stable's behavior was a problem (silently discarding your update)
Matthew Draper
@matthewd
May 31 2016 19:16
If you were only hitting it on the updated_on, then probably not, I guess
Jason Frey
@Fryguy
May 31 2016 19:16
updated_on is mostly used for reporting purposes, so there's a good chance it was just not seen as well
I don't think we have much business logic that uses that column
Joe Rafaniello
@jrafanie
May 31 2016 19:17
I don't know, with 4.2-stable, this doesn't actually set color to blue
  def setup
    Parrot.create!(name: 'Joe')
  end

  def test_update_for_unselected_column
    parrot = Parrot.where(name: 'Joe').select(:name).first
    parrot.update(color: 'blue')
    assert_equal 'blue', Parrot.first.color
  end
Matthew Draper
@matthewd
May 31 2016 19:17
@jrafanie right; that's definitely a bug IMO. But are you hitting that general case?
Joe Rafaniello
@jrafanie
May 31 2016 19:18
I"m not sure how to find all the places we do select to ensure we're selecting the columns for a subsequent update
@matthewd the only time it seems to work as I'd expect is if you don't use select
Matthew Draper
@matthewd
May 31 2016 19:18
Rails 5 seems to be doing a good job of telling you about them :wink:
Joe Rafaniello
@jrafanie
May 31 2016 19:18
:laughing: => :sob:
Jason Frey
@Fryguy
May 31 2016 19:19
we've been on Rails 5 for a while now, and ManageIQ/manageiq#8687 is the first occurence of this so ¯\_(ツ)_/¯
Joe Rafaniello
@jrafanie
May 31 2016 19:19
this should just work in both 4-2-stable and 5-0-stable but it is nil in 4-2-stable and raises the NotImplementedError on 5-0-stable
  def test_update_for_selected_column
    parrot = Parrot.where(name: 'Joe').select(:name, :color).first
    parrot.update(color: 'blue')
    assert_equal 'blue', Parrot.first.color
  end
ETOOMANYBUGS or I'm doing something wrong
Matthew Draper
@matthewd
May 31 2016 19:21
More than one Parrot?
Joe Rafaniello
@jrafanie
May 31 2016 19:21
haha, let me check
Matthew Draper
@matthewd
May 31 2016 19:22
The self-contained AR bug script thingy can help protect against that sort of ambiguity (hint hint :smile:)
Joe Rafaniello
@jrafanie
May 31 2016 19:22
it's hard to run git bisect that way
Matthew Draper
@matthewd
May 31 2016 19:22
Really? That's the main reason we prefer it :confused:
Joe Rafaniello
@jrafanie
May 31 2016 19:28
we're probably talking about different things @matthewd, I thought you meant the "single file rails app with bundler_inline in it" <--- not sure how to git bisect that unless you inject sha1 into the gem line
Matthew Draper
@matthewd
May 31 2016 19:29
path:… or just bundle exec it from inside your rails/rails clone
Joe Rafaniello
@jrafanie
May 31 2016 19:31
ok, yeah, I don't think it matters, I couldn't bundle update in activerecord for git bisect purposes at some commits anyway because they referenced arel master which is not the same as it is now
Joe Rafaniello
@jrafanie
May 31 2016 19:39
I don't know, I must be doing something wrong, 4.2-stable always discards your updates if you do .select
@matthewd all that aside, 5.0 now raises an exception where 4.2-stable doesn't if you forget to select (updated_on, updated_at, and the column you want to update)... is that worthy of a bug?
Matthew Draper
@matthewd
May 31 2016 19:41
The 5.0 behaviour is bug-worthy, because the exception is unhelpful, and the comparison with 4.2 will be relevant in considering how to address it
… but while something will definitely change from what it does right now, I doubt we'll go back to the 4.2 behaviour
Joe Rafaniello
@jrafanie
May 31 2016 19:42
From my testing, we have no workaround for the exception + discarded update... adding updated_on, updated_at, column names we want to update... we have to remove the select
Jason Frey
@Fryguy
May 31 2016 19:44
@jrafanie That seems to go against what I thought you stated earlier...that is, I thought if it was added to the select it would not give the error (and would also get written)
Joe Rafaniello
@jrafanie
May 31 2016 19:44
look at the test, don't read my words
;-)
Jason Frey
@Fryguy
May 31 2016 19:44
ETOOMANYLINESOFCODE
Joe Rafaniello
@jrafanie
May 31 2016 19:44
le sigh
Jason Frey
@Fryguy
May 31 2016 19:45
I had a hard time figuring out which of the 4/5 were errors, which was supposed to pass, and which wasn't :confused:
Joe Rafaniello
@jrafanie
May 31 2016 19:45
ETOOMANYBUGS
let me change it to assert nothing raised as bug 1
Jason Frey
@Fryguy
May 31 2016 19:46
Also your test checks the object itself (which may not have the methods), but it doesn't check whether it was written to the database or not
those are 2 different things
Matthew Draper
@matthewd
May 31 2016 19:47
I think we can ignore the updated_on/_at ones
Jason Frey
@Fryguy
May 31 2016 19:47
that is, it could have written updated_on to the database, but if you call .updated_on it could still return nil. Better test might be to requery the object and see if the backend got the expected value
Matthew Draper
@matthewd
May 31 2016 19:48
Those are just another instance of the same thing we're seeing when the regular column is present/omitted
So I think they complicate the readability, without being necessary for a fix
Joe Rafaniello
@jrafanie
May 31 2016 19:52
@matthewd yeah, the trick is that as a user, you'd expect, ok, I need to provide the "color" in the select if I want to do update(color => "blue")... you'd be wrong on 5-0-stable because even that blows up with the NotImplementedError because it wants the timestamps in the select
Jason Frey
@Fryguy
May 31 2016 19:53
@jrafanie If you then provide the timestamps in the select, does it then work?
Joe Rafaniello
@jrafanie
May 31 2016 19:53
@Fryguy by work, if you mean doesn't throw that exception, then yes, it works
it doesn't actually save it, at least from my testing
Jason Frey
@Fryguy
May 31 2016 19:54
no, by work I mean...not throw the exception and also write to the database
ok, then that seems like a real bug
Matthew Draper
@matthewd
May 31 2016 19:57
@jrafanie I understand that, but it's a secondary symptom from the main problem you're reporting. It merits a mention in the bug report ("note how this interacts with a missing updated_at" or something), but exploding the test gist to six mostly-failing assertions makes it more of a distraction from the root cause, IMO
(but yeah, the fact that 4.2 sounds also-though-differently buggy here is not helping things :smile:)
@MD Can you see if this patch fixes that issue, or send me a repro script?
diff --git a/activerecord/lib/active_record/attribute.rb b/activerecord/lib/active_record/attribute.rb
index 701d24d..e7c9c21 100644
--- a/activerecord/lib/active_record/attribute.rb
+++ b/activerecord/lib/active_record/attribute.rb
@@ -211,6 +211,9 @@ def value
end
end

+ def changed?
+ end
+
def value_for_database
end
Joe Rafaniello
@jrafanie
May 31 2016 19:59
ugh, looksl ike the test model parrots is a bad one to use because it doesn't have a id column
D, [2016-05-31T15:58:04.853592 #23296] DEBUG -- :   Parrot Load (0.3ms)  SELECT  "parrots"."name", "parrots"."color", "parrots"."updated_on", "parrots"."updated_at" FROM "parrots" WHERE "parrots"."name" = $1 ORDER BY "parrots"."id" ASC LIMIT $2  [["name", "Joe"], ["LIMIT", 1]]
D, [2016-05-31T15:58:04.853974 #23296] DEBUG -- :    (0.1ms)  SAVEPOINT active_record_1
D, [2016-05-31T15:58:04.855471 #23296] DEBUG -- :   SQL (0.3ms)  UPDATE "parrots" SET "color" = $1 WHERE "parrots"."id" IS NULL  [["color", "blue"]]
D, [2016-05-31T15:58:04.855719 #23296] DEBUG -- :    (0.1ms)  RELEASE SAVEPOINT active_record_1
D, [2016-05-31T15:58:04.856439 #23296] DEBUG -- :   Parrot Load (0.2ms)  SELECT  "parrots".* FROM "parrots" WHERE "parrots"."name" = $1 AND "parrots"."color" = $2 ORDER BY "parrots"."id" ASC LIMIT $3  [["name", "Joe"], ["color", "blue"], ["LIMIT", 1]]
Matthew Draper
@matthewd
May 31 2016 20:00
@jrafanie did I mention the standalone script? :wink2:
Joe Rafaniello
@jrafanie
May 31 2016 20:00
that might be why i'm not seeing the update
Jason Frey
@Fryguy
May 31 2016 20:00
:trollface:
Joe Rafaniello
@jrafanie
May 31 2016 20:01
that's even more :trollface: worthy since I've been told to provide unit tests in other situations ;-)
Matthew Draper
@matthewd
May 31 2016 20:02
Generally I'll go with a script for "this weird thing is happening", and only reach first for a test if I'm trying to define a slight variation on an existing one for example
Both are fine as means of reporting (though the script is slightly preferred, unless you've already tracked down the cause)… the big advantage of the stand-alone is that it makes the repro & reporting steps simpler
Joe Rafaniello
@jrafanie
May 31 2016 21:08
Ok, on a hunch, I used a standalone script to recreate it and the oddities with saving the record are fixed (the AR test model Parrot I used previously didn't have an id column previously and that caused the confusion) :wink: @matthewd
Joe Rafaniello
@jrafanie
May 31 2016 21:20
Opened an issue: rails/rails#25228
I need to run out, will be offline, working on things, ttyl
Matthew Draper
@matthewd
May 31 2016 21:20
Thanks! :heart: