Story #10538

[API] Implement new API server methods to support collection deletion using trash_at and delete_at

Added by Tom Morris almost 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
11/15/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
3.0

Description

Three pieces:

- Trash now, delete at earliest permitted time (possibly now) - using HTTP "DELETE" verb
- Trash now - new API, /arvados/v1/collections/{uuid}/trash
- Trash in the future - generic "update" API

Validate and enforce constraints in the model (see Expiring Collections).

Currently, to ensure permission signatures remain valid until their expiry times, delete_at cannot be changed to earlier than max(transaction_time + blobSignatureTTL, trash_at).

Respect include_trash flag.

Use include_trash flag in keep-balance.

When generating collection API responses, Keep signatures should expire no later than trash time.


Subtasks

Task #10738: Review 10538-trash-deleteResolvedTom Clegg

Associated revisions

Revision a12864a3
Added by Tom Clegg almost 5 years ago

Merge branch '10538-trash-delete' closes #10538

History

#1 Updated by Tom Morris almost 5 years ago

  • Subject changed from [API] Implement new API server methods to [API] Implement new API server methods to support collection deletion

#2 Updated by Tom Clegg almost 5 years ago

  • Description updated (diff)

#3 Updated by Tom Morris almost 5 years ago

  • Story points set to 3.0

#4 Updated by Tom Clegg almost 5 years ago

  • Subject changed from [API] Implement new API server methods to support collection deletion to [API] Implement new API server methods to support collection deletion using trash_at and delete_at

#5 Updated by Tom Clegg almost 5 years ago

  • Description updated (diff)

#6 Updated by Tom Morris almost 5 years ago

  • Target version set to Arvados Future Sprints

#7 Updated by Tom Morris almost 5 years ago

  • Assigned To set to Tom Clegg
  • Target version changed from Arvados Future Sprints to 2017-01-04 sprint

#8 Updated by Tom Clegg almost 5 years ago

  • Description updated (diff)

#10 Updated by Lucas Di Pentima almost 5 years ago

  • services/api/db/migrate/20161213172944_full_text_search_indexes.rb
    • I think it's not convenient to modify old migration files because those servers that already applied the migration won’t get the update.
  • services/api/app/models/collection.rb
    • Line 241: should @validation_timestamp be used to get the expiration timestamp?
    • Line 450: Isn’t ‘|| false’ unnecessary?
    • Line 456: I find the repeated conditionals a little difficult to read, do you think this would be clearer?
      def default_trash_interval
        if trash_at_changed? && !delete_at_changed?
          self.delete_at = trash_at.nil? ? nil : trash_at + Rails.configuration.default_trash_lifetime.seconds
        end
      end
      
    • Line 465, 466: If trash_at is nil and delete_at is not nil, then the error message is confusing, because (at least for me) it only mentions the case for when delete_at is nil. Maybe change it to “delete_at should be set when trash_at is set, and should be nil otherwise”?
  • sdk/go/arvados/collection.go
    • Line 24: Should this be IsTrashed/is_trashed instead of IsTrash/is_trash?
  • I think it would be clearer if in the tests instead of referring to the fixture collections by uuid, we do it using their fixture label. If the issue is that the default scope avoid this, maybe some comment besides the UUID describing what type of collection is being referred to can be helpful to improve readability.
    • services/api/test/functional/arvados/v1/collections_controller_test.rb Lines 978, 979, 936, 946, 967
    • services/api/test/unit/collection_test.rb Lines 473, 492

#11 Updated by Tom Clegg almost 5 years ago

Lucas Di Pentima wrote:

  • services/api/db/migrate/20161213172944_full_text_search_indexes.rb
    • I think it's not convenient to modify old migration files because those servers that already applied the migration won’t get the update.

I'm assuming you mean 924b783e07028b17cd5403205361e4715f7a212f although it's not on this branch ... this fixed a bug where an infrequently-updated system running a lot of migrations at once would use an outdated schema in full_text_tsvector. I suppose the best fix would be to add a new migration that checks whether the indexes are correct, and re-runs the 20161213 migration if not?

  • services/api/app/models/collection.rb
    • Line 241: should @validation_timestamp be used to get the expiration timestamp?

I considered doing this to avoid the extra query, but validation isn't a prerequisite to calling signed_manifest_text -- e.g., collections#show would call signed_manifest_text without a validation.

  • Line 450: Isn’t ‘|| false’ unnecessary?

Strangely, no: trash_at might be nil, and "nil && false" is nil (which makes some sense since you don't need to evaluate "x" in "nil && x")... so '|| false' is necessary to ensure self.is_trashed is either true or false.

  • Line 456: I find the repeated conditionals a little difficult to read, do you think this would be clearer?
    [...]

Yes, updated.

  • Line 465, 466: If trash_at is nil and delete_at is not nil, then the error message is confusing, because (at least for me) it only mentions the case for when delete_at is nil. Maybe change it to “delete_at should be set when trash_at is set, and should be nil otherwise”?

Yes, updated (but with "must" instead of "should").

  • sdk/go/arvados/collection.go
    • Line 24: Should this be IsTrashed/is_trashed instead of IsTrash/is_trash?

Yes, oops.

  • I think it would be clearer if in the tests instead of referring to the fixture collections by uuid, we do it using their fixture label. If the issue is that the default scope avoid this, maybe some comment besides the UUID describing what type of collection is being referred to can be helpful to improve readability.
    • services/api/test/functional/arvados/v1/collections_controller_test.rb Lines 978, 979, 936, 946, 967
    • services/api/test/unit/collection_test.rb Lines 473, 492

Indeed, updated. Added comments about default scope in the fixture file.

#12 Updated by Lucas Di Pentima almost 5 years ago

Thanks for the updates on fixture recalling, it's a lot clearer now and will help us as a reminder for future test writings. I searched if there was some way to tell the test suite to get unscoped fixtures but couldn't find any reference about that.
Please check services/api/test/unit/collection_test.rb:473, I suppose that commented line is not needed anymore.
With that, lgtm.

#13 Updated by Tom Clegg almost 5 years ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:a12864a31d5569c74ed32157d5fe928a1c2563b7.

Also available in: Atom PDF