Story #2762

Protect database integrity wrt owner_uuid

Added by Tom Clegg over 7 years ago. Updated over 7 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Start date:
04/21/2014
Due date:
% Done:

100%

Estimated time:
(Total: 5.00 h)
Story points:
1.0

Subtasks

Task #2731: Refuse to delete an object that owns other objects.ResolvedTom Clegg

Task #2730: Refuse to delete an object that is referenced by links.ResolvedTom Clegg

Task #2807: Review 2762-owner-uuid-integrityResolvedTom Clegg

Associated revisions

Revision ea9592e9
Added by Tom Clegg over 7 years ago

Merge branch '2762-owner-uuid-integrity'

closes #2762

Revision 057f835f (diff)
Added by Tom Clegg over 7 years ago

2762: Do not try to auto-destroy referring links when destroying a model with no uuid. refs #2762

History

#1 Updated by Tom Clegg over 7 years ago

  • Target version changed from 2014-05-07 Storing and Organizing Data to 2014-05-28 Pipeline Factory

#2 Updated by Tom Clegg over 7 years ago

  • Assigned To set to Tom Clegg

#3 Updated by Tom Clegg over 7 years ago

  • Status changed from New to In Progress

#4 Updated by Brett Smith over 7 years ago

Should test "change owner from legit #{o_class} to legit #{new_o_class} owner" say new_o = new_o_class.create?

If possible, I think it might improve readability if destroy_permission_links and its callback declaration lived in the same place. In other words, either define destroy_permission_links in the HasUuid module, or declare its before_destroy callback in ArvadosModel.

Heads-up: Rails 4 split has_many(dependent: :restrict) into two separate values. Might be good to figure out which one you prefer and be ready to make the switch when the time comes.

There's trailing whitespace in can_be_an_owner.rb. (That header comment that just says "Protect" is also a little enigmatic. :) )

Thanks.

#5 Updated by Tom Clegg over 7 years ago

Brett Smith wrote:

Should test "change owner from legit #{o_class} to legit #{new_o_class} owner" say new_o = new_o_class.create?

Yes, fixed.

If possible, I think it might improve readability if destroy_permission_links and its callback declaration lived in the same place. In other words, either define destroy_permission_links in the HasUuid module, or declare its before_destroy callback in ArvadosModel.

Yes, fixed (moved to HasUuid).

Heads-up: Rails 4 split has_many(dependent: :restrict) into two separate values. Might be good to figure out which one you prefer and be ready to make the switch when the time comes.

Saw that. Unfortunately it seems there's no (documented) option that works in both rails3 and rails4 so we just have to notice that tests are failing in rails4, and change to :restrict_with_exception.

There's trailing whitespace in can_be_an_owner.rb. (That header comment that just says "Protect" is also a little enigmatic. :) )

Ah yes. Wrote the rest of the sentence.

Now @ fb8dd22

#6 Updated by Brett Smith over 7 years ago

Tom Clegg wrote:

Brett Smith wrote:

Heads-up: Rails 4 split has_many(dependent: :restrict) into two separate values. Might be good to figure out which one you prefer and be ready to make the switch when the time comes.

Saw that. Unfortunately it seems there's no (documented) option that works in both rails3 and rails4 so we just have to notice that tests are failing in rails4, and change to :restrict_with_exception.

That sounds good to me. I thought it would be good to have a migration plan in mind, but I'm fine with something this rare being manual. Everything looks good to me; please go ahead and merge this. Thanks.

#7 Updated by Anonymous over 7 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 80 to 100

Applied in changeset arvados|commit:ea9592e911a3ccdbbbfeed8812b67e968fe1cc5f.

Also available in: Atom PDF