Skip to content

Use each_batch instead of in_batches

Arturo Herrero requested to merge service-each-batch into master

What does this MR do?

Service#properties is a text column using serialize :properties, JSON. We also have an after_initialize callback introduced in 09cdd943.

If we use a query that doesn't return all the columns, we can get an error, as we are using Service.each_batch.

Failure/Error: self.properties = {} if properties.nil?

ActiveModel::MissingAttributeError:
  missing attribute: properties
# ./app/models/service.rb:281:in `initialize_properties'
# ./app/models/concerns/each_batch.rb:61:in `each_batch'

We have to use has_attribute? to avoid the ActiveRecord::MissingAttributeError exception.

Alternative

There is one alternative to this implementation but I'm not sure what it's better from the database point of view. We can remove the after_initialize callback if we have the following database default:

diff --git a/app/models/service.rb b/app/models/service.rb
index 0998a9a102a..f21919b4234 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -38,8 +38,6 @@ class Service < ApplicationRecord
   default_value_for :tag_push_events, true
   default_value_for :wiki_page_events, true

-  after_initialize :initialize_properties
-
   after_commit :reset_updated_properties
   after_commit :cache_project_has_external_issue_tracker
   after_commit :cache_project_has_external_wiki
@@ -294,10 +292,6 @@ def category
     read_attribute(:category).to_sym
   end

-  def initialize_properties
-    self.properties = {} if has_attribute?(:properties) && properties.nil?
-  end
-
   def title
     # implement inside child
   end
diff --git a/db/migrate/20201022162121_default_properties_on_services.rb b/db/migrate/20201022162121_default_properties_on_services.rb
new file mode 100644
index 00000000000..1ad612ac319
--- /dev/null
+++ b/db/migrate/20201022162121_default_properties_on_services.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+class DefaultPropertiesOnServices < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    change_column_default(:services, :properties, from: nil, to: '{}')
+  end
+end
diff --git a/db/schema_migrations/20201022162121 b/db/schema_migrations/20201022162121
new file mode 100644
index 00000000000..2c1cbc90101
--- /dev/null
+++ b/db/schema_migrations/20201022162121
@@ -0,0 +1 @@
+e08a2727d9488c122d39412db870c5f02c24acad628d9e2b6dfe1356f4b3967b
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index fc3d1dbef59..6c50fe841ee 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -15978,7 +15978,7 @@ CREATE TABLE services (
     created_at timestamp without time zone,
     updated_at timestamp without time zone,
     active boolean DEFAULT false NOT NULL,
-    properties text,
+    properties text DEFAULT '{}'::text,
     push_events boolean DEFAULT true,
     issues_events boolean DEFAULT true,
     merge_requests_events boolean DEFAULT true,

Related:

Merge request reports