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

Replace CollectionProxy#load_target in PaperTrail::Model#record_destroy with CollectionProxy#reset #811

Closed

Conversation

sedx
Copy link
Contributor

@sedx sedx commented May 18, 2016

With this line, when object destroyed and version was created PaperTrail load associated version. It's bring useless query to DB.

From log:

Started DELETE "/deploys/48" for 127.0.0.1 at 2016-05-18 12:29:33 +0500
User Load (0.5ms) SELECT users.* FROM users WHERE users.active = 1 AND users.deleted_at IS NULL AND users.id = 1 ORDER BY users.id ASC LIMIT 1
Processing by DeploysController#destroy as HTML
...
SQL (0.4ms) UPDATE deploys SET deploys.deleted_at = '2016-05-18 07:29:33' WHERE deploys.id = 48
SQL (0.4ms) INSERT INTO user_actions (item_id, item_type, event, object, whodunnit, created_at) VALUES (48, 'Deploy', 'destroy', 'OBJECT_INFO', '1', '2016-05-18 07:29:33')
#useless request to DB
UserAction Load (0.6ms) SELECT user_actions.* FROM user_actions WHERE user_actions.item_id = 48 AND user_actions.item_type = 'Deploy' ORDER BY created_at DESC, user_actions.created_at ASC, user_actions.id ASC

(87.9ms) COMMIT
Redirected to http://server.dev/deploys
Completed 302 Found in 113ms (ActiveRecord: 89.9ms)

user_actions is a custom version class:

class Deploy < ActiveRecord::Base
  has_paper_trail class_name: "UserAction",
                  versions: :user_actions,
                  version: :user_action,
                  unless: Proc.new{ PaperTrail.whodunnit.blank? },
                  skip: ( base::UNTRACKED_ATTRIBUTES || [] ) + ALWAYS_UNTRACKED
end

module PaperTrail
  class Version < ActiveRecord::Base
    PaperTrail::Rails::Engine.eager_load!
      include PaperTrail::VersionConcern
      self.abstract_class = true
  end
end

class UserAction < PaperTrail::Version
  belongs_to :user, ->{unscope(:where)}, foreign_key: :whodunnit
  default_scope ->{order('created_at DESC')}
  self.table_name = :user_actions
end

I'm not sure that my solution is correct, because I don't actually know why need to call#load_target, but tests was not broken.

There are bug_report file, if interested.

With this line, when object destroyed and version was createdPaper Trail load associated version. It's bring
useless query to DB.
@sedx sedx force-pushed the fix_loading_association_on_destroy branch from 1177eb1 to 2dd4ce8 Compare May 18, 2016 11:52
@jaredbeck
Copy link
Member

I'm not sure that my solution is correct, because I don't actually know why need to call#load_target, but tests was not broken.

By using git blame, I was able to find this discussion of load_target: #144, which I'm still reading.

@jaredbeck
Copy link
Member

Instead of removing this line, how about using CollectionProxy#reset? That way, we know the association will not be cached.

…llectionProxy#load_target.

Also add bullet integration to associations_tests for more logical test problem with N+1 query.
Bullet disabled for counter caches, but still catch N+1 queries.
Also add skip_bullet lambda, to disable bullet for specific test. Used it in test of "mark for destruction" cases, because without it produce n+1 query, but when add includes(:author), request to load authors drop mark for destruction flag.
@sedx sedx changed the title Remove load_target for PaperTrail::Model#record_destroy Replace CollectionProxy#load_target in PaperTrail::Model#record_destroy with CollectionProxy#reset May 19, 2016
@sedx
Copy link
Contributor Author

sedx commented May 19, 2016

@jaredbeck Thanks for advice! I'm don't know about CollectionProxy#reset. Replace CollectionProxy#load_target with CollectionProxy#reset. Also update tests for more simple detect N+1 query in assotiation's tests

jaredbeck added a commit that referenced this pull request May 20, 2016
@jaredbeck
Copy link
Member

Thanks, Ilya! Merged offline in 0eba14d, without bullet.

I was unsure about introducing bullet. Let's discuss it in a separate PR.

@jaredbeck jaredbeck closed this May 20, 2016
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.

2 participants