Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes a problem of ambiguous table names when using only_deleted method and joining tables that have a scope on with_deleted #346

Conversation

thromera
Copy link
Contributor

Fixes #26 and #27 (that are still present).

This bug would happen when overriding sentinel with active field

# paranoid.rb
module Paranoid
  extend ActiveSupport::Concern

  # See index section here: https://github.com/radar/paranoia

  included do
    acts_as_paranoid column: :active, sentinel_value: true
    include Overrides
  end

  # If not defined in a separate module, Paranoia will be prepended before Paranoid, causing these methods to be later
  # in the call chain
  module Overrides
    def paranoia_restore_attributes
      {
        deleted_at: nil,
        active: true
      }
    end

    def paranoia_destroy_attributes
      {
        deleted_at: current_time_from_proper_timezone,
        active: nil
      }
    end
  end
end
#post.rb
class Post < ActiveRecord::Base
    include Paranoid

    belongs_to :author, -> {with_deleted}
    belongs_to :event, -> {with_deleted}

    validates_presence_of :author, :event
end
#author.rb
class Author < ActiveRecord::Base
    include Paranoid

    has_many :posts, -> {with_deleted}, dependent: :destroy
end
#event.rb
class Event < ActiveRecord::Base
    include Paranoid

    has_many :posts, -> {with_deleted}
end
Post.only_deleted.joins(:event)
2.2.3 :001 > Post.only_deleted.joins(:event)
  Post Load (10.1ms)  SELECT "posts".* FROM "posts" INNER JOIN "events" ON "events"."id" = "posts"."event_id" AND "events"."active" = ? WHERE ("active" IS NULL OR "active" != 't')  [["active", "t"]]
ActiveRecord::StatementInvalid: SQLite3::SQLException: ambiguous column name: active: SELECT "posts".* FROM "posts" INNER JOIN "events" ON "events"."id" = "posts"."event_id" AND "events"."active" = ? WHERE ("active" IS NULL OR "active" != 't')

@@ -5,6 +5,7 @@

test_framework = defined?(MiniTest::Test) ? MiniTest::Test : MiniTest::Unit::TestCase


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

end



Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@BenMorganIO
Copy link
Collaborator

The commit for this doesn't lead to a Github profile and isn't owned by anyone. I'm a bit skeptical security wise.

@thromera
Copy link
Contributor Author

thromera commented Jan 2, 2017

I'm currently in vacation, I will fix that next week, my git config was probably wrong at that time.

@thromera thromera force-pushed the ambiguous_table_names_on_only_deleted_joins branch 3 times, most recently from 70eb9d0 to 8023f10 Compare January 12, 2017 08:34
@thromera
Copy link
Contributor Author

It should be fixed now. (I've also fixed the useless new lines.)

@BenMorganIO
Copy link
Collaborator

Cool. I'd love to get this in, but it looks like you need to rebase the latest changes from the core branch.

@thromera thromera force-pushed the ambiguous_table_names_on_only_deleted_joins branch from a98eb8b to 3b33272 Compare February 7, 2017 10:17
schneems and others added 10 commits February 7, 2017 11:18
The homepage is supposed to be where you can find the code. It is displayed on rubygems.org page. Currently it will redirect you back to the same page. Should point at this github repo.
Touch record on destroy by leveraging the paranoia_destroy_attributes.
Applied the same to the restore-method as this eliminates the extra query.
…od and joining tables that have a scope on `with_deleted`

Update homepage in gemspec

The homepage is supposed to be where you can find the code. It is displayed on rubygems.org page. Currently it will redirect you back to the same page. Should point at this github repo.

Update README to use proper version for Rails 5

Version 2.2.0

Ignore failures from all jruby's on travis

Add explicit language about dependent: :destroy

Update CHANGELOG.md

update ruby and rails versions

Use ActiveSupport.on_load to correctly re-open ActiveRecord::Base. rubysherpas#335

Touch record on paranoia-destroy. Fixes rubysherpas#296

Touch record on destroy by leveraging the paranoia_destroy_attributes.
Applied the same to the restore-method as this eliminates the extra query.
@thromera
Copy link
Contributor Author

thromera commented Feb 7, 2017

I'm closing this PR since I've messed up with my rebase. Sorry about that.
Good news is #379 should be OK to merge.

@thromera thromera closed this Feb 7, 2017
@thromera thromera deleted the ambiguous_table_names_on_only_deleted_joins branch February 7, 2017 10:27
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.

8 participants