Fixes #11767 - avoid cleaning of interface attributes #2713
Conversation
| @@ -546,7 +546,8 @@ def process_taxonomy | |||
| end | |||
|
|
|||
| def template_used | |||
| host = Host.new(clean_interfaces_attributes.except(:host_parameters_attributes)) | |||
| host = params[:id] ? Host.find(params[:id]) : Host.new | |||
There was a problem hiding this comment.
Using the real host has an unfortunate side effect that calling attributes= below will immediate change relations such as Puppet classes. Try taking an existing host, load #edit, add a Puppet class to it and click Resolve, it'll get added in the DB.
(Setting WoC as the patch seems to hinge on this line, and it could cause a lot of problems.)
There was a problem hiding this comment.
It's not caused by attributes= but by puppetclass_ids=. In this case we don't need this (and other has_many associations) to resolve templates so we can ignore these. The view renders only the templates partial so form is not touched (unlike process_taxonomy action which renders the whole form). I added also config_group_ids which is part of params. To be more safe for associations added in future we could ignore all /.*_ids$/ keys, not sure if it's any better.
There was a problem hiding this comment.
To be more safe for associations added in future we could ignore all /.*_ids$/ keys, not sure if it's any better.
This might be safer in case plugins are involved - if the host form has been extended any to include nested attributes, then plugin authors aren't going to have much control over the .except() call in the line below. It might be safer for everybody to filter them out by default.
Would using .readonly help?
There was a problem hiding this comment.
Modified to filter all *_ids paramteres and added extra comment about it.
Would using .readonly help?
No 😢, readonly seems to be effective only on the object itself, associations ignore it.
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
@ares could you rebase this please? |
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
sure thing, rebased |
|
Thanks. I've been going through the history and code and agree, this seems to resolve many of the related issues in a decent way. Can I suggest adding a couple of expectations to those functional tests, which would fail if AR tried to make any changes to an associated object during what should be a no-op call? These should be on all three tests for template_used and process_taxonomy. |
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
Sounds as a good idea, added. |
| @@ -878,16 +878,20 @@ class Host::Valid < Host::Managed; end | |||
| end | |||
|
|
|||
| test 'template_used returns templates with interfaces' do | |||
| ActiveRecord::Base.any_instance.expects(:destroy).never | |||
There was a problem hiding this comment.
I think these will fail, they'll have to be lower than the host/NIC creation.
There was a problem hiding this comment.
well even if it wouldn't fail it's probably safer to stub just before xhr, thanks
This is alternative approach to fix #11767 which avoids removing params. It's covered by tests which I had to fix, they sent attributes that do not come from form. Also it's needed to use existing host during editing so NICs sent as nested attributes are correctly matched.
Before testing, please read 8405 and 9058 as well as 11767 and 11768 since they are all related and should be fixed by this patch.