Add missing unique indices

[bf4]

Referenced in #294

Note that because indexed fields are added in order, the attached commit includes the index on :tag_id and the compound index on taggable_id, taggable_type, context

I was running consistency_fail on my project and it pointed out that since the app enforces uniqueness on these fields, the database should as well. (It's more reliable race-condition-wise, too)

This commit is not ruby 1.8 compatible. Based on the README, that seems appropriate

[tilsammans]

Thanks, I will take a look. Do you have EXPLAIN output at hand that shows the improvement?

We will also need an upgrade migration for everyone who already has the gem.

[bf4]

Per #294 (comment), I'll name the index

[bf4]

Sorry for the delay, I kept forgetting what I needed to do, add an upgrade migration. Will do over the weekend.

[bf4]

soon..

[makaroni4]

Why not to add just [:tag_id, :taggable_id, :taggable_type] unique index? It will protect Tagging duplicates. With current index in PR([:tag_id, :taggable_id, :taggable_type, :context, :tagger_id, :tagger_type]) duplicates still could be created.

[bf4]

Ok, in terms of how engines and migrations work, right now we're copying over the migration.rb file. In order to allow an upgrade migration, I believe we'd need for me to make these changes in a second migration and update the engine to run install/copy/run multiple migrations in a sorted order.

We could just specify our migrations and inject them directly but that would change the current behavior.

Do you have any particular insights into that?

@makaroni4 in terms of uniqueness constraint, context needs to be included because that is the 'tag group'. i.e. you could tag the genre of music with alternative and also tag the radio station style of the music as alternative (bad example) where the tag would be alternative in both cases but the context would be different, and that's okay. As for tagger_id and tagger_type, perhaps you're correct that they shouldn't be included in the uniqueness constraint, but that is how the constraint is currently specified in the rails code

[makaroni4]

@bf4 for current constraint your index is good, you are right.

[bf4]

Looks like I'll have to change the engine code to run multiple migrations

[bf4]

Should be done now... could probably improve the documentation, but otherwise, I modified the commit to

1. No change to the existing migration
2. Add a second migration to correct the schema (drops indices... warning?)
3. rails g acts_as_taggable_on:migration Runs the migrations in sorted order, skipping them if they exist, else creating them. i.e. it can be run for install and upgrade.
4. Added an UPGRADE file for post install message

[bf4]

bump

[bf4]

bump

[tilsammans]

I've seen your bumps but the pull request doesn't merge cleanly and I'm swamped at the moment.
It will have to wait until time frees up for me, because this pull request needs investigating.

[bf4]

Ok, will update the PR to merge cleanly

[bf4]

@tilsammans Rebased the commit on top and force pushed

[parndt]

As this is a plugin for Rails (I see the dependency in the gemspec), why not just hook into the rake railties:install:migrations API that already exists for cleanly managing migration copying and main application migration order? Is there something I'm missing?

[bf4]

@parndt Code example? Will that handle migration ordering, even while preserving the name of the already-existing migration? I find the documentation for this stuff pretty spotty, and mostly involves code-diving, which is why, when I get around to it, I'm going to help out with the documentation at https://github.com/bf4/docrails/tree/rails-engines/guides (as suggested by steve klabnik)

[parndt]

For ordering you just have to name them sortably, for example 1_migration.rb, 2_migration.rb, 3_migration.rb or using timestamps: 20100913234705_create_refinerycms_authentication_schema.rb, 20120301234455_add_slug_to_refinery_users.rb as either format is accepted and Rails doesn't seem to mind which.

Then, ensure your files are in ENGINE_ROOT/db/migrate/ and provide a Railtie of some type e.g.

# lib/plugin_name.rb which gets autoloaded by gem naming conventions.
require 'plugin_name/engine'
module PluginName
end

# lib/plugin_name/engine.rb
require 'rails'
module PluginName
  class Engine < Rails::Engine
  end
end

Or am I misunderstanding the requirements?

[bf4]

I think so, @parndt there is already a migration prior to this PR named "migration.rb". So, a way to make that sortable didn't immediately occur to me, unless it were something like new-1_migration, new-2_migration, but that breaks the rails convention and we'd still have to implement next_migration_number anayways

[parndt]

Why not just rename that to 1_migration.rb?

[bf4]

@parndt Did you look at the diff?

[parndt]

I looked but I did not understand why this was required.

[bf4]

Because too much of rails engine knowledge requires source diving and I missed how easy it is to rename and move the files to db/migrate 😁

[parndt]

😉

[bf4]

I, wrongly, thought that would break existing applications that used the gem by adding a duplicate migration. Now that you write that, I see that 1_migration.rb will resolve to the existing migration.rb Yay

[parndt]

Yep, and Rails should install it as something like Rails.root/db/migrate/<timestamp>_migration.acts_as_taggable_on.rb if we follow the conventions. Which would also require us moving it to Engine.root/db/migrate/1_migration.rb and adding a Railtie/Engine definition.

[bf4]

Updated to use the simpler Rails::Engine integration. Added some other commits I'm happy to remove and put in a different PR, if you want.

[bf4]

If you're reviewing, I found a bug that it's appending '_engine' to the copied migration name. Looking at how to set the basename

[bf4]

@parndt How annoying, not exactly as we thought

current generated migration name pre-PR

• db/migrate/20130307233647_acts_as_taggable_on_migration.rb

new migration name(s) per simple rename of files as suggested above

• db/migrate/20130805144843_migration.acts_as_taggable_on_engine.rb
• db/migrate/20130805144844_add_missing_unique_indices.acts_as_taggable_on_engine.rb

Relevant code

• https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/railties/databases.rake#L590
• https://github.com/rails/rails/blob/3-2-stable/railties/lib/rails/railtie.rb#L164
• https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/migration.rb#L484

Solution: rename 1_migration.rb to 1_acts_as_taggable_on_migration.rb since the migration.name is derived from the filename. Similarly, I had to rename migration to 2_add_missing_unique_indices.rb to match the class name in the file

Command: rake railties:install:migrations FROM=acts_as_taggable_on_engine or rake acts_as_taggable_on_engine:install:migrations

[bf4]

Hi ❓

[parndt]

@bf4 did you figure out the _engine problem? That happens for my seo_meta plugin but not for the Refinery ones, I think it's something to do with engine_name in the < Rails::Engine declaration.

[bf4]

@parndt Yeah. See comments above "rename 1_migration.rb to 1_acts_as_taggable_on_migration.rb since the migration.name is derived from the filename. Similarly, I had to rename migration to 2_add_missing_unique_indices.rb to match the class name in the file"

[bf4]

Hello old friend, I've come to bump you once again @tilsammans @mbleigh

[bf4]

Need help maintaining this?

[tilsammans]

@bf4 yes absolutely but I cannot add you as a collaborator to this repo. You'll have to stalk @mbleigh on Twitter.

[bf4]

Alrighty then

[ghost]

As far as I can tell there is no code in this gem to handle exceptions triggered by simultaneous writes of tag names to the newly introduced unique index. Am I correct, and is this by design? Do I need to handle exceptions when calling save_tags?

[seuros]

Correct, you have to handle the exception in your application. Unless you have a very active application with multiple instances, the exception handling won't be required.

[ghost]

I have to admit that that sounds like wishful thinking to me and I'm sort of surprised avoiding this issue is not a goal in this project. I don't have a particularly active application but I definitely found a duplicate tag exception in my logs (of course, it might just have been someone double-submitting a form). Is this problem so difficult to solve that a solution is not appropriate for this gem? Should I open a new issue and submit a pull request if I can solve the problem? At minimum, a mention that tags may not save correctly during normal operation seems to me like it's worth a mention in the project documentation. Thanks for sharing this gem and and for your guidance on this issue,

Andrew

[seuros]

A PR is welcome if it include tests. The reason i think why gems don't handle exception is that it depend on the adapter you are using. Also there is a multitude of exceptions that need to be handled :

• Duplicate entry
• Database off-line
• Database restarted
• Table locked
• etc

[ghost]

Thanks for those points. I tend to think of those other issues as things that would happen during "normal" operating conditions. However, I see your point that you don't know what exception to expect when a duplicate tag is saved. It might be sufficient just to handle all exceptions and reattempt saving tags some configured number of times to avoid most failures.

[seuros]

I do know the exception that is raised. But it raised by the adapter (pg in my case) not ActiveRecord.
ActiveRecord test the uniqueness of the tag, the exception is raised only another process insert a record will it being validated or if the model is saved without validation.

I think it ActiveRecord that need to be fixed to handle this type exceptions.

[bf4]

This should be a new issue re: catching db-level exceptions. The db index
was added to reflect the existing AR validations. It's an interesting
question, but as @seuros wrote, the problem is really that
validates_uniqueness doesn't really validate uniqueness. See Ernie
Miller's talk 'An Intervention for ActiveRecord'