t.integer :item_id does not play well with, nor complains about id: :uuid

[evadne]

Silent data loss occurs for versions generated against models that use id: :uuid if the backing item_id column is not also an :uuid. Versions are created and retrieved by a meaningless portion of the model’s primary key.

This following configuration:

• create_table :users, id: :uuid { |t| …
• create_table :versions, { |t| … t.integer :item_id, :null => false

Exposes these following issues:

• Versions were actually generated
• Versions were generated with an incorrect item_id (obviously)
• This was not caught by relevant run-time safety checks in has_paper_trail.
• If you manually add another version with the correct UUID, then User#versions returns both versions.

This is caused because:

• It was possible to grab the UUID-backed object ID and stuff it into an integer column
• It was possible to save such an object
• The misconfiguration was not caught at run-time
• #versions was able to retrieve both version models

I propose hardening the project by:

• Auditing has_paper_trail and relevant initializers, so preliminary checks are carried out against the schema of the source table and the target versions table. If there is any inconsistency between the primary key column type on the model and the item_id column type on the versions table, throw an exception and do not proceed.
• Auditing the #versions finder, so we know why a version model with an incorrect item_id was returned, and are able to then fix other relevant issues if needed.

I am aware that this is caused by misconfiguration and is not a bug. I am also able to completely mitigate the problem by ensuring type consistency between the model’s identifier column and the item_id column in the versions table. However, I think it is highly beneficial for Paper Trail to identify and expose this kind of misconfiguration intelligently.

I am not aware of such a fix on master and volunteer to fix it, as long as this addition is considered relevant and appropriate. Please close this issue if it is out of scope or inappropriate.

Thanks so much.

Environment:

• OS X 10.9.3 13D45a
• Postgres 9.3.4 thru Homebrew
• Rails 4.1.0
• pg 0.17.1
• paper_trail 3.0.1

[batter]

If you want to make a pull request, then by all means, this seems like a logical idea. I will say, that to me (perhaps I'm biased as I am a maintainer of the gem who knows the internals pretty well), it would seem somewhat obvious that you can't have the versions association work out of the box if you are using a uuid for the primary key of the table corresponding to the model you are trying to attach it to unless you also make the uuid column a String on the migration for the versions table, as a basic understanding of join statements between tables makes it pretty obvious that you can't have a type inconsistency between keys. But an error message sounds like a good idea to me if it will help prevent people from running into this in the future, and save them time. 😄

[evadne]

Agreed. We should throw an exception and not comply.

On Apr 29, 2014, at 2:22, Ben Atkins notifications@github.com wrote:

If you want to make a pull request, then by all means, this seems like a
logical idea. I will say, that to me (perhaps I'm biased as I am a
maintainer of the gem who knows the internals pretty well), it would seem
somewhat obvious that you can't have the versions association work out of
the box if you are using a uuid for the primary key of the table
corresponding to the model you are trying to attach it to unless you also
make the uuid column a String on the migration for the versions table, as a
basic understanding of join statements between tables makes it pretty
obvious that you can't have a type inconsistency between keys. But an error
message sounds like a good idea to me if it will help prevent people from
running into this in the future, and save them time. [image: 😄]

Reply to this email directly or view it on
GitHubhttps://github.com//issues/363#issuecomment-41594405
.

[batter]

@evadne - Have you taken any cracks at adding an informative error message?

[evadne]

Not yet, I got sidetracked.

On May 24, 2014, at 1:20 AM, Ben Atkins notifications@github.com wrote:

@evadne - Have you taken any cracks at adding an informative error message?

—
Reply to this email directly or view it on GitHub.

[batter]

@evadne - Any progress on this? Trying to get this into 3.0.3 before releasing it. Perhaps you can just point me in the right direction or share any progress you did make and I can try to handle the rest.

[jaredbeck]

A function to inspect the data type of the primary key could be as simple as:

def paper_trail_assert_supported
  if columns_hash["id"].try(:type) != :integer
    fail ::PaperTrail::UnsupportedModel,
      "PaperTrail only supports models with integer primary keys."
  end
end

I see two possible times such an assertion could be made:

1. At class-definition-time, in has_paper_trail
   • I'd prefer this, but I can't figure out how to test it in the test/dummy app
2. At query-time (e.g. in record_create, record_update, and ``record_destroy`)

[rdlugosz]

+1 on addressing this—at least with an exception if not an overall change that supports UUIDs. Current state is that everything appears to work fine, but once you have enough records you'll start seeing odd versions associated with seemingly random records.

The reason for the problem is pretty obvious once you investigate and realize item_id is stored as an integer, but if you're just a user who set up paper_trail a long time ago it's surprising that it doesn't work. (That was my experience, at least.)

FWIW - the current approach keys off of a composite index of item_type and item_id. Would need to test this, but since UUID keys should be universally unique it may actually be faster to find versions keyed off of a single item_uuid field. I'd not expect it to be any slower than the current approach.

There are a variety of advantages to using UUID keys; I hope support will be considered for a future version of paper_trail.

[jaredbeck]

+1 on addressing this—at least with an exception if not an overall change that supports UUIDs

Let's start with the former, ie. an exception. See my notes on a potential implementation. A PR would be welcome, thanks!

[preston]

I recently got bit by this and had to change the column item_id column type from an integer to a string. I would have preferred sticking with the native UUID type, though was unless if that would be dangerous.

[h8rry]

@preston you can also change the migration file to uuid type

[preston]

@h8rry Tried it, but as the OP describes in detail, it's problematic.

[h8rry]

@batter. Can you confirm that if the following would work reliably? It works for me in Rails 5.

If you are using Postgres, you can change the item_id to :uuid type in the migration file, so that item_id will match with the corresponding record's id in :uuid.

def change
  create_table :versions, versions_table_options do |t|
    t.string   :item_type, item_type_options
    t.uuid     :item_id,   null: false
    t.string   :event,     null: false
    ...
  end
  add_index :versions, [:item_type, :item_id]
end

...

Also, if you want the each version record to have an id with :uuid type, you can also do the following.

def versions_table_options
  if mysql?
    { options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci", id: :uuid }
  else
    { id: :uuid }
  end
end

[batter]

Thanks for the suggestion @h8rry, neat to hear you have this setup working.

Can you confirm that if the following would work reliably?

I can't, but we can certainly try to work it into our test suite as a special custom version class and see if it works for ActiveRecord 3 & 4 as well.

Also, if you want the each version record to have an id with :uuid type, you can also do the following.

It looks semi-reasonable to me, however, are there other SQL databases beyond Postgres that support a uuid type? I know SQLite definitely doesn't, so I don't think this else statement can be used as broadly as you're using it here.

[zharikovpro]

Just tested @h8rry solution with Postgres. Confirm that it works well for me.

[jaredbeck]

.. we can certainly try to work it into our test suite as a special custom version class ..

As Ben says, the next step on this issue is to add tests. Start by creating a new model in spec/dummy_app/app/models. Migrations go in spec/dummy_app/db/migrate/20110208155312_set_up_test_tables.rb. Thanks!

[danielolivaresd]

Just to be clear, is @h8rry's solution the "official" (suggested) way of using PT with uuid for now?

I personally think that it works, but I don't know what would happen if not all of the tables of the models that #has_paper_trail use uuid (haven't tested, since all of my models use uuid).

[jaredbeck]

.. is @h8rry's solution the "official" (suggested) way of using PT with uuid for now?

We can't "officially" support something until we have tests for it.

[jaredbeck]

Closing due to inactivity. Contributions welcome, as described above.

[Geesu]

We just ran into this issue as we have a mix of id and uuid (as we slowly transition to uuid columns). Unfortunately we didn't know this was happening until today, so for some of our models the data just needs to be destroyed given we can't associate an int to a uuid.

Question - instead of making the item_id column a uuid, can we just make it varchar?

[nemesit]

wtf why is this closed its very much an issue

[belt]

Making the column a character varying works for int, bigint, and uuid primary keys.

Be aware, when altering tables, DBs typically lock the table and then cast the data.
This will not be a good experience on live (production) DB instances for humans (lock-contention and timeouts).