Skip to content

Fix SourceSynchronizer silently dropping edits to translated attributes - #116

Merged
npflood merged 1 commit into
masterfrom
phase0/verify-source-synchronizer
Aug 13, 2026
Merged

Fix SourceSynchronizer silently dropping edits to translated attributes#116
npflood merged 1 commit into
masterfrom
phase0/verify-source-synchronizer

Conversation

@npflood

@npflood npflood commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

What this fixes

SourceSynchronizer#compare_and_sync read local state with:

local = 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 and the other 20-odd — live
inside the translations jsonb and never appear in #attributes. Slicing there
returns {} for them, so local always looked different from the stored
snapshot.

Why it matters

Two symptoms, both reproduced in the added spec before the fix:

  1. An unchanged re-sync was scored locally_modified rather than synced.
  2. A genuine upstream edit was silently droppedapplied stayed 0, no
    SyncConflict was raised, and the value never changed.

The second is the serious one. The failure that had been predicted from reading
the code was "spurious conflicts on every second run", which would at least be
visible. What actually happens is that translated fields stop syncing after
record creation, with nothing raised anywhere.

Why the suite didn't catch it

SOURCE_ATTRS in spec/services/source_synchronizer_spec.rb is
%w[scientific_name family_names] — both real columns — and the idempotency
spec uses that default. The one spec that passes %w[description] exercises the
invalid-payload path, which returns before compare_and_sync is reached.

The change

Read through the public reader instead of the attributes hash, so column-backed
and translated attributes behave identically. Applied at both call sites
(compare_and_sync and handle_source_deletion).

A note on Mobility's fallbacks plugin is included in the code comment: reading
a missing locale returns the :en value, but sync writes and reads within the
same locale, so the comparison stays symmetric.

Verification

  • New spec spec/services/source_synchronizer_translated_spec.rb covers the
    mechanism (translated attrs absent from #attributes) and both symptoms.
  • Full suite: 2,307 examples, 0 failures.
  • Rubocop clean on both changed files.

Context

Found while verifying risk R5 of the plant-data ownership migration, which plans
to use SourceSynchronizer to reconcile ECHOcommunity plant data into this API.
24 of the plant fields involved are translated, so this would have blocked that
work. The suspicion originated in fpi-connector/docs/target-api-analysis.md,
where it was explicitly labelled a code-reading conclusion needing verification.

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.
@npflood
npflood merged commit 3909208 into master Aug 13, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant