Merge pull request #523 from performant-software/bugfix/dupe-uids

Deduplicate highlights with identical UIDs and prevent future UID collisions

Gemfile

@@ -53,4 +53,6 @@ gem 'base64', '~> 0.2.0'
 gem 'mutex_m', '~> 0.2.0'
 gem 'bigdecimal', '~> 3.1'
 gem 'psych', '< 4'
-gem "dotenv-rails", "~> 2.8"
+gem 'dotenv-rails', '~> 2.8'
+# pinned to < 9.3 until https://github.com/ilyakatz/data-migrate/issues/302 resolved
+gem 'data_migrate', '~> 9.2', '< 9.3'

Gemfile.lock

@@ -102,6 +102,9 @@ GEM
     concurrent-ruby (1.3.4)
     connection_pool (2.4.1)
     crass (1.0.6)
+    data_migrate (9.2.0)
+      activerecord (>= 6.1)
+      railties (>= 6.1)
     date (3.3.4)
     devise (4.7.3)
       bcrypt (~> 3.0)
@@ -297,6 +300,7 @@ DEPENDENCIES
   bigdecimal (~> 3.1)
   bootsnap (~> 1.7.7)
   byebug
+  data_migrate (~> 9.2, < 9.3)
   devise (~> 4.7.1)
   devise_token_auth (~> 1.1.5)
   dotenv-rails (~> 2.8)

Procfile

@@ -1,3 +1,3 @@
-release: rails db:migrate
+release: rails db:migrate:with_data
 web: bundle exec puma -C config/puma.rb
 worker: bundle exec sidekiq -e production -C config/sidekiq.yml

app.json

@@ -2,7 +2,7 @@
   "name": "dm-2",
   "description": "Digital Mappa 2",
   "scripts": {
-    "postdeploy": "bundle exec rake db:migrate"
+    "postdeploy": "bundle exec rake db:migrate:with_data"
   },
   "repository": "https://github.com/performant-software/dm-2",
   "env": {

bin/update

@@ -18,7 +18,7 @@ chdir APP_ROOT do
   system('bundle check') || system!('bundle install')
 
   puts "\n== Updating database =="
-  system! 'bin/rails db:migrate'
+  system! 'bin/rails db:migrate:with_data'
 
   puts "\n== Removing old logs and tempfiles =="
   system! 'bin/rails log:clear tmp:clear'

client/package.json

@@ -41,6 +41,7 @@
     "redux-devtools-extension": "^2.13.2",
     "redux-thunk": "^2.2.0",
     "redux-token-auth": "^0.19.0",
+    "uuid": "^10.0.0",
     "xmldom": "^0.6.0"
   },
   "resolutions": {

client/src/CanvasResource.js

@@ -45,6 +45,7 @@ import HighlightColorSelect from './HighlightColorSelect';
 import AddImageLayer from './AddImageLayer';
 import TextField from 'material-ui/TextField';
 import deepEqual from 'deep-equal';
+import { v4 as uuidv4 } from 'uuid';
 
 // overlay these modules
 openSeaDragonFabricOverlay(OpenSeadragon, fabric);
@@ -375,7 +376,7 @@ class CanvasResource extends Component {
     overlay.fabricCanvas().on('path:created', event => {
       if (event.path) {
         let path = event.path;
-        const highlightUid = `dm_canvas_highlight_${Date.now()}`;
+        const highlightUid = `dm_canvas_highlight_${uuidv4()}`;
         path._highlightUid = highlightUid;
         path.perPixelTargetFind = true;
         this.overlay.fabricCanvas().setActiveObject(path);
@@ -822,7 +823,7 @@ class CanvasResource extends Component {
     this.lineInProgress.perPixelTargetFind = true;
     this.lineInProgress.selectable = true;
     this.lockCanvasObject(this.lineInProgress, true);
-    const highlightUid = `dm_canvas_highlight_${Date.now()}`;
+    const highlightUid = `dm_canvas_highlight_${uuidv4()}`;
     this.lineInProgress['_highlightUid'] = highlightUid;
 
     this.props.setSaving({ doneSaving: false });
@@ -849,7 +850,7 @@ class CanvasResource extends Component {
   }
 
   addShape(fabricObject) {
-    const highlightUid = `dm_canvas_highlight_${Date.now()}`;
+    const highlightUid = `dm_canvas_highlight_${uuidv4()}`;
     const { highlightColors } = this.props;
     const instanceKey = this.getInstanceKey();
     fabricObject['_highlightUid'] = highlightUid;

client/src/TextResource.js

@@ -1,6 +1,7 @@
 import React, { Component } from 'react';
 import { bindActionCreators } from 'redux';
 import { connect } from 'react-redux';
+import { v4 as uuidv4 } from 'uuid';
 
 import { yellow500 } from 'material-ui/styles/colors';
 import DropDownMenu from 'material-ui/DropDownMenu';
@@ -814,7 +815,7 @@ class TextResource extends Component {
     const markType = this.state.documentSchema.marks.highlight;
     const { document_id } = this.props;
     const editorState = this.getEditorState();
-    const cmd = addMark( markType, {highlightUid: `dm_text_highlight_${Date.now()}`, documentId: document_id });
+    const cmd = addMark( markType, {highlightUid: `dm_text_highlight_${uuidv4()}`, documentId: document_id });
     cmd( editorState, this.state.editorView.dispatch );
     this.state.editorView.focus();
   }
@@ -1259,7 +1260,7 @@ class TextResource extends Component {
     });
     pastedMarks.forEach((mark, index) => {
       let markEntry = Object.assign({}, mark.attrs);
-      mark.attrs['highlightUid'] = markEntry['newHighlightUid'] = `dm_text_highlight_${Date.now()}_${index}`;
+      mark.attrs['highlightUid'] = markEntry['newHighlightUid'] = `dm_text_highlight_${uuidv4()}_${index}`;
       mark.attrs['documentId'] = this.props.document_id;
       this.highlightsToDuplicate.push(markEntry);
     });

client/yarn.lock

@@ -9569,6 +9569,11 @@ [email protected]:
   version "1.0.1"
   resolved "https://registry.yarnpkg.com/utils-merge/-/utils-merge-1.0.1.tgz#9f95710f50a267947b2ccc124741c1028427e713"
 
+uuid@^10.0.0:
+  version "10.0.0"
+  resolved "https://registry.yarnpkg.com/uuid/-/uuid-10.0.0.tgz#5a95aa454e6e002725c79055fd42aaba30ca6294"
+  integrity sha512-8XkAphELsDnEGrDxUOHB3RGvXz6TeuYSGEZBOjtTtPm2lwhGBjLgOzLHB63IUWfBpNucQjND6d3AOudO+H3RWQ==
+
 uuid@^3.0.1, uuid@^3.3.2:
   version "3.3.3"
   resolved "https://registry.yarnpkg.com/uuid/-/uuid-3.3.3.tgz#4568f0216e78760ee1dbf3a4d2cf53e224112866"

db/data/20241002192349_deduplicate_highlights.rb

@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+class DeduplicateHighlights < ActiveRecord::Migration[6.1]
+  # one-time data migration to deduplicate highlights with identical UIDs
+  def up
+    # NOTE: Document.highlight_map would always return one record per UID, even when there were
+    # multiple records with identical UIDs. As such, since Document.highlight_map was relying on
+    # default database ordering with no ORDER BY, it is impossible to replicate the correct order
+    # with ORDER BY; we can only use the ordering from the same SQL statement executed previosuly
+    # by highlight_map. Once we retrieve the correct highlight per each UID we can delete all
+    # others sharing its UID.
+    Document.pluck(:id).each do |doc_id|
+      # get the correct highlight database ID per each UID; MUST be in order returned by this query
+      query = <<-SQL
+        SELECT "highlights".* FROM "highlights" WHERE "highlights"."document_id" = #{doc_id};
+      SQL
+      doc_highlights = Highlight.find_by_sql(query)
+      correct_highlights = doc_highlights.to_h { |hl| [hl[:uid], hl[:id]] }
+      # find all highlights with matching UIDs but non-matching database IDs (i.e. unused duplicates)
+      to_delete_ids= doc_highlights.select {
+        |hl| correct_highlights[hl[:uid]] != hl[:id]
+      }.pluck(:id)
+      # destroy unused duplicate records
+      Highlight.where(:id => to_delete_ids).destroy_all
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

db/data_schema.rb

@@ -0,0 +1 @@
+DataMigrate::Data.define(version: 20241002192349)

db/schema.rb

@@ -44,6 +44,9 @@
     t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true
   end
 
+  create_table "data_migrations", primary_key: "version", id: :string, force: :cascade do |t|
+  end
+
   create_table "document_folders", force: :cascade do |t|
     t.string "title"
     t.string "parent_type"