10538: Exchange expires_at for new trash_at and delete_at columns.
authorTom Clegg <tom@curoverse.com>
Tue, 27 Dec 2016 18:57:53 +0000 (13:57 -0500)
committerTom Clegg <tom@curoverse.com>
Wed, 28 Dec 2016 02:36:13 +0000 (21:36 -0500)
15 files changed:
apps/workbench/app/controllers/projects_controller.rb
apps/workbench/test/controllers/projects_controller_test.rb
sdk/go/arvados/collection.go
services/api/app/controllers/arvados/v1/collections_controller.rb
services/api/app/models/arvados_model.rb
services/api/app/models/collection.rb
services/api/config/application.default.yml
services/api/db/migrate/20161222153434_split_expiry_to_trash_and_delete.rb [new file with mode: 0644]
services/api/db/structure.sql
services/api/lib/sweep_trashed_collections.rb [new file with mode: 0644]
services/api/test/fixtures/collections.yml
services/api/test/functional/arvados/v1/collections_controller_test.rb
services/api/test/unit/collection_test.rb
services/arv-web/arv-web.py
services/crunch-run/crunchrun.go

index 48b2c421fb08385e648d95ba8cac809e948eb7b8..273f9d0c8063c178489ca93972cf64c0b39bbc18 100644 (file)
@@ -149,10 +149,10 @@ class ProjectsController < ApplicationController
         link.destroy
       end
 
-      # If this object has the 'expires_at' attribute, then simply mark it
-      # expired.
-      if item.attributes.include?("expires_at")
-        item.update_attributes expires_at: Time.now
+      # If this object has the 'trash_at' attribute, then simply mark it
+      # as trash.
+      if item.attributes.include?("trash_at")
+        item.update_attributes trash_at: Time.now
         @removed_uuids << item.uuid
       elsif item.owner_uuid == @object.uuid
         # Object is owned by this project. Remove it from the project by
@@ -161,7 +161,7 @@ class ProjectsController < ApplicationController
           item.update_attributes owner_uuid: current_user.uuid
           @removed_uuids << item.uuid
         rescue ArvadosApiClient::ApiErrorResponseException => e
-          if e.message.include? '_owner_uuid_name_unique'
+          if e.message.include? '_owner_uuid_'
             rename_to = item.name + ' removed from ' +
                         (@object.name ? @object.name : @object.uuid) +
                         ' at ' + Time.now.to_s
index d0b1e287ff6c26f923c319f55c8cf30039ca24bf..1b19f2f4936a6cfd8c3b82cd1f8ebaaf32db49fb 100644 (file)
@@ -101,8 +101,9 @@ class ProjectsControllerTest < ActionController::TestCase
   end
 
   test "project admin can remove collections from the project" do
-    # Deleting an object that supports 'expires_at' should make it
-    # completely inaccessible to API queries, not simply moved out of the project.
+    # Deleting an object that supports 'trash_at' should make it
+    # completely inaccessible to API queries, not simply moved out of
+    # the project.
     coll_key = "collection_to_remove_from_subproject"
     coll_uuid = api_fixture("collections")[coll_key]["uuid"]
     delete(:remove_item,
@@ -116,12 +117,12 @@ class ProjectsControllerTest < ActionController::TestCase
 
     use_token :subproject_admin
     assert_raise ArvadosApiClient::NotFoundException do
-      Collection.find(coll_uuid)
+      Collection.find(coll_uuid, cache: false)
     end
   end
 
   test "project admin can remove items from project other than collections" do
-    # An object which does not have an expired_at field (e.g. Specimen)
+    # An object which does not have an trash_at field (e.g. Specimen)
     # should be implicitly moved to the user's Home project when removed.
     specimen_uuid = api_fixture('specimens', 'in_asubproject')['uuid']
     delete(:remove_item,
index 71f52476153bc4ecea1fbf26e3830dde6f2cdf2b..2090f56846d00afd6a82cd637a58f89d93196445 100644 (file)
@@ -12,7 +12,7 @@ import (
 // Collection is an arvados#collection resource.
 type Collection struct {
        UUID                   string     `json:"uuid,omitempty"`
-       ExpiresAt              *time.Time `json:"expires_at,omitempty"`
+       TrashAt                *time.Time `json:"trash_at,omitempty"`
        ManifestText           string     `json:"manifest_text,omitempty"`
        CreatedAt              *time.Time `json:"created_at,omitempty"`
        ModifiedAt             *time.Time `json:"modified_at,omitempty"`
index 017c023db2ad1363fdd8549040bc402e17dce59f..33b7a2dd4eecf86400803ebbcc841f1f2719654f 100644 (file)
@@ -1,6 +1,8 @@
 require "arvados/keep"
 
 class Arvados::V1::CollectionsController < ApplicationController
+  include DbCurrentTime
+
   def self.limit_index_columns_read
     ["manifest_text"]
   end
@@ -13,6 +15,13 @@ class Arvados::V1::CollectionsController < ApplicationController
     super
   end
 
+  def find_objects_for_index
+    if params[:include_trash] || action_name == 'destroy'
+      @objects = Collection.unscoped.readable_by(*@read_users)
+    end
+    super
+  end
+
   def find_object_by_uuid
     if loc = Keep::Locator.parse(params[:id])
       loc.strip_hints!
@@ -23,10 +32,10 @@ class Arvados::V1::CollectionsController < ApplicationController
           manifest_text: c.signed_manifest_text,
         }
       end
+      true
     else
       super
     end
-    true
   end
 
   def show
@@ -37,6 +46,18 @@ class Arvados::V1::CollectionsController < ApplicationController
     end
   end
 
+  def destroy
+    if !@object.is_trashed
+      @object.update_attributes!(trash_at: db_current_time)
+    end
+    earliest_delete = (@object.trash_at +
+                       Rails.configuration.blob_signature_ttl.seconds)
+    if @object.delete_at > earliest_delete
+      @object.update_attributes!(delete_at: earliest_delete)
+    end
+    show
+  end
+
   def find_collections(visited, sp, &b)
     case sp
     when ArvadosModel
index 910db7e0c7bad2bb0a38ebbc8d0b02a40b63561f..fd542ca909e296c1284b06a2843b9859c6acc8d6 100644 (file)
@@ -597,7 +597,7 @@ class ArvadosModel < ActiveRecord::Base
     if self == ArvadosModel
       # If called directly as ArvadosModel.find_by_uuid rather than via subclass,
       # delegate to the appropriate subclass based on the given uuid.
-      self.resource_class_for_uuid(uuid).find_by_uuid(uuid)
+      self.resource_class_for_uuid(uuid).unscoped.find_by_uuid(uuid)
     else
       super
     end
index 901084c7636a61496944b4c18616e2d79c3508e5..93d46209276c95a2cc3544ee0c82f437aec6943c 100644 (file)
@@ -1,4 +1,5 @@
 require 'arvados/keep'
+require 'sweep_trashed_collections'
 
 class Collection < ArvadosModel
   extend DbCurrentTime
@@ -8,17 +9,21 @@ class Collection < ArvadosModel
 
   serialize :properties, Hash
 
+  before_validation :set_validation_timestamp
   before_validation :default_empty_manifest
   before_validation :check_encoding
   before_validation :check_manifest_validity
   before_validation :check_signatures
   before_validation :strip_signatures_and_update_replication_confirmed
+  before_validation :ensure_trash_at_not_in_past
+  before_validation :sync_trash_state
+  before_validation :default_trash_interval
   validate :ensure_pdh_matches_manifest_text
+  validate :validate_trash_and_delete_timing
   before_save :set_file_names
-  before_save :expires_at_not_in_past
 
-  # Query only undeleted collections by default.
-  default_scope where("expires_at IS NULL or expires_at > statement_timestamp()")
+  # Query only untrashed collections by default.
+  default_scope where("is_trashed = false")
 
   api_accessible :user, extend: :common do |t|
     t.add :name
@@ -29,7 +34,9 @@ class Collection < ArvadosModel
     t.add :replication_desired
     t.add :replication_confirmed
     t.add :replication_confirmed_at
-    t.add :expires_at
+    t.add :delete_at
+    t.add :trash_at
+    t.add :is_trashed
   end
 
   after_initialize do
@@ -45,9 +52,9 @@ class Collection < ArvadosModel
                 # API response, and never let clients select the
                 # manifest_text column.
                 #
-                # We need expires_at to determine the correct
-                # timestamp in signed_manifest_text.
-                'manifest_text' => ['manifest_text', 'expires_at'],
+                # We need trash_at and is_trashed to determine the
+                # correct timestamp in signed_manifest_text.
+                'manifest_text' => ['manifest_text', 'trash_at', 'is_trashed'],
                 )
   end
 
@@ -77,7 +84,7 @@ class Collection < ArvadosModel
       api_token = current_api_client_authorization.andand.api_token
       signing_opts = {
         api_token: api_token,
-        now: db_current_time.to_i,
+        now: @validation_timestamp.to_i,
       }
       self.manifest_text.each_line do |entry|
         entry.split.each do |tok|
@@ -225,11 +232,15 @@ class Collection < ArvadosModel
   end
 
   def signed_manifest_text
-    if has_attribute? :manifest_text
+    if !has_attribute? :manifest_text
+      return nil
+    elsif is_trashed
+      return manifest_text
+    else
       token = current_api_client_authorization.andand.api_token
       exp = [db_current_time.to_i + Rails.configuration.blob_signature_ttl,
-             expires_at].compact.map(&:to_i).min
-      @signed_manifest_text = self.class.sign_manifest manifest_text, token, exp
+             trash_at].compact.map(&:to_i).min
+      self.class.sign_manifest manifest_text, token, exp
     end
   end
 
@@ -367,6 +378,11 @@ class Collection < ArvadosModel
     super - ["manifest_text"]
   end
 
+  def self.where *args
+    SweepTrashedCollections.sweep_if_stale
+    super
+  end
+
   protected
   def portable_manifest_text
     self.class.munge_manifest_locators(manifest_text) do |match|
@@ -403,13 +419,63 @@ class Collection < ArvadosModel
     super
   end
 
-  # If expires_at is being changed to a time in the past, change it to
+  # Use a single timestamp for all validations, even though each
+  # validation runs at a different time.
+  def set_validation_timestamp
+    @validation_timestamp = db_current_time
+  end
+
+  # If trash_at is being changed to a time in the past, change it to
   # now. This allows clients to say "expires {client-current-time}"
   # without failing due to clock skew, while avoiding odd log entries
   # like "expiry date changed to {1 year ago}".
-  def expires_at_not_in_past
-    if expires_at_changed? and expires_at
-      self.expires_at = [db_current_time, expires_at].max
+  def ensure_trash_at_not_in_past
+    if trash_at_changed? && trash_at
+      self.trash_at = [@validation_timestamp, trash_at].max
+    end
+  end
+
+  # Caller can move into/out of trash by setting/clearing is_trashed
+  # -- however, if the caller also changes trash_at, then any changes
+  # to is_trashed are ignored.
+  def sync_trash_state
+    if is_trashed_changed? && !trash_at_changed?
+      if is_trashed
+        self.trash_at = @validation_timestamp
+      else
+        self.trash_at = nil
+        self.delete_at = nil
+      end
+    end
+    self.is_trashed = trash_at && trash_at <= @validation_timestamp || false
+    true
+  end
+
+  # If trash_at is updated without touching delete_at, automatically
+  # update delete_at to a sensible value.
+  def default_trash_interval
+    if trash_at && trash_at_changed? && !delete_at_changed?
+      self.delete_at = trash_at + Rails.configuration.default_trash_lifetime.seconds
+    elsif trash_at.nil? && trash_at_changed? && !delete_at_changed?
+      self.delete_at = nil
+    end
+  end
+
+  def validate_trash_and_delete_timing
+    if trash_at.nil? != delete_at.nil?
+      errors.add :delete_at, "must be nil if and only if trash_at is nil"
     end
+
+    earliest_delete = ([@validation_timestamp, trash_at_was].compact.min +
+                       Rails.configuration.blob_signature_ttl.seconds)
+    if delete_at && delete_at < earliest_delete
+      errors.add :delete_at, "#{delete_at} is too soon: earliest allowed is #{earliest_delete}"
+    end
+
+    if delete_at && delete_at < trash_at
+      errors.add :delete_at, "must not be earlier than trash_at"
+    end
+
+    true
   end
 end
index c8d2d1d6f1e6665b5b5b67dbd028a9be049248fc..bb1355d030a74fa4594ea1dee54ef1a91ef70ef2 100644 (file)
@@ -180,9 +180,15 @@ common:
   # The default is 2 weeks.
   blob_signature_ttl: 1209600
 
-  # Default lifetime for ephemeral collections: 2 weeks.
+  # Default lifetime for ephemeral collections: 2 weeks. This must not
+  # be less than blob_signature_ttl.
   default_trash_lifetime: 1209600
 
+  # Interval (seconds) between trash sweeps. During a trash sweep,
+  # collections are marked as trash if their trash_at time has
+  # arrived, and deleted if their delete_at time has arrived.
+  trash_sweep_interval: 60
+
   # Maximum characters of (JSON-encoded) query parameters to include
   # in each request log entry. When params exceed this size, they will
   # be JSON-encoded, truncated to this size, and logged as
@@ -445,3 +451,4 @@ test:
   git_repositories_dir: <%= Rails.root.join 'tmp', 'git', 'test' %>
   git_internal_dir: <%= Rails.root.join 'tmp', 'internal.git' %>
   websocket_address: <% if ENV['ARVADOS_TEST_EXPERIMENTAL_WS'] %>"wss://0.0.0.0:<%= ENV['ARVADOS_TEST_WSS_PORT'] %>/websocket"<% else %>false<% end %>
+  trash_sweep_interval: -1
diff --git a/services/api/db/migrate/20161222153434_split_expiry_to_trash_and_delete.rb b/services/api/db/migrate/20161222153434_split_expiry_to_trash_and_delete.rb
new file mode 100644 (file)
index 0000000..13e4419
--- /dev/null
@@ -0,0 +1,42 @@
+class SplitExpiryToTrashAndDelete < ActiveRecord::Migration
+  def up
+    Collection.transaction do
+      add_column(:collections, :trash_at, :datetime)
+      add_index(:collections, :trash_at)
+      add_column(:collections, :is_trashed, :boolean, null: false, default: false)
+      add_index(:collections, :is_trashed)
+      rename_column(:collections, :expires_at, :delete_at)
+      add_index(:collections, :delete_at)
+
+      Collection.reset_column_information
+      Collection.
+        where('delete_at is not null and delete_at <= statement_timestamp()').
+        delete_all
+      Collection.
+        where('delete_at is not null').
+        update_all('is_trashed = true, trash_at = statement_timestamp()')
+      add_index(:collections, [:owner_uuid, :name],
+                unique: true,
+                where: 'is_trashed = false',
+                name: 'index_collections_on_owner_uuid_and_name')
+      remove_index(:collections,
+                   name: 'collection_owner_uuid_name_unique')
+    end
+  end
+
+  def down
+    Collection.transaction do
+      remove_index(:collections, :delete_at)
+      rename_column(:collections, :delete_at, :expires_at)
+      add_index(:collections, [:owner_uuid, :name],
+                unique: true,
+                where: 'expires_at is null',
+                name: 'collection_owner_uuid_name_unique')
+      remove_index(:collections,
+                   name: 'index_collections_on_owner_uuid_and_name')
+      remove_column(:collections, :is_trashed)
+      remove_index(:collections, :trash_at)
+      remove_column(:collections, :trash_at)
+    end
+  end
+end
index 7ee7ea6ff98fbd1845eed558737cf16c557343ce..6ef03cff11aed3b7dba49fa5445c09749ac0930d 100644 (file)
@@ -169,8 +169,10 @@ CREATE TABLE collections (
     name character varying(255),
     description character varying(524288),
     properties text,
-    expires_at timestamp without time zone,
-    file_names character varying(8192)
+    delete_at timestamp without time zone,
+    file_names character varying(8192),
+    trash_at timestamp without time zone,
+    is_trashed boolean DEFAULT false NOT NULL
 );
 
 
@@ -1495,13 +1497,6 @@ CREATE INDEX api_clients_search_index ON api_clients USING btree (uuid, owner_uu
 CREATE INDEX authorized_keys_search_index ON authorized_keys USING btree (uuid, owner_uuid, modified_by_client_uuid, modified_by_user_uuid, name, key_type, authorized_user_uuid);
 
 
---
--- Name: collection_owner_uuid_name_unique; Type: INDEX; Schema: public; Owner: -; Tablespace: 
---
-
-CREATE UNIQUE INDEX collection_owner_uuid_name_unique ON collections USING btree (owner_uuid, name) WHERE (expires_at IS NULL);
-
-
 --
 -- Name: collections_full_text_search_idx; Type: INDEX; Schema: public; Owner: -; Tablespace: 
 --
@@ -1656,6 +1651,20 @@ CREATE UNIQUE INDEX index_authorized_keys_on_uuid ON authorized_keys USING btree
 CREATE INDEX index_collections_on_created_at ON collections USING btree (created_at);
 
 
+--
+-- Name: index_collections_on_delete_at; Type: INDEX; Schema: public; Owner: -; Tablespace: 
+--
+
+CREATE INDEX index_collections_on_delete_at ON collections USING btree (delete_at);
+
+
+--
+-- Name: index_collections_on_is_trashed; Type: INDEX; Schema: public; Owner: -; Tablespace: 
+--
+
+CREATE INDEX index_collections_on_is_trashed ON collections USING btree (is_trashed);
+
+
 --
 -- Name: index_collections_on_modified_at; Type: INDEX; Schema: public; Owner: -; Tablespace: 
 --
@@ -1670,6 +1679,20 @@ CREATE INDEX index_collections_on_modified_at ON collections USING btree (modifi
 CREATE INDEX index_collections_on_owner_uuid ON collections USING btree (owner_uuid);
 
 
+--
+-- Name: index_collections_on_owner_uuid_and_name; Type: INDEX; Schema: public; Owner: -; Tablespace: 
+--
+
+CREATE UNIQUE INDEX index_collections_on_owner_uuid_and_name ON collections USING btree (owner_uuid, name) WHERE (is_trashed = false);
+
+
+--
+-- Name: index_collections_on_trash_at; Type: INDEX; Schema: public; Owner: -; Tablespace: 
+--
+
+CREATE INDEX index_collections_on_trash_at ON collections USING btree (trash_at);
+
+
 --
 -- Name: index_collections_on_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace: 
 --
@@ -2706,4 +2729,6 @@ INSERT INTO schema_migrations (version) VALUES ('20161115171221');
 
 INSERT INTO schema_migrations (version) VALUES ('20161115174218');
 
-INSERT INTO schema_migrations (version) VALUES ('20161213172944');
\ No newline at end of file
+INSERT INTO schema_migrations (version) VALUES ('20161213172944');
+
+INSERT INTO schema_migrations (version) VALUES ('20161222153434');
\ No newline at end of file
diff --git a/services/api/lib/sweep_trashed_collections.rb b/services/api/lib/sweep_trashed_collections.rb
new file mode 100644 (file)
index 0000000..ab2d27a
--- /dev/null
@@ -0,0 +1,34 @@
+require 'current_api_client'
+
+module SweepTrashedCollections
+  extend CurrentApiClient
+
+  def self.sweep_now
+    act_as_system_user do
+      Collection.unscoped.
+        where('delete_at is not null and delete_at < statement_timestamp()').
+        destroy_all
+      Collection.unscoped.
+        where('is_trashed = false and trash_at < statement_timestamp()').
+        update_all('is_trashed = true')
+    end
+  end
+
+  def self.sweep_if_stale
+    return if Rails.configuration.trash_sweep_interval <= 0
+    exp = Rails.configuration.trash_sweep_interval.seconds
+    need = false
+    Rails.cache.fetch('SweepTrashedCollections', expires_in: exp) do
+      need = true
+    end
+    if need
+      Thread.new do
+        begin
+          sweep_now
+        ensure
+          ActiveRecord::Base.connection.close
+        end
+      end
+    end
+  end
+end
index 2272b0f4a041094455c6a06a979a0ed4947f531a..192ed17057a001c89cbb94539075cd77e078049c 100644 (file)
@@ -230,10 +230,42 @@ expired_collection:
   modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
   modified_at: 2014-02-03T17:22:54Z
   updated_at: 2014-02-03T17:22:54Z
-  expires_at: 2001-01-01T00:00:00Z
+  is_trashed: true
+  trash_at: 2001-01-01T00:00:00Z
+  delete_at: 2038-01-01T00:00:00Z
   manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:expired\n"
   name: expired_collection
 
+trashed_on_next_sweep:
+  uuid: zzzzz-4zz18-4guozfh77ewd2f0
+  portable_data_hash: 0b21a217243bfce5617fb9224b95bcb9+49
+  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  created_at: 2016-12-07T22:01:00.123456Z
+  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+  modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
+  modified_at: 2016-12-27T22:01:30.123456Z
+  updated_at: 2016-12-27T22:01:30.123456Z
+  is_trashed: false
+  trash_at: 2016-12-07T22:01:30.123456Z
+  delete_at: 2112-01-01T00:00:00Z
+  manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:expired\n"
+  name: trashed_on_next_sweep
+
+deleted_on_next_sweep:
+  uuid: zzzzz-4zz18-3u1p5umicfpqszp
+  portable_data_hash: 0b21a217243bfce5617fb9224b95bcb9+49
+  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  created_at: 2016-12-07T22:01:00.234567Z
+  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+  modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
+  modified_at: 2016-12-27T22:01:30.234567Z
+  updated_at: 2016-12-27T22:01:30.234567Z
+  is_trashed: true
+  trash_at: 2016-12-07T22:01:30.234567Z
+  delete_at: 2016-12-27T22:01:30.234567Z
+  manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:expired\n"
+  name: deleted_on_next_sweep
+
 collection_expires_in_future:
   uuid: zzzzz-4zz18-padkqo7yb8d9i3j
   portable_data_hash: 0b21a217243bfce5617fb9224b95bcb9+49
@@ -243,7 +275,8 @@ collection_expires_in_future:
   modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
   modified_at: 2014-02-03T17:22:54Z
   updated_at: 2014-02-03T17:22:54Z
-  expires_at: 2038-01-01T00:00:00Z
+  trash_at: 2038-01-01T00:00:00Z
+  delete_at: 2038-03-01T00:00:00Z
   manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:expired\n"
   name: collection_expires_in_future
 
index b96e22ed6583befa82153c623da9bac73014745e..cc759a6c3ec9e646580c7d745debd9e8592c3b3c 100644 (file)
@@ -1,6 +1,7 @@
 require 'test_helper'
 
 class Arvados::V1::CollectionsControllerTest < ActionController::TestCase
+  include DbCurrentTime
 
   def permit_unsigned_manifests isok=true
     # Set security model for the life of a test.
@@ -930,4 +931,48 @@ EOS
       assert_response 200
     end
   end
+
+  test 'get trashed collection with include_trash' do
+    uuid = 'zzzzz-4zz18-mto52zx1s7sn3ih'
+    authorize_with :active
+    get :show, {
+      id: uuid,
+      include_trash: true,
+    }
+    assert_response 200
+  end
+
+  test 'get trashed collection without include_trash' do
+    uuid = 'zzzzz-4zz18-mto52zx1s7sn3ih'
+    authorize_with :active
+    get :show, {
+      id: uuid,
+    }
+    assert_response 404
+  end
+
+  test 'trash collection using http DELETE verb' do
+    uuid = collections(:collection_owned_by_active).uuid
+    authorize_with :active
+    delete :destroy, {
+      id: uuid,
+    }
+    assert_response 200
+    c = Collection.unscoped.find_by_uuid(uuid)
+    assert_operator c.trash_at, :<, db_current_time
+    assert_equal c.delete_at, c.trash_at + Rails.configuration.blob_signature_ttl
+  end
+
+  test 'delete long-trashed collection immediately using http DELETE verb' do
+    uuid = 'zzzzz-4zz18-mto52zx1s7sn3ih'
+    authorize_with :active
+    delete :destroy, {
+      id: uuid,
+    }
+    assert_response 200
+    c = Collection.unscoped.find_by_uuid(uuid)
+    assert_operator c.trash_at, :<, db_current_time
+    assert_operator c.delete_at, :<, db_current_time
+    assert_equal c.delete_at, c.trash_at + Rails.configuration.blob_signature_ttl
+  end
 end
index 1c85a716e3bd34bf269448a455d5d6bef0938408..d1cb9785938bec789c9ff7098d4747005f6b89a0 100644 (file)
@@ -1,4 +1,5 @@
 require 'test_helper'
+require 'sweep_trashed_collections'
 
 class CollectionTest < ActiveSupport::TestCase
   include DbCurrentTime
@@ -307,11 +308,11 @@ class CollectionTest < ActiveSupport::TestCase
     end
   end
 
-  test 'signature expiry does not exceed expires_at' do
+  test 'signature expiry does not exceed trash_at' do
     act_as_user users(:active) do
       t0 = db_current_time
       c = Collection.create!(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:x\n", name: 'foo')
-      c.update_attributes! expires_at: (t0 + 1.hours)
+      c.update_attributes! trash_at: (t0 + 1.hours)
       c.reload
       sig_exp = /\+A[0-9a-f]{40}\@([0-9]+)/.match(c.signed_manifest_text)[1].to_i
       assert_operator sig_exp.to_i, :<=, (t0 + 1.hours).to_i
@@ -322,10 +323,10 @@ class CollectionTest < ActiveSupport::TestCase
     act_as_user users(:active) do
       c = Collection.create!(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:x\n",
                              name: 'foo',
-                             expires_at: db_current_time + 1.years)
+                             trash_at: db_current_time + 1.years)
       sig_exp = /\+A[0-9a-f]{40}\@([0-9]+)/.match(c.signed_manifest_text)[1].to_i
       expect_max_sig_exp = db_current_time.to_i + Rails.configuration.blob_signature_ttl
-      assert_operator c.expires_at.to_i, :>, expect_max_sig_exp
+      assert_operator c.trash_at.to_i, :>, expect_max_sig_exp
       assert_operator sig_exp.to_i, :<=, expect_max_sig_exp
     end
   end
@@ -348,7 +349,7 @@ class CollectionTest < ActiveSupport::TestCase
       uuid = c.uuid
 
       # mark collection as expired
-      c.update_attribute 'expires_at', Time.new.strftime("%Y-%m-%d")
+      c.update_attributes!(trash_at: Time.new.strftime("%Y-%m-%d"))
       c = Collection.where(uuid: uuid)
       assert_empty c, 'Should not be able to find expired collection'
 
@@ -359,6 +360,108 @@ class CollectionTest < ActiveSupport::TestCase
     end
   end
 
+  test 'trash_at cannot be set too far in the past' do
+    act_as_user users(:active) do
+      t0 = db_current_time
+      c = Collection.create!(manifest_text: '', name: 'foo')
+      c.update_attributes! trash_at: (t0 - 2.weeks)
+      c.reload
+      assert_operator c.trash_at, :>, t0
+    end
+  end
+
+  [['trash-to-delete interval negative',
+    :collection_owned_by_active,
+    {trash_at: Time.now+2.weeks, delete_at: Time.now},
+    {state: :invalid}],
+   ['trash-to-delete interval too short',
+    :collection_owned_by_active,
+    {trash_at: Time.now+3.days, delete_at: Time.now+7.days},
+    {state: :invalid}],
+   ['trash-to-delete interval ok',
+    :collection_owned_by_active,
+    {trash_at: Time.now, delete_at: Time.now+15.days},
+    {state: :trash_now}],
+   ['trash-to-delete interval short, but far enough in future',
+    :collection_owned_by_active,
+    {trash_at: Time.now+13.days, delete_at: Time.now+15.days},
+    {state: :trash_future}],
+   ['trash by setting is_trashed bool',
+    :collection_owned_by_active,
+    {is_trashed: true},
+    {state: :trash_now}],
+   ['trash in future by setting just trash_at',
+    :collection_owned_by_active,
+    {trash_at: Time.now+1.week},
+    {state: :trash_future}],
+   ['trash in future by setting trash_at and delete_at',
+    :collection_owned_by_active,
+    {trash_at: Time.now+1.week, delete_at: Time.now+4.weeks},
+    {state: :trash_future}],
+   ['untrash by clearing is_trashed bool',
+    :expired_collection,
+    {is_trashed: false},
+    {state: :not_trash}],
+  ].each do |test_name, fixture_name, updates, expect|
+    test test_name do
+      act_as_user users(:active) do
+        min_exp = (db_current_time +
+                   Rails.configuration.blob_signature_ttl.seconds)
+        if fixture_name == :expired_collection
+          # Fixture-finder shorthand doesn't find trashed collections
+          # because they're not in the default scope.
+          c = Collection.unscoped.find_by_uuid('zzzzz-4zz18-mto52zx1s7sn3ih')
+        else
+          c = collections(fixture_name)
+        end
+        updates_ok = c.update_attributes(updates)
+        expect_valid = expect[:state] != :invalid
+        assert_equal updates_ok, expect_valid, c.errors.full_messages.to_s
+        case expect[:state]
+        when :invalid
+          refute c.valid?
+        when :trash_now
+          assert c.is_trashed
+          assert_not_nil c.trash_at
+          assert_operator c.trash_at, :<=, db_current_time
+          assert_not_nil c.delete_at
+          assert_operator c.delete_at, :>=, min_exp
+        when :trash_future
+          refute c.is_trashed
+          assert_not_nil c.trash_at
+          assert_operator c.trash_at, :>, db_current_time
+          assert_not_nil c.delete_at
+          assert_operator c.delete_at, :>=, c.trash_at
+          # Currently this minimum interval is needed to prevent early
+          # garbage collection:
+          assert_operator c.delete_at, :>=, min_exp
+        when :not_trash
+          refute c.is_trashed
+          assert_nil c.trash_at
+          assert_nil c.delete_at
+        else
+          raise "bad expect[:state]==#{expect[:state].inspect} in test case"
+        end
+      end
+    end
+  end
+
+  test 'default trash interval > blob signature ttl' do
+    Rails.configuration.default_trash_lifetime = 86400 * 21 # 3 weeks
+    start = db_current_time
+    act_as_user users(:active) do
+      c = Collection.create!(manifest_text: '', name: 'foo')
+      c.update_attributes!(trash_at: start + 86400.seconds)
+      assert_operator c.delete_at, :>=, start + (86400*22).seconds
+      assert_operator c.delete_at, :<, start + (86400*22 + 30).seconds
+      c.destroy
+
+      c = Collection.create!(manifest_text: '', name: 'foo')
+      c.update_attributes!(is_trashed: true)
+      assert_operator c.delete_at, :>=, start + (86400*21).seconds
+    end
+  end
+
   test "find_all_for_docker_image resolves names that look like hashes" do
     coll_list = Collection.
       find_all_for_docker_image('a' * 64, nil, [users(:active)])
@@ -366,13 +469,29 @@ class CollectionTest < ActiveSupport::TestCase
     assert_includes(coll_uuids, collections(:docker_image).uuid)
   end
 
-  test 'expires_at cannot be set too far in the past' do
+  test "move to trash in SweepTrashedCollections" do
+    uuid = 'zzzzz-4zz18-4guozfh77ewd2f0'
+    c = Collection.where('uuid=? and is_trashed=false', uuid).first
+    assert c
+    assert_raises(ActiveRecord::RecordNotUnique) do
+      act_as_user users(:active) do
+        Collection.create!(owner_uuid: c.owner_uuid,
+                           name: c.name)
+      end
+    end
+    SweepTrashedCollections.sweep_now
+    c = Collection.unscoped.find_by_uuid(uuid)
+    assert_equal true, c.is_trashed
     act_as_user users(:active) do
-      t0 = db_current_time
-      c = Collection.create!(manifest_text: '', name: 'foo')
-      c.update_attributes! expires_at: (t0 - 2.weeks)
-      c.reload
-      assert_operator c.expires_at, :>, t0
+      assert Collection.create!(owner_uuid: c.owner_uuid,
+                                name: c.name)
     end
   end
+
+  test "delete in SweepTrashedCollections" do
+    uuid = 'zzzzz-4zz18-3u1p5umicfpqszp'
+    assert_not_empty Collection.unscoped.where(uuid: uuid)
+    SweepTrashedCollections.sweep_now
+    assert_empty Collection.unscoped.where(uuid: uuid)
+  end
 end
index 5a95e27b93b26789d10d93f4dadeac849fcfaa9b..f440aa608773de22eb221049993d91b63115edf4 100755 (executable)
@@ -72,7 +72,7 @@ class ArvWeb(object):
                         et = 'add'
                     else:
                         et = 'remove'
-                if ev['properties']['new_attributes']['expires_at'] is not None:
+                if ev['properties']['new_attributes']['trash_at'] is not None:
                     et = 'remove'
 
             self.evqueue.put((self.project, et, ev['object_uuid']))
index e13033edb3920ff9de4c9fa5061e6d54471febfd..971cb3a27a246c9fbe1a325496a6da63e6e69ac4 100644 (file)
@@ -637,7 +637,7 @@ func (runner *ContainerRunner) CaptureOutput() error {
        err = runner.ArvClient.Create("collections",
                arvadosclient.Dict{
                        "collection": arvadosclient.Dict{
-                               "expires_at":    time.Now().Add(runner.trashLifetime).Format(time.RFC3339),
+                               "trash_at":      time.Now().Add(runner.trashLifetime).Format(time.RFC3339),
                                "name":          "output for " + runner.Container.UUID,
                                "manifest_text": manifestText}},
                &response)
@@ -708,7 +708,7 @@ func (runner *ContainerRunner) CommitLogs() error {
        err = runner.ArvClient.Create("collections",
                arvadosclient.Dict{
                        "collection": arvadosclient.Dict{
-                               "expires_at":    time.Now().Add(runner.trashLifetime).Format(time.RFC3339),
+                               "trash_at":      time.Now().Add(runner.trashLifetime).Format(time.RFC3339),
                                "name":          "logs for " + runner.Container.UUID,
                                "manifest_text": mt}},
                &response)