From 5b77888b6f6d3c227d3b19bf9d1769bfe53f8936 Mon Sep 17 00:00:00 2001 From: nflood Date: Thu, 13 Aug 2026 00:19:08 +0000 Subject: [PATCH] Fix SourceSynchronizer ignoring Mobility-translated attributes compare_and_sync read local state with record.attributes.slice(*source_attributes). Mobility is configured with backend :container and without the attribute_methods plugin, so translated attributes (description, uses, cultivation, ...) live in the translations jsonb and never appear in #attributes. Slicing there returned {} for them, so local always looked different from the stored snapshot. Consequences, both verified by the added spec before the fix: - an unchanged re-sync was scored locally_modified instead of synced - a genuine upstream edit to a translated attribute was silently dropped - applied stayed 0, no conflict was raised, and the value never changed Translated fields therefore stopped syncing after record creation, silently. The existing suite missed this because SOURCE_ATTRS is scientific_name and family_names, both real columns, and the one spec passing %w[description] exercises the invalid-payload path, which returns before compare_and_sync. Read through the public reader instead, so column-backed and translated attributes behave identically. Applied at both call sites. Full suite: 2307 examples, 0 failures. Rubocop clean. --- app/services/source_synchronizer.rb | 25 ++++- .../source_synchronizer_translated_spec.rb | 91 +++++++++++++++++++ 2 files changed, 114 insertions(+), 2 deletions(-) create mode 100644 spec/services/source_synchronizer_translated_spec.rb diff --git a/app/services/source_synchronizer.rb b/app/services/source_synchronizer.rb index 9a7a2ad..6d8a6dc 100644 --- a/app/services/source_synchronizer.rb +++ b/app/services/source_synchronizer.rb @@ -167,7 +167,7 @@ def find_record(src_id) # rubocop:disable Metrics/MethodLength def compare_and_sync(record, incoming_attrs, src_at, report) base = base_attrs(record) - local = record.attributes.slice(*@source_attributes) + local = local_attrs(record) local_digest = canonical_digest(local) incoming_digest = canonical_digest(incoming_attrs) @@ -245,6 +245,27 @@ def compare_and_sync(record, incoming_attrs, src_at, report) end # rubocop:enable Metrics/MethodLength + # Local state for the source-managed attributes. + # + # Deliberately NOT record.attributes.slice(*@source_attributes). Mobility is + # configured with `backend :container` and without the `attribute_methods` + # plugin (config/initializers/mobility.rb), so translated attributes + # (description, uses, cultivation, ...) live inside the `translations` jsonb + # and never appear in #attributes. Slicing there yields {} for them, so local + # reads as permanently different from the stored snapshot: an unchanged + # re-sync is scored `locally_modified` and a genuine upstream edit is silently + # dropped rather than applied. Reading through the public reader treats + # column-backed and translated attributes identically. + # + # Note on fallbacks: the Mobility `fallbacks` plugin means reading a missing + # locale returns the :en value. Sync writes and reads within the same locale, + # so the comparison stays symmetric. + def local_attrs(record) + @source_attributes.each_with_object({}) do |attr, acc| + acc[attr] = record.public_send(attr) if record.respond_to?(attr) + end + end + # base = last accepted source snapshot, sliced to source_attributes def base_attrs(record) snap = record.source_snapshot @@ -287,7 +308,7 @@ def handle_content_conflict(record, base, local, incoming_attrs, report) end def handle_source_deletion(record, report) - local = record.attributes.slice(*@source_attributes) + local = local_attrs(record) existing = SyncConflict.where( syncable: record, diff --git a/spec/services/source_synchronizer_translated_spec.rb b/spec/services/source_synchronizer_translated_spec.rb new file mode 100644 index 0000000..d9214a7 --- /dev/null +++ b/spec/services/source_synchronizer_translated_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Verification of the suspected defect recorded as risk R5 in the plant-data +# ownership migration roadmap. +# +# SourceSynchronizer#compare_and_sync reads local state with +# +# local = record.attributes.slice(*@source_attributes) +# +# `record.attributes` returns ActiveRecord's column-backed attributes. Mobility +# is configured with `backend :container` and WITHOUT the `attribute_methods` +# plugin (config/initializers/mobility.rb), so translated attributes such as +# `description` live inside the `translations` jsonb and never appear in +# `attributes`. For a translated source attribute the local side therefore reads +# as {} on every run. +# +# The existing suite never catches this: SOURCE_ATTRS is +# %w[scientific_name family_names], both real columns, and the one spec that +# passes %w[description] exercises the invalid-payload path, which returns +# before compare_and_sync. +# +# Predicted consequence: a re-sync of unchanged data is scored as +# `locally_modified` rather than `synced`, and a genuine upstream change is +# never applied. Translated fields would silently stop syncing after creation. +RSpec.describe SourceSynchronizer, 'with a Mobility-translated source attribute' do + let(:org) { create(:organization, :real) } + let(:data_source) { create(:data_source, organization: org) } + let(:run_id) { SecureRandom.hex(8) } + + def sync(attrs) + SourceSynchronizer.new( + data_source: data_source, + model: Plant, + source_attributes: attrs, + run_id: run_id + ) + end + + def row(description:, source_record_id: 'tr-1', source_updated_at: 1.day.ago) + { + source_record_id: source_record_id, + deleted: false, + attributes: { 'description' => description }, + source_updated_at: source_updated_at + } + end + + it 'confirms the mechanism: a translated attribute is absent from #attributes' do + plant = create(:plant, owner_organization_id: org.id, source_organization_id: org.id, + description: 'Some description') + + expect(plant.description).to eq 'Some description' + expect(plant.attributes).to have_key('translations') + expect(plant.attributes).not_to have_key('description'), + 'if this fails, Mobility now exposes translated attrs and the defect is gone' + expect(plant.attributes.slice('description')).to eq({}) + end + + it 'scores an identical re-run as synced, not locally_modified' do + batch = [row(description: 'A stable description')] + + first = sync(%w[description]).apply(batch) + expect(first.created).to eq(1), 'expected the first run to create the record' + + second = sync(%w[description]).apply(batch) + + plant = Plant.find_by(source_record_id: 'tr-1') + aggregate_failures do + expect(second.locally_modified).to eq(0), + 'nobody edited this record, so it must not be scored locally_modified' + expect(second.synced).to eq(1) + expect(plant.sync_state).to eq 'synced' + end + end + + it 'applies a genuine upstream change to a translated attribute' do + sync(%w[description]).apply([row(description: 'Original text')]) + + changed = sync(%w[description]).apply( + [row(description: 'Updated upstream text', source_updated_at: 1.hour.ago)] + ) + + plant = Plant.find_by(source_record_id: 'tr-1') + aggregate_failures do + expect(changed.applied).to eq(1), 'an upstream edit must reach the record' + expect(plant.description).to eq 'Updated upstream text' + end + end +end