Fixes #27185 - dirty association after save #6881

sseelam2 · 2019-07-08T22:37:51Z

Created this PR to obtain consistent results for _changed? Dirty Associations was returning true even after saving the changes to the database.

theforeman-bot · 2019-07-08T22:37:55Z

@sseelam2, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #27185: Can't register a system using activation key with system purpose. is the correct one
move ticket #27185 from Katello to the Foreman project
or file a new ticket in the Foreman project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2019-07-08T22:37:55Z

Issues: #27185

theforeman-bot · 2019-07-08T22:46:07Z

@sseelam2, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #27185: Can't register a system using activation key with system purpose. is the correct one
move ticket #27185 from Katello to the Foreman project
or file a new ticket in the Foreman project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

ares

👍 on the direction, 2 small concerns

ares · 2019-07-09T06:59:39Z

app/models/concerns/dirty_associations.rb

  module ClassMethods
    # usage:
    #   class Model
    #     dirty_has_many_associations :organizations, :locations
    def dirty_has_many_associations(*args)
      extension = Module.new do
+        define_method "reset_state" do


reset_state is quite generic name, given this module is included in many models, how about reset_dirty_cache_state?

Yes. That sounds better. Thanks. 👍

ares · 2019-07-09T07:04:32Z

app/models/concerns/dirty_associations.rb

  module ClassMethods
    # usage:
    #   class Model
    #     dirty_has_many_associations :organizations, :locations
    def dirty_has_many_associations(*args)
      extension = Module.new do
+        define_method "reset_state" do
+          args.each do |assoc|


Given we iterate over args inside the method, this won't work if someone callse dirty_has_many_associations in one model several times. If you move it to this block, you can define reset method per each arg. An example of a model that calls dirty_has_many_associations twice - ProvisioningTemplate. Once in the model definition itself, second time thanks to including Taxonomix concern.

By the time after save is called args get replaced with the args of the last dirty association even if I move the reset_state code to the block. Could please clarify if my understanding is right? @ares

I don't think that's what happens. When dirty_has_many_associations is called, it only defines new methods, so you can call it many times. The method you're adding should follow the same pattern I think. I'll comment more in the code.

Thanks for clarifying.

theforeman-bot · 2019-07-09T07:05:25Z

@sseelam2, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #27185: Can't register a system using activation key with system purpose. is the correct one
move ticket #27185 from Katello to the Foreman project
or file a new ticket in the Foreman project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2019-07-09T07:05:26Z

@sseelam2, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #27185: Can't register a system using activation key with system purpose. is the correct one
move ticket #27185 from Katello to the Foreman project
or file a new ticket in the Foreman project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

ares · 2019-07-09T07:06:40Z

I moved the redmine issue for you to Foreman. Note that the patch break tests though. It seems to change some behavior we rely on.

houndci-bot · 2019-07-11T00:55:14Z

test/unit/dirty_associations_test.rb

+      end
+    end
+
+    if !ActiveRecord::Base.connection.table_exists? 'dummy_dirty_association'


Style/NegatedIf: Favor unless over if for negative conditions.

houndci-bot · 2019-07-11T00:55:15Z

test/unit/dirty_associations_test.rb

+      end
+    end
+
+    if !ActiveRecord::Base.connection.table_exists? 'third_mock'


Style/NegatedIf: Favor unless over if for negative conditions.

houndci-bot · 2019-07-11T00:55:15Z

test/unit/dirty_associations_test.rb

+      end
+    end
+
+    if !ActiveRecord::Base.connection.table_exists? 'second_mock'


Style/NegatedIf: Favor unless over if for negative conditions.

houndci-bot · 2019-07-11T00:55:15Z

test/unit/dirty_associations_test.rb

 class DirtyAssociationsTest < ActiveSupport::TestCase
  def setup
+    if !ActiveRecord::Base.connection.table_exists? 'first_mock'


Style/NegatedIf: Favor unless over if for negative conditions.

houndci-bot · 2019-07-11T00:55:15Z

app/models/concerns/dirty_associations.rb

    def dirty_has_many_associations(*args)
+      @associations = @associations + args


Style/SelfAssignment: Use self-assignment shorthand +=.

jturel · 2019-07-11T14:02:28Z

test/unit/dirty_associations_test.rb

@@ -1,6 +1,7 @@
 require 'test_helper'

-class DummyDirtyAssociationsModel
+class DummyDirtyAssociationsModel < ApplicationRecord


I think it's good to have an example tested that doesn't extend ApplicationRecord to show that the existing behavior doesn't actually require a real database model. Is it necessary to change this?

If ApplicationRecord is removed, DummyDirtyAssociationsModel becomes a class and after_save will not work on it.

you can implement/mock after_save on plain class too

I think we should preserve the original behavior which can be done like Marek mentioned or conditionally defining the after_save callback if the class actually responds to such a method

@sseelam2 this concern does not seem incorporated, do you plan to change it?

@ares Yes. I'm on it.

ares · 2019-07-16T11:06:10Z

app/models/concerns/dirty_associations.rb

      extension = Module.new do
        args.each do |association|
          association_ids = association.to_s
          association_ids = association_ids.singularize + '_ids' unless association.to_s.end_with?('_ids')

+          define_method "reset_dirty_cache_state" do


since you now define this in args.each block, the name should be dynamic, something like define_method "reset_#{association}_dirty_cache_state"

ares · 2019-07-16T11:09:43Z

app/models/concerns/dirty_associations.rb

  module ClassMethods
    # usage:
    #   class Model
    #     dirty_has_many_associations :organizations, :locations
    def dirty_has_many_associations(*args)
      extension = Module.new do
+        define_method "reset_state" do
+          args.each do |assoc|


I don't think that's what happens. When dirty_has_many_associations is called, it only defines new methods, so you can call it many times. The method you're adding should follow the same pattern I think. I'll comment more in the code.

ares

I added more comments, hope it describes requested changes thoroughly enough

ares · 2019-07-16T11:15:34Z

app/models/concerns/dirty_associations.rb

      extension = Module.new do
        args.each do |association|
          association_ids = association.to_s
          association_ids = association_ids.singularize + '_ids' unless association.to_s.end_with?('_ids')

+          define_method "reset_dirty_cache_state" do
+            dirty_associations.each do |assoc|


since you now moved this into a block where association represent the association we define the cache for, you shouldn't be iterating over all dirty associations here. Just define reset_#{association}_dirty_cache_state method that only cleans up one attribute, so IMHO it should look like this

define_method "reset_#{association}_dirty_cache_state" do instance_variable_set("@#{association_ids}_changed", false) instance_variable_set("@#{association_ids}_was", nil) end

then the (new) reset_dirty_cache_state method should only iterate over associations stored in @@dirt_associations array and call respective reset_#{association}_dirty_cache_state methods

I have made the change and the tests pass, but I get the below error in the rails log since Taxonomix is a module. @ares

18:49:35 rails.1 | 2019-07-16T18:49:35 [F|app|f48f8ad3] NoMethodError (undefined method `+' for nil:NilClass): 18:49:35 rails.1 | 2019-07-16T18:49:35 [F|app|f48f8ad3] 18:49:35 rails.1 | 2019-07-16T18:49:35 [F|app|f48f8ad3] app/models/concerns/dirty_associations.rb:38:in `dirty_has_many_associations' 18:49:35 rails.1 | | app/models/concerns/taxonomix.rb:21:in `block in <module:Taxonomix>' 18:49:35 rails.1 | | app/models/auth_sources/auth_source_ldap.rb:30:in `include' 18:49:35 rails.1 | | app/models/auth_sources/auth_source_ldap.rb:30:in `<class:AuthSourceLdap>' 18:49:35 rails.1 | | app/models/auth_sources/auth_source_ldap.rb:21:in `<top (required)>' 18:49:35 rails.1 | | app/controllers/api/v2/auth_source_ldaps_controller.rb:6:in `<class:AuthSourceLdapsController>' 18:49:35 rails.1 | | app/controllers/api/v2/auth_source_ldaps_controller.rb:3:in `<module:V2>' 18:49:35 rails.1 | | app/controllers/api/v2/auth_source_ldaps_controller.rb:2:in `<module:Api>' 18:49:35 rails.1 | | app/controllers/api/v2/auth_source_ldaps_controller.rb:1:in `<top (required)>' 18:49:35 rails.1 | | lib/foreman/middleware/telemetry.rb:10:in `call' 18:49:35 rails.1 | | lib/foreman/middleware/catch_json_parse_errors.rb:9:in `call' 18:49:35 rails.1 | | lib/foreman/middleware/logging_context_session.rb:22:in `call' 18:49:35 rails.1 | | lib/foreman/middleware/logging_context_request.rb:11:in `call' 18:49:35 rails.1 | | /home/vagrant/katello/lib/katello/prevent_json_parsing.rb:12:in `call'

Is this still the issue? the trace points to line 38 but I guess in current version it's line 35 that would be failing. How do you reproduce it?

In modules, we may need mattr_accessor instead but that would work with classes again (same issue as with @@). Perhaps we could use

class << self attr_accessor :dirty_associations end

that should work both in class and module and keep separate storage even if module is included in the class.

ares · 2019-07-16T11:19:55Z

app/models/concerns/dirty_associations.rb

    def dirty_has_many_associations(*args)
+      @@dirty_associations ||= []


don't use class variables like this, this would cause problems with class inheritance, note that @@vars are shared between parents and children classes. It's bettter to use class_attribute or simply @var since this is a class scope already (note we're in module ClassMethods), simply replacing @@ with @ should do.

Yes. That makes sense. Made the change you suggested.

ares · 2019-07-16T11:20:04Z

app/models/concerns/dirty_associations.rb

    def dirty_has_many_associations(*args)
+      @@dirty_associations ||= []
+      @@dirty_associations += args
+      dirty_associations = @@dirty_associations


we don't need this I believe

ares · 2019-07-16T11:21:10Z

also you need to rebase this for rubocop to pass and tests to run

jturel · 2019-07-24T15:03:04Z

[test foreman]

jturel · 2019-07-24T18:04:05Z

Looks like there's lint in the user model test:

[vagrant@centos7-devel foreman]$ bundle exec rubocop test/models/user_test.rb 
Inspecting 1 file
C

Offenses:

test/models/user_test.rb:379:21: C: Layout/SpaceAroundOperators: Operator = should be surrounded by a single space.
    extra_role      = Role.where(:name => "foobar").first_or_create

jturel · 2019-08-01T15:39:39Z

[test foreman]
[test katello]

jturel · 2019-08-02T01:25:05Z

[test katello]

jturel · 2019-08-02T14:47:38Z

test/unit/dirty_associations_test.rb

+
+    @first_mock.second_mock_ids = []
+    @first_mock.third_mock_ids = []
+    @first_mock.save!


One request for this test, since it is very much concerned with the state of _changed?, is to assert that _changed? is true before calling save! This defines the behavior we expect more precisely and may help catch regressions in the future.

Yes. Thanks for pointing that out. @jturel

ianballou · 2019-08-02T19:36:24Z

[test katello]

sseelam2 · 2019-08-05T20:49:20Z

[test katello]

ares

Code wise it looks good now, not sure what's with [test katello] that got sigtermed. There's still one concern with the test that I think hasn't been addressed, so I'm keeping "request changes" status. I'll proceed with testing now, since I don't expect changes to app code.

ares · 2019-08-06T13:33:29Z

I tested the change and it works fine! Please keep the update as a separate commit, I'll squash on merge.

jturel

Nice work @sseelam2 - no further suggestions from me :)

ares · 2019-08-07T07:23:03Z

app/models/concerns/dirty_associations.rb

@@ -14,16 +14,35 @@
 module DirtyAssociations
  extend ActiveSupport::Concern

+  included do
+      after_save :reset_dirty_cache_state


I don't like this change tbh, this module is always expected to be included in active record model, which always has after_save defined. It's just the test that we wanted to keep simple and therefore we don't use the model there.

I was hoping to actually modify the test model so that it would define after_save even though it does not inherit from ActiveRecord. But if that does not feel right, I'd prefer the original version. Hence, please drop this commit from the PR and I'll merge. Sorry for going back and forth.

sseelam2 · 2019-08-08T02:16:14Z

Since the model is always included in active record model, I have removed my last commit.

ares

Thanks @sseelam2, great work in complicated area, merging!

sseelam2 · 2019-08-08T11:58:08Z

Thanks @sseelam2, great work in complicated area, merging!

Thanks for all your feedback @ares and @jturel. Couldn't have completed it without you guys.

theforeman-bot added the Waiting on contributor label Jul 8, 2019

theforeman-bot added Needs testing Not yet reviewed labels Jul 8, 2019

ares requested changes Jul 9, 2019

View reviewed changes

theforeman-bot removed the Not yet reviewed label Jul 9, 2019

sseelam2 force-pushed the act_key_bug branch from ee34c87 to 849ccaa Compare July 11, 2019 00:55

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jul 11, 2019

houndci-bot reviewed Jul 11, 2019

View reviewed changes

sseelam2 force-pushed the act_key_bug branch from 849ccaa to c0c8a89 Compare July 11, 2019 13:48

jturel reviewed Jul 11, 2019

View reviewed changes

sseelam2 force-pushed the act_key_bug branch from c0c8a89 to 5d2de3b Compare July 11, 2019 18:32

ares reviewed Jul 16, 2019

View reviewed changes

ares requested changes Jul 16, 2019

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Jul 16, 2019

sseelam2 force-pushed the act_key_bug branch from 5d2de3b to fe2f44a Compare July 16, 2019 18:37

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jul 16, 2019

sseelam2 changed the title ~~Fixes #27185 - dirty association change~~ Fixes #27185 - dirty association after save Jul 16, 2019

sseelam2 force-pushed the act_key_bug branch from fe2f44a to ac08dc8 Compare July 24, 2019 16:08

sseelam2 force-pushed the act_key_bug branch 2 times, most recently from 9ff27c1 to 363e25c Compare August 1, 2019 13:36

sseelam2 force-pushed the act_key_bug branch 2 times, most recently from 6ae4503 to 75e4066 Compare August 1, 2019 19:06

jturel reviewed Aug 2, 2019

View reviewed changes

Fixes #27185 - dirty association after save

35b127c

sseelam2 force-pushed the act_key_bug branch from 75e4066 to 35b127c Compare August 5, 2019 13:17

ares requested changes Aug 6, 2019

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Aug 6, 2019

theforeman-bot added Needs re-review and removed Waiting on contributor labels Aug 7, 2019

jturel previously approved these changes Aug 7, 2019

View reviewed changes

theforeman-bot removed the Needs re-review label Aug 7, 2019

ares requested changes Aug 7, 2019

View reviewed changes

theforeman-bot added the Waiting on contributor label Aug 7, 2019

sseelam2 dismissed jturel’s stale review via 35b127c August 8, 2019 02:11

sseelam2 force-pushed the act_key_bug branch from 9da4428 to 35b127c Compare August 8, 2019 02:11

theforeman-bot added Needs re-review and removed Waiting on contributor labels Aug 8, 2019

ares approved these changes Aug 8, 2019

View reviewed changes

theforeman-bot removed the Needs re-review label Aug 8, 2019

ares merged commit 02a24ab into theforeman:develop Aug 8, 2019

		def dirty_has_many_associations(*args)
		@associations = @associations + args

		def dirty_has_many_associations(*args)
		@@dirty_associations \|\|= []

Fixes #27185 - dirty association after save #6881

Fixes #27185 - dirty association after save #6881

Conversation

sseelam2 commented Jul 8, 2019 • edited Loading

theforeman-bot commented Jul 8, 2019

theforeman-bot commented Jul 8, 2019

theforeman-bot commented Jul 8, 2019

ares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sseelam2 Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theforeman-bot commented Jul 9, 2019

theforeman-bot commented Jul 9, 2019

ares commented Jul 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ares commented Jul 16, 2019

jturel commented Jul 24, 2019

jturel commented Jul 24, 2019

jturel commented Aug 1, 2019

jturel commented Aug 2, 2019

jturel Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou commented Aug 2, 2019

sseelam2 commented Aug 5, 2019

ares left a comment

Choose a reason for hiding this comment

ares commented Aug 6, 2019

jturel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sseelam2 commented Aug 8, 2019

ares left a comment

Choose a reason for hiding this comment

sseelam2 commented Aug 8, 2019

sseelam2 commented Jul 8, 2019 •

edited

Loading

sseelam2 Jul 9, 2019 •

edited

Loading

jturel Aug 2, 2019 •

edited

Loading