Fixed issue #1663 - Sequencer ExternalSync lookup and storage is affected by ID collisions.

app/models/external_sync.rb

@@ -5,6 +5,10 @@ class ExternalSync < ApplicationModel
 
   class << self
 
+    def sanitized_source_id(source_id)
+      Digest::SHA2.hexdigest(source_id)
+    end
+
     def changed?(object:, previous_changes: {}, current_changes:)
       changed = false
       previous_changes ||= {}

lib/sequencer/unit/import/common/model/attributes/remote_id.rb

@@ -12,7 +12,7 @@ class Sequencer
 
               def process
                 state.provide(:remote_id) do
-                  resource.fetch(attribute).dup.to_s.downcase
+                  resource.fetch(attribute).dup.to_s
                 end
               rescue KeyError => e
                 handle_failure(e)

lib/sequencer/unit/import/common/model/external_sync/integrity.rb

@@ -19,8 +19,8 @@ class Sequencer
 
               def up_to_date?
                 return false if entry.blank?
-                return true if entry.source_id == remote_id
-                entry.update!(source_id: remote_id)
+                return true if entry.source_id == sanitized_remote_id
+                entry.update!(source_id: sanitized_remote_id)
                 true
               end
 
@@ -37,11 +37,15 @@ class Sequencer
               def create
                 ::ExternalSync.create(
                   source:    external_sync_source,
-                  source_id: remote_id,
+                  source_id: sanitized_remote_id,
                   object:    model_class.name,
                   o_id:      instance.id
                 )
               end
+
+              def sanitized_remote_id
+                @sanitized_remote_id ||= ::ExternalSync.sanitized_source_id(remote_id)
+              end
             end
           end
         end

lib/sequencer/unit/import/common/model/lookup/external_sync.rb

@@ -14,11 +14,6 @@ class Sequencer
               provides :instance
 
               def process
-                synced_instance = ::ExternalSync.find_by(
-                  source:    external_sync_source,
-                  source_id: remote_id,
-                  object:    model_class.name,
-                )
                 return if !synced_instance
 
                 state.provide(:instance) do
@@ -27,6 +22,55 @@ class Sequencer
               rescue => e
                 handle_failure(e)
               end
+
+              private
+
+              def synced_instance
+                @synced_instance ||= correct_entry || corrected_entry
+              end
+
+              def correct_entry
+                ::ExternalSync.find_by(
+                  source:    external_sync_source,
+                  source_id: sanitized_remote_id,
+                  object:    model_class.name,
+                )
+              end
+
+              def sanitized_remote_id
+                @sanitized_remote_id ||= ::ExternalSync.sanitized_source_id(remote_id)
+              end
+
+              def corrected_entry
+                return if obsolete_entry.blank?
+                obsolete_entry.update!(source_id: sanitized_remote_id)
+                obsolete_entry
+              end
+
+              def obsolete_entry
+                @obsolete_entry ||= begin
+                  if Rails.application.config.db_case_sensitive
+                    case_sensitive_entry
+                  else
+                    case_insensitive_entry
+                  end
+                end
+              end
+
+              def case_sensitive_entry
+                ::ExternalSync.where(
+                  source: external_sync_source,
+                  object: model_class.name,
+                ).where('LOWER(source_id) = LOWER(?)', remote_id).first
+              end
+
+              def case_insensitive_entry
+                ::ExternalSync.find_by(
+                  source:    external_sync_source,
+                  source_id: remote_id,
+                  object:    model_class.name,
+                )
+              end
             end
           end
         end

lib/sequencer/unit/import/ldap/users/lost/ids.rb

@@ -13,15 +13,15 @@ class Sequencer
               end
 
               def active_ids
-                ExternalSync.joins('INNER JOIN users ON (users.id = external_syncs.o_id)')
-                            .where(
-                              source: external_sync_source,
-                              object: model_class.name,
-                              users:  {
-                                active: true
-                              }
-                            )
-                            .pluck(:o_id)
+                ::ExternalSync.joins('INNER JOIN users ON (users.id = external_syncs.o_id)')
+                              .where(
+                                source: external_sync_source,
+                                object: model_class.name,
+                                users:  {
+                                  active: true
+                                }
+                              )
+                              .pluck(:o_id)
               end
             end
           end

spec/lib/external_sync_spec.rb

@@ -2,6 +2,27 @@ require 'rails_helper'
 
 RSpec.describe ExternalSync do
 
+  context '#sanitized_source_id' do
+
+    let(:source_id) { 'AbCdEfG124' }
+
+    it 'sanitizes source ids' do
+      sanitized_source_id = described_class.sanitized_source_id(source_id)
+      expect(sanitized_source_id).to_not eq(source_id)
+    end
+
+    it 'returns case insenstive value' do
+      sanitized_source_id = described_class.sanitized_source_id(source_id)
+      expect(sanitized_source_id).to eq(sanitized_source_id.downcase)
+    end
+
+    it 'avoids case sensitive collitions' do
+      sanitized_source_id           = described_class.sanitized_source_id(source_id)
+      sanitized_source_id_downcased = described_class.sanitized_source_id(source_id.downcase)
+      expect(sanitized_source_id).to_not eq(sanitized_source_id_downcased)
+    end
+  end
+
   context '#changed?' do
 
     it 'keeps ActiveRecord instance unchanged on local but no remote changes' do

spec/lib/sequencer/unit/import/common/model/attributes/remote_id_spec.rb

@@ -38,9 +38,10 @@ RSpec.describe Sequencer::Unit::Import::Common::Model::Attributes::RemoteId, seq
     provided = process(parameters)
 
     expect(provided).to include(remote_id: '1337')
+    expect(provided[:remote_id]).to be_a(String)
   end
 
-  it 'downcases the value to prevent case sensivity issues with the ORM' do
+  it 'does not change the value to prevent id collision issues' do
     parameters = {
       resource: {
         id: 'AbCdEfG',
@@ -49,7 +50,7 @@ RSpec.describe Sequencer::Unit::Import::Common::Model::Attributes::RemoteId, seq
 
     provided = process(parameters)
 
-    expect(provided[:remote_id]).to eq(parameters[:resource][:id].downcase)
+    expect(provided[:remote_id]).to eq(parameters[:resource][:id])
   end
 
   it 'duplicates the value to prevent attribute changes' do

spec/lib/sequencer/unit/import/common/model/lookup/external_sync_spec.rb

@@ -2,14 +2,14 @@ require 'rails_helper'
 
 RSpec.describe Sequencer::Unit::Import::Common::Model::Lookup::ExternalSync, sequencer: :unit do
 
-  it 'finds model_class instances by remote_id' do
+  it 'finds model_class instance by remote_id' do
     user                 = create(:user)
     external_sync_source = 'test'
     remote_id            = '1337'
 
     ExternalSync.create(
       source:    external_sync_source,
-      source_id: remote_id,
+      source_id: ExternalSync.sanitized_source_id(remote_id),
       o_id:      user.id,
       object:    user.class,
     )
@@ -22,4 +22,68 @@ RSpec.describe Sequencer::Unit::Import::Common::Model::Lookup::ExternalSync, seq
 
     expect(provided[:instance]).to eq(user)
   end
+
+  context 'obsolete plain remote_id' do
+
+    let(:user) { create(:user) }
+    let(:external_sync_source) { 'test' }
+    let(:remote_id) { 'AbCdEfG' }
+
+    it 'finds model_class instance' do
+      ExternalSync.create(
+        source:    external_sync_source,
+        source_id: remote_id,
+        o_id:      user.id,
+        object:    user.class,
+      )
+
+      provided = process(
+        remote_id:            remote_id,
+        model_class:          user.class,
+        external_sync_source: external_sync_source,
+      )
+
+      expect(provided[:instance]).to eq(user)
+    end
+
+    it 'corrects external sync entry' do
+      entry = ExternalSync.create(
+        source:    external_sync_source,
+        source_id: remote_id,
+        o_id:      user.id,
+        object:    user.class,
+      )
+
+      process(
+        remote_id:            remote_id,
+        model_class:          user.class,
+        external_sync_source: external_sync_source,
+      )
+
+      entry.reload
+
+      expect(entry.source_id).to eq(ExternalSync.sanitized_source_id(remote_id))
+    end
+
+    it 'operates case agnostic' do
+      entry = ExternalSync.create(
+        source:    external_sync_source,
+        source_id: remote_id.downcase,
+        o_id:      user.id,
+        object:    user.class,
+      )
+
+      provided = process(
+        remote_id:            remote_id,
+        model_class:          user.class,
+        external_sync_source: external_sync_source,
+      )
+
+      expect(provided[:instance]).to eq(user)
+
+      entry.reload
+
+      expect(entry.source_id).to eq(ExternalSync.sanitized_source_id(remote_id))
+    end
+  end
 end