fixes #9981 - making sure lookup_keys with errors get displayed if they're broken

[unorthodoxgeek]

apparently, ActiveRecord has a bug...
foo.update_attributes(params) creates a new bar, but it fails validations so isn't saved in DB.

foo.bars will have unsaved bars, but foo.bars.includes(:baz) will discard the unsaved bars.

I ❤️ Rails, don't you?

[unorthodoxgeek]

thanks @waldenraines for helping out in investigating this issue 👍

[orrabin]

@unorthodoxgeek works for me! 👍

[waldenraines]

👍

[domcleal]

This removes the optimisation for existing smart variables and triggers a Bullet warning. Can the includes be used on existing records only?

[unorthodoxgeek]

This is an active record bug. Doesn't seem to have a workaround... So not really.

[domcleal]

Do you know in what version we can revert it?

OK, I see now that you're not going to be able to use new_record, since it's retrieving all lookup keys. Could you whitelist it in Bullet then, probably via a skip_bullet type helper?

[unorthodoxgeek]

updated. there doesn't seem to be a N+1 issue anymore, but bullet doesn't see that, probably because it listens to includes and not to the preloader service...

[domcleal]

What Bullet warning do you still get precisely? Can you paste the logs? I'm not seeing one here.

I wonder if we shouldn't just call .includes back when we're retrieving the initial Puppetclass, since then we'll be including the right objects before we call .update_attributes in the #update action, and won't be reloading them when intending to retrieve.

[unorthodoxgeek]

ok, now it's working correctly, and doesn't break MVC - I moved the preloader to a before filter, it works like a charm, no bullet warning, no extra queries.

[domcleal]

Can we use .includes with find_resource instead? This class doesn't appear to be part of the intended public API (rails/rails#11719 (comment)).

[unorthodoxgeek]

no, that's the point of this PR, this doesn't work when using includes.

[domcleal]

The original code called .includes on the association, not on the original resource, causing the objects in the association to get reloaded. Calling includes on the top level resource (Puppetclass), before setting attributes on it ought to work?

[unorthodoxgeek]

sometimes I wonder why I even bother disagreeing with you. you're right, as you usually are 😄

[domcleal]

Nah, that'd make for a very dull life, and we'd learn nothing.

[domcleal]

I think this needs to happen before the .find, probably by overriding resource_scope?

def resource_scope
  super.includes(:lookup_keys => [:lookup_values])
end

At the moment it's preloading it here, but it's not having an effect later when the view calls @puppetclass.lookup_keys[0].lookup_values or something, so causes a second query (and Bullet warning).

You can replicate this in the console, if you compare:

pc = Puppetclass.includes(:lookup_keys => [:lookup_values]).find('example')
pc.lookup_keys.first.lookup_values

to

pc = Puppetclass.find('example')
pc.lookup_keys.includes(:lookup_values)
pc.lookup_keys.first.lookup_values

The latter will generate extra queries.

[domcleal]

Merged as de65857, thanks @unorthodoxgeek.