Fixes #11350 - Duplicate parameters can be created within subtype

[dLobatog]

Parameter class leaves its :name uniqueness validation to subtypes such as
CommonParameter, GroupParameter, and so forth. This means that one can
trivially create duplicate parameters by calling Parameter.new, then changing
the type attribute of the newly created object.

To avoid this situation (and remove some duplicated validations) we can simply
declare a default validation on all Parameter names scoped by reference_id, and
override it for CommonParameter which doesn't have a reference_id.

[domcleal]

I couldn't reproduce the issue you reported at #2600 (comment), because the unique index catches it.

   (0.1ms)  begin transaction
  SQL (0.6ms)  INSERT INTO "parameters" ("created_at", "hidden_value", "name", "priority", "reference_id", "type", "updated_at", "value") VALUES (?, ?, ?, ?, ?, ?, ?, ?)  [["created_at", Fri, 14 Aug 2015 06:49:35 UTC +00:00], ["hidden_value", false], ["name", "foo"], ["priority", nil], ["reference_id", 2], ["type", "DomainParameter"], ["updated_at", Fri, 14 Aug 2015 06:49:35 UTC +00:00], ["value", nil]]
   (27.4ms)  commit transaction
   (0.1ms)  begin transaction
  SQL (0.4ms)  INSERT INTO "parameters" ("created_at", "hidden_value", "name", "priority", "reference_id", "type", "updated_at", "value") VALUES (?, ?, ?, ?, ?, ?, ?, ?)  [["created_at", Fri, 14 Aug 2015 06:49:35 UTC +00:00], ["hidden_value", false], ["name", "foo"], ["priority", nil], ["reference_id", 2], ["type", "DomainParameter"], ["updated_at", Fri, 14 Aug 2015 06:49:35 UTC +00:00], ["value", nil]]
2015-08-14T07:49:35 [sql] [D] SQLite3::ConstraintException: UNIQUE constraint failed: parameters.type, parameters.reference_id, parameters.name: INSERT INTO "parameters" ("created_at", "hidden_value", "name", "priority", "reference_id", "type", "updated_at", "value") VALUES (?, ?, ?, ?, ?, ?, ?, ?)
   (0.1ms)  rollback transaction
ActiveRecord::StatementInvalid: SQLite3::ConstraintException: UNIQUE constraint failed: parameters.type, parameters.reference_id, parameters.name: INSERT INTO "parameters" ("created_at", "hidden_value", "name", "priority", "reference_id", "type", "updated_at", "value") VALUES (?, ?, ?, ?, ?, ?, ?, ?)

I don't think creating parameters via the parent class and changing the STI 'type' column is correct usage. Perhaps you'd use .becomes or something to change the class, as we probably have this issue in a lot of other STI based models - you rely on having the correct subclass loaded for more than just uniqueness validations.

Oh yes, we also have Foreman::STI which can interpret :type in a .new call, so if that was added to Parameter then you could even do Parameter.new(:type => 'DomainParameter') and you'd actually get a DomainParameter back.

[domcleal]

Would uniqueness scope include :type as well? Putting it in the subclass would do this automatically.

[dLobatog]

@domcleal Updated, now it takes into account type in the uniqueness validator. I've also added that to the test.

I don't think it's correct usage either but creating parameters through Parameter.new could happen at some point. Probably something is iffy with my environment, as not only my development db does contain the index but as non_unique (unique doesn't show up on my schema.rb nor when I run SHOW INDEX FROM parameters on mysql), but also when I ran tests with a mint db it seems not to create the index properly either. I'll keep digging if this is something wrong with my env or Foreman.

The patch doesn't provide much if the unique index exists and works, feel free to disregard if you feel the scant LOC savings are not worth the hassle 😄

[domcleal]

Strange that you're missing unique, I can't think why that'd be. We have had bugs such as #8366 so have recreated the index, maybe something went wrong then.

The proposed change does make me somewhat uncomfortable, especially as in #2563 we go in the opposite direction with a different parameter validator by moving them from the base class into the subclasses! (We did this to run validation correctly when the referenced model was unsaved, i.e. when cloning, since reference_id wouldn't be filled in yet.)

[dLobatog]

Alright, will close here & in Redmine, reopen at will.