Convert status values to expected types

ee/app/models/geo_node_status.rb

@@ -8,6 +8,7 @@ class GeoNodeStatus < ApplicationRecord
   delegate :selective_sync_type, to: :geo_node
 
   after_initialize :initialize_feature_flags
+  before_save :coerce_status_field_values, if: :status_changed?
 
   attr_accessor :storage_shards
 
@@ -166,25 +167,23 @@ class GeoNodeStatus < ApplicationRecord
   def self.alternative_status_store_accessor(attr_names)
     attr_names.each do |attr_name|
       define_method(attr_name) do
-        status[attr_name].nil? ? read_attribute(attr_name) : status[attr_name].to_i
+        val = status[attr_name]
+
+        # Backwards-compatible line for when the status was written by an
+        # earlier release without the `status` field
+        val ||= read_attribute(attr_name)
+
+        convert_status_value(attr_name, val)
       end
 
       define_method("#{attr_name}=") do |val|
-        status[attr_name] = val.nil? ? nil : val.to_i
+        val = convert_status_value(attr_name, val)
 
-        return unless self.class.column_names.include?(attr_name)
-
-        write_attribute(attr_name, val)
+        status[attr_name] = val
       end
     end
   end
 
-  # We migrated from attributes stored in individual columns to
-  # a single JSONB hash. To create accessors method for every fields in hash we could use
-  # Rails embeded store_accessor but in this case we won't have smooth update procedure.
-  # The method "alternative_status_store_accessor" does actually the same that store_accessor would
-  # but it uses the old fashioned fields in case the new ones are not set yet. We also casting the type into integer when
-  # we set the value to preserve the old behaviour.
   alternative_status_store_accessor RESOURCE_STATUS_FIELDS
 
   def self.current_node_status
@@ -236,6 +235,36 @@ class GeoNodeStatus < ApplicationRecord
     self.column_names - EXCLUDED_PARAMS + EXTRA_PARAMS
   end
 
+  # Helps make alternative_status_store_accessor act more like regular Rails
+  # attributes. Request params values are always strings, but when saved as
+  # attributes of a model, they are converted to the appropriate types. We could
+  # manually map a specified type to each attribute, but for now, the type can
+  # be easily inferred by the attribute name.
+  #
+  # If you add a new status attribute that does not look like existing
+  # attributes, then you'll get an error until you handle it in the cases below.
+  #
+  # @param [String] attr_name the status key
+  # @param [String, Integer, Boolean] val being assigned or retrieved
+  # @return [String, Integer, Boolean] converted value based on attr_name
+  def convert_status_value(attr_name, val)
+    return if val.nil?
+
+    case attr_name
+    when /_count\z/ then val.to_i
+    when /_enabled\z/ then val.to_s == 'true'
+    else raise "Unhandled status attribute name format \"#{attr_name}\""
+    end
+  end
+
+  # Leverages attribute reader methods written by
+  # alternative_status_store_accessor to convert string values to integers and
+  # booleans if necessary.
+  def coerce_status_field_values
+    status_attrs = status.slice(*RESOURCE_STATUS_FIELDS)
+    self.assign_attributes(status_attrs)
+  end
+
   def initialize_feature_flags
     self.repository_verification_enabled = Gitlab::Geo.repository_verification_enabled?
 

ee/spec/models/geo_node_status_spec.rb

@@ -1327,18 +1327,17 @@ RSpec.describe GeoNodeStatus, :geo, :geo_fdw do
 
     context 'backward compatibility when counters stored in separate columns' do
       describe '#projects_count' do
-        it 'counts the number of projects' do
+        it 'returns data from the deprecated field if it is not defined in the status field' do
           subject.write_attribute(:projects_count, 10)
           subject.status = {}
 
           expect(subject.projects_count).to eq 10
         end
 
-        it 'sets data in both ways, deprecated and the new one' do
+        it 'sets data in the new status field' do
           subject.projects_count = 10
 
           expect(subject.projects_count).to eq 10
-          expect(subject.read_attribute(:projects_count)).to eq 10
         end
 
         it 'uses column counters when calculates percents using attr_in_percentage' do
@@ -1358,5 +1357,13 @@ RSpec.describe GeoNodeStatus, :geo, :geo_fdw do
         expect(subject.projects_count).to eq 10
       end
     end
+
+    context 'status booleans are converted into booleans' do
+      it 'returns boolean value' do
+        subject.status = { "repositories_replication_enabled" => "true" }
+
+        expect(subject.repositories_replication_enabled).to eq true
+      end
+    end
   end
 end