15311: Remove stringify-symbol behavior on database load
authorPeter Amstutz <pamstutz@veritasgenetics.com>
Thu, 6 Jun 2019 15:36:34 +0000 (11:36 -0400)
committerPeter Amstutz <pamstutz@veritasgenetics.com>
Fri, 7 Jun 2019 14:39:08 +0000 (10:39 -0400)
* Add test case demonstrating bug

* Remove convert_serialized_symbols_to_strings hook

* Add "rake symbols:check" to warn you if there are any serialized
columns that may have legacy symbols.

* Add "rake symbols:stringify" to warn you if there are any serialized
columns that may have legacy symbols.

* Add release notes for checking and stringifying symbols.

* Add table of contents to upgrading doc page.

* Add note about the has_symbol_keys_in_database_somehow fixture.

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>

doc/admin/upgrading.html.textile.liquid
services/api/app/models/arvados_model.rb
services/api/lib/tasks/symbols.rake [new file with mode: 0644]
services/api/test/fixtures/links.yml
services/api/test/integration/container_request_test.rb [new file with mode: 0644]
services/api/test/unit/arvados_model_test.rb

index 79e11285725902dba5d3cc895a938f50fdb99817..b25dc10916063b63a3bd54d1255cef35569d53a6 100644 (file)
@@ -30,7 +30,24 @@ Note to developers: Add new items at the top. Include the date, issue number, co
 TODO: extract this information based on git commit messages and generate changelogs / release notes automatically.
 {% endcomment %}
 
-h3. v1.4.0 (2019-06-05)
+table(table table-bordered table-condensed).
+|_. development|"master":#master|\3.|
+|_. latest stable|"v1.4.0":#v1_4_0|\3.|
+|_\5. past stable|
+|"v1.3.3":#v1_3_3|"v1.3.0":#v1_3_0|\3.|
+|"v1.2.1":#v1_2_1|"v1.2.0":#v1_2_0|\3.|
+|"v1.1.4":#v1_1_4|"v1.1.3":#v1_1_3|"v1.1.2":#v1_1_2|"v1.1.1":#v1_1_1|"v1.1.0":#v1_1_0|
+|\5. "older":#older|
+
+h3(#master). development master (as of 2019-06-07)
+
+h4. No longer stripping ':' from strings in serialized database columns
+
+ (bug #15311) Strings read from serialized columns in the database with a leading ':' would have the ':' stripped after loading the record.  This behavior existed due to legacy serialization behavior which stored Ruby symbols with a leading ':'.  Unfortunately this corrupted fields where the leading ":" was intentional.  This behavior has been removed.
+
+You can test if any records in your database are affected by going to the API server directory and running @bundle exec rake symbols:check@.  This will report which records contain fields with a leading ':' that would previously have been stripped.  If there are records to be updated, you can update the database using @bundle exec rake symbols:stringify@.
+
+h3(#v1_4_0). v1.4.0 (2019-06-05)
 
 h4. Populating the new file_count and file_size_total columns on the collections table
 
@@ -135,7 +152,13 @@ h4. New configuration
 
 Arvados is migrating to a centralized configuration file for all components.  During the migration, legacy configuration files will continue to be loaded.  See "Migrating Configuration":config-migration.html for details.
 
-h3. v1.3.0 (2018-12-05)
+h3(#v1_3_3). v1.3.3 (2019-05-14)
+
+This release corrects a potential data loss issue, if you are running Arvados 1.3.0 or 1.3.1 we strongly recommended disabling @keep-balance@ until you can upgrade to 1.3.3 or 1.4.0. With keep-balance disabled, there is no chance of data loss.
+
+We've put together a "wiki page":https://dev.arvados.org/projects/arvados/wiki/Recovering_lost_data which outlines how to recover blocks which have been put in the trash, but not yet deleted, as well as how to identify any collections which have missing blocks so that they can be regenerated. The keep-balance component has been enhanced to provide a list of missing blocks and affected collections and we've provided a "utility script":https://github.com/curoverse/arvados/blob/master/tools/keep-xref/keep-xref.py  which can be used to identify the workflows that generated those collections and who ran those workflows, so that they can be rerun.
+
+h3(#v1_3_0). v1.3.0 (2018-12-05)
 
 This release includes several database migrations, which will be executed automatically as part of the API server upgrade. On large Arvados installations, these migrations will take a while. We've seen the upgrade take 30 minutes or more on installations with a lot of collections.
 
@@ -143,11 +166,11 @@ The @arvados-controller@ component now requires the /etc/arvados/config.yml file
 
 Support for the deprecated "jobs" API is broken in this release.  Users who rely on it should not upgrade.  This will be fixed in an upcoming 1.3.1 patch release, however users are "encouraged to migrate":upgrade-crunch2.html as support for the "jobs" API will be dropped in an upcoming release.  Users who are already using the "containers" API are not affected.
 
-h3. v1.2.1 (2018-11-26)
+h3(#v1_2_1). v1.2.1 (2018-11-26)
 
 There are no special upgrade notes for this release.
 
-h3. v1.2.0 (2018-09-05)
+h3(#v1_2_0). v1.2.0 (2018-09-05)
 
 h4. Regenerate Postgres table statistics
 
@@ -177,7 +200,7 @@ To add the Arvados Controller to your system please refer to the "installation i
 
 Verify your setup by confirming that API calls appear in the controller's logs (_e.g._, @journalctl -fu arvados-controller@) while loading a workbench page.
 
-h3. v1.1.4 (2018-04-10)
+h3(#v1_1_4). v1.1.4 (2018-04-10)
 
 h4. arvados-cwl-runner regressions (2018-04-05)
 
@@ -304,11 +327,11 @@ baseCommand: echo
 
 This bug has been fixed in Arvados release v1.2.0.
 
-h3. v1.1.3 (2018-02-08)
+h3(#v1_1_3). v1.1.3 (2018-02-08)
 
 There are no special upgrade notes for this release.
 
-h3. v1.1.2 (2017-12-22)
+h3(#v1_1_2). v1.1.2 (2017-12-22)
 
 h4. The minimum version for Postgres is now 9.4 (2017-12-08)
 
@@ -322,11 +345,11 @@ As part of story "#11908":https://dev.arvados.org/issues/11908, commit "8f987a92
 *# Install the @rh-postgresql94@ backport package from either Software Collections: http://doc.arvados.org/install/install-postgresql.html or the Postgres developers: https://www.postgresql.org/download/linux/redhat/
 *# Restore from the backup using @psql@
 
-h3. v1.1.1 (2017-11-30)
+h3(#v1_1_1). v1.1.1 (2017-11-30)
 
 There are no special upgrade notes for this release.
 
-h3. v1.1.0 (2017-10-24)
+h3(#v1_1_0). v1.1.0 (2017-10-24)
 
 h4. The minimum version for Postgres is now 9.3 (2017-09-25)
 
@@ -340,7 +363,7 @@ As part of story "#12032":https://dev.arvados.org/issues/12032, commit "68bdf4cb
 *# Install the @rh-postgresql94@ backport package from either Software Collections: http://doc.arvados.org/install/install-postgresql.html or the Postgres developers: https://www.postgresql.org/download/linux/redhat/
 *# Restore from the backup using @psql@
 
-h3. Older versions
+h3(#older). Older versions
 
 h4. Upgrade slower than usual (2017-06-30)
 
index 339bc9e23fdaf2334b060fda789e97130772be6a..efef7b812022868bc855dc12393ffa6ea341cd12 100644 (file)
@@ -27,7 +27,6 @@ class ArvadosModel < ApplicationRecord
   after_create :log_create
   after_update :log_update
   after_destroy :log_destroy
-  after_find :convert_serialized_symbols_to_strings
   before_validation :normalize_collection_uuids
   before_validation :set_default_owner
   validate :ensure_valid_uuids
@@ -602,41 +601,6 @@ class ArvadosModel < ApplicationRecord
     false
   end
 
-  def self.has_symbols? x
-    if x.is_a? Hash
-      x.each do |k,v|
-        return true if has_symbols?(k) or has_symbols?(v)
-      end
-    elsif x.is_a? Array
-      x.each do |k|
-        return true if has_symbols?(k)
-      end
-    elsif x.is_a? Symbol
-      return true
-    elsif x.is_a? String
-      return true if x.start_with?(':') && !x.start_with?('::')
-    end
-    false
-  end
-
-  def self.recursive_stringify x
-    if x.is_a? Hash
-      Hash[x.collect do |k,v|
-             [recursive_stringify(k), recursive_stringify(v)]
-           end]
-    elsif x.is_a? Array
-      x.collect do |k|
-        recursive_stringify k
-      end
-    elsif x.is_a? Symbol
-      x.to_s
-    elsif x.is_a? String and x.start_with?(':') and !x.start_with?('::')
-      x[1..-1]
-    else
-      x
-    end
-  end
-
   def self.where_serialized(colname, value, md5: false)
     colsql = colname.to_s
     if md5
@@ -677,23 +641,6 @@ class ArvadosModel < ApplicationRecord
     self.class.serialized_attributes
   end
 
-  def convert_serialized_symbols_to_strings
-    # ensure_serialized_attribute_type should prevent symbols from
-    # getting into the database in the first place. If someone managed
-    # to get them into the database (perhaps using an older version)
-    # we'll convert symbols to strings when loading from the
-    # database. (Otherwise, loading and saving an object with existing
-    # symbols in a serialized field will crash.)
-    jsonb_cols = self.class.columns.select{|c| c.type == :jsonb}.collect{|j| j.name}
-    (jsonb_cols + self.class.serialized_attributes.keys).uniq.each do |colname|
-      if self.class.has_symbols? attributes[colname]
-        attributes[colname] = self.class.recursive_stringify attributes[colname]
-        send(colname + '=',
-             self.class.recursive_stringify(attributes[colname]))
-      end
-    end
-  end
-
   def foreign_key_attributes
     attributes.keys.select { |a| a.match(/_uuid$/) }
   end
diff --git a/services/api/lib/tasks/symbols.rake b/services/api/lib/tasks/symbols.rake
new file mode 100644 (file)
index 0000000..a2e6df8
--- /dev/null
@@ -0,0 +1,109 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+require 'current_api_client'
+
+include CurrentApiClient
+
+def has_symbols? x
+  if x.is_a? Hash
+    x.each do |k,v|
+      return true if has_symbols?(k) or has_symbols?(v)
+    end
+  elsif x.is_a? Array
+    x.each do |k|
+      return true if has_symbols?(k)
+    end
+  elsif x.is_a? Symbol
+    return true
+  elsif x.is_a? String
+    return true if x.start_with?(':') && !x.start_with?('::')
+  end
+  false
+end
+
+def check_for_serialized_symbols rec
+  jsonb_cols = rec.class.columns.select{|c| c.type == :jsonb}.collect{|j| j.name}
+  (jsonb_cols + rec.class.serialized_attributes.keys).uniq.each do |colname|
+    if has_symbols? rec.attributes[colname]
+      st = recursive_stringify rec.attributes[colname]
+      puts "Found value potentially containing Ruby symbols in #{colname} attribute of #{rec.uuid}, current value is\n#{rec.attributes[colname].to_s[0..1024]}\nrake symbols:stringify will update it to:\n#{st.to_s[0..1024]}\n\n"
+    end
+  end
+end
+
+def recursive_stringify x
+  if x.is_a? Hash
+    Hash[x.collect do |k,v|
+           [recursive_stringify(k), recursive_stringify(v)]
+         end]
+  elsif x.is_a? Array
+    x.collect do |k|
+      recursive_stringify k
+    end
+  elsif x.is_a? Symbol
+    x.to_s
+  elsif x.is_a? String and x.start_with?(':') and !x.start_with?('::')
+    x[1..-1]
+  else
+    x
+  end
+end
+
+def stringify_serialized_symbols rec
+  # ensure_serialized_attribute_type should prevent symbols from
+  # getting into the database in the first place. If someone managed
+  # to get them into the database (perhaps using an older version)
+  # we'll convert symbols to strings when loading from the
+  # database. (Otherwise, loading and saving an object with existing
+  # symbols in a serialized field will crash.)
+  jsonb_cols = rec.class.columns.select{|c| c.type == :jsonb}.collect{|j| j.name}
+  (jsonb_cols + rec.class.serialized_attributes.keys).uniq.each do |colname|
+    if has_symbols? rec.attributes[colname]
+      begin
+        st = recursive_stringify rec.attributes[colname]
+        puts "Updating #{colname} attribute of #{rec.uuid} from\n#{rec.attributes[colname].to_s[0..1024]}\nto\n#{st.to_s[0..1024]}\n\n"
+        rec.write_attribute(colname, st)
+        rec.save!
+      rescue => e
+        puts "Failed to update #{rec.uuid}: #{e}"
+      end
+    end
+  end
+end
+
+namespace :symbols do
+  desc 'Warn about serialized values starting with ":" that may be symbols'
+  task check: :environment do
+    [ApiClientAuthorization, ApiClient,
+     AuthorizedKey, Collection,
+     Container, ContainerRequest, Group,
+     Human, Job, JobTask, KeepDisk, KeepService, Link,
+     Node, PipelineInstance, PipelineTemplate,
+     Repository, Specimen, Trait, User, VirtualMachine,
+     Workflow].each do |klass|
+      act_as_system_user do
+        klass.all.each do |c|
+          check_for_serialized_symbols c
+        end
+      end
+    end
+  end
+
+  task stringify: :environment do
+    [ApiClientAuthorization, ApiClient,
+     AuthorizedKey, Collection,
+     Container, ContainerRequest, Group,
+     Human, Job, JobTask, KeepDisk, KeepService, Link,
+     Node, PipelineInstance, PipelineTemplate,
+     Repository, Specimen, Trait, User, VirtualMachine,
+     Workflow].each do |klass|
+      act_as_system_user do
+        klass.all.each do |c|
+          stringify_serialized_symbols c
+        end
+      end
+    end
+  end
+end
index e66baceb28d0a28b3efb5361ca2a3a06b9401d75..2f66433379ed82db8cc95fa1e395d4c020420cec 100644 (file)
@@ -507,6 +507,15 @@ multilevel_collection_1_readable_by_active:
   head_uuid: zzzzz-4zz18-pyw8yp9g3pr7irn
   properties: {}
 
+#
+# This fixture was used in the test "Stringify symbols coming from
+# serialized attribute in database" which tested the hook
+# "convert_serialized_symbols_to_strings".  That hook (and the
+# corresponding test) was removed in #15311.  This fixture remains to
+# facilitate manual testing of the "rake symbols:check" and "rake
+# symbols:stringify" tasks that we added to assist with database
+# fixup.
+#
 has_symbol_keys_in_database_somehow:
   uuid: zzzzz-o0j2j-enl1wg58310loc6
   owner_uuid: zzzzz-tpzed-000000000000000
diff --git a/services/api/test/integration/container_request_test.rb b/services/api/test/integration/container_request_test.rb
new file mode 100644 (file)
index 0000000..26cc081
--- /dev/null
@@ -0,0 +1,39 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+require 'test_helper'
+
+class ContainerRequestIntegrationTest < ActionDispatch::IntegrationTest
+
+  test "test colon in input" do
+    # Tests for bug #15311 where strings with leading colons get
+    # corrupted when the leading ":" is stripped.
+    val = {"itemSeparator" => ":"}
+    post "/arvados/v1/container_requests",
+      params: {
+        :container_request => {
+          :name => "workflow",
+          :state => "Committed",
+          :command => ["echo"],
+          :container_image => "fa3c1a9cb6783f85f2ecda037e07b8c3+167",
+          :output_path => "/",
+          :priority => 1,
+          :runtime_constraints => {"vcpus" => 1, "ram" => 1},
+          :mounts => {
+            :foo => {
+              :kind => "json",
+              :content => JSON.parse(SafeJSON.dump(val)),
+            }
+          }
+        }
+      }.to_json,
+      headers: {
+        'HTTP_AUTHORIZATION' => "OAuth2 #{api_client_authorizations(:active).api_token}",
+        'CONTENT_TYPE' => 'application/json'
+      }
+    assert_response :success
+    assert_equal "arvados#containerRequest", json_response['kind']
+    assert_equal val, json_response['mounts']['foo']['content']
+  end
+end
index 0fcdad704f4e1133d6f6148e2dd5bccce87c34c3..7d9da1e561d24b0d18d383a869e5ca1b9de8ecb6 100644 (file)
@@ -82,17 +82,6 @@ class ArvadosModelTest < ActiveSupport::TestCase
     end
   end
 
-  test "Stringify symbols coming from serialized attribute in database" do
-    set_user_from_auth :admin_trustedclient
-    fixed = Link.find_by_uuid(links(:has_symbol_keys_in_database_somehow).uuid)
-    assert_equal(["baz", "foo"], fixed.properties.keys.sort,
-                 "Hash symbol keys from DB did not get stringified.")
-    assert_equal(['waz', 'waz', 'waz', 1, nil, false, true],
-                 fixed.properties['baz'],
-                 "Array symbol values from DB did not get stringified.")
-    assert_equal true, fixed.save, "Failed to save fixed model back to db."
-  end
-
   test "No HashWithIndifferentAccess in database" do
     set_user_from_auth :admin_trustedclient
     link = Link.create!(link_class: 'test',