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

VirtualDelegate: Fix foreign key for belongs_to #10717

Merged
merged 6 commits into from
Sep 9, 2016

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Aug 23, 2016

We are using delegates more and more.
Found a few bugs with edge cases:

  1. When delegating to an association that has a belongs_to, and the foreign key is non-standard, then it got the foreign key wrong. Example: belongs_to :ems_owner, :class_name => 'User' had a foreign key of user_id instead of ems_owner_id.
  2. When delegating to a self referring reference, it was not returning records. Example: TestClass.belongs_to :parent, :class_name => "TestClass"
  3. Test was accessing postgres due to changes in active record establish_connection. For some reason, this is only an issue when adding a has_one relation.

These changes are necessary to add delegation for ems_owner and improve performance of the vms screen.

/cc @dmetzger57 thoughts on migrating to darga-4?
/cc @NickLaMuro thanks for all the help. pulled tests out of #10476 and puled ar_virtual bug out of #10704

@kbrock kbrock force-pushed the ar_virtual_foreign_key branch 3 times, most recently from aa5af68 to c61b7c3 Compare August 24, 2016 16:41
@kbrock kbrock changed the title Ar virtual foreign key VirtualDelegate: Fix foreign key for belongs_to Aug 30, 2016
@kbrock
Copy link
Member Author

kbrock commented Aug 30, 2016

@Fryguy did you get a chance to look at this one. it is a bug that is blocking me in a number of places

end
end
end
end
end

def self.select_from_alias(to_model, to_ref, col, to_model_col_name, src_model_id)
Copy link
Member

Choose a reason for hiding this comment

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

Minor but can you document this method? Perhaps with examples that demonstrate inputs/outputs. I think that would go a long way to helping me understand the use cases for this and why it fixes the problem in question.

I think what I'm concerned about is that all of those cases in the OP are handled in ActiveRecord already, so it feels like we are reinventing problems they have already solved.

before:
if this table is part of the primary query, then the table alias will
conflict.

after:
use a different table alias so it does not conflict.
the db connection is non standard, and establish_connection is not
properly inheriting the connection from the parent but rather the
default (postgres)

only really shows up once we use has_one tests
this also tests the has_one with self reference
@miq-bot
Copy link
Member

miq-bot commented Sep 2, 2016

Checked commits kbrock/manageiq@dc8e1a3~...519a63b with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍪

@kbrock
Copy link
Member Author

kbrock commented Sep 7, 2016

@Fryguy I added the extra documentation. did you have a chance to review?

Of note, this pr is only +12/-5 code changes. The majority (+185/-3) is docs/specs.

@dmetzger57
Copy link
Contributor

+1 for Darga/yes

@Fryguy Fryguy merged commit b1cc742 into ManageIQ:master Sep 9, 2016
@Fryguy Fryguy added this to the Sprint 46 Ending Sep 12, 2016 milestone Sep 9, 2016
@Fryguy Fryguy added the core label Sep 9, 2016
@kbrock kbrock deleted the ar_virtual_foreign_key branch September 9, 2016 16:48
@chessbyte
Copy link
Member

Darga Backport conflict:

$ git cherry-pick -e -x -m 1 b1cc742 
error: could not apply b1cc742... Merge pull request #10717 from kbrock/ar_virtual_foreign_key
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

$ git status
On branch darga
Your branch is up-to-date with 'upstream/darga'.
You are currently cherry-picking commit b1cc742.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

    modified:   spec/lib/extensions/ar_virtual_spec.rb

Unmerged paths:
  (use "git add <file>..." to mark resolution)

    both modified:   lib/extensions/ar_virtual.rb

$ git diff
diff --cc lib/extensions/ar_virtual.rb
index b87bfb7,ea52bc0..0000000
--- a/lib/extensions/ar_virtual.rb
+++ b/lib/extensions/ar_virtual.rb
@@@ -109,9 -111,38 +109,41 @@@ module VirtualDelegate
        "#{prefix == true ? to : prefix}_" if prefix
      end

++<<<<<<< HEAD
++=======
+     # @param col [String] attribute name
+     # @param to [Symbol] association name of targeted association
+     # @param to_model [Class] association class of targeted association
+     # @param to_ref [Association] association from source class to target association
+     # @return [Proc] lambda to return arel that selects the attribute in a sub-query
+     # @return [Nil] if the attribute (col) can not be represented in sql.
+     #
+     # To generate a proc, the following cases must happen:
+     #   - the column has sql (virtual_column with arel OR real sql attribute)
+     #   - the association has sql representation (a real association has sql)
+     #   - the association is to a single record (has_one or belongs_to)
+     #
+     # example
+     #
+     #   for the given class definition:
+     #
+     #     class Vm
+     #       belongs_to :hosts #, :foreign_key => :host_id, :primary_key => :id
+     #       virtual_delegate :name, :to => :host, :prefix => true, :allow_nil => true
+     #     end
+     #
+     #   The virtual_delegate calls:
+     #
+     #     virtual_delegate_arel("name", :host, Host, Vm.reflection_with_virtual(:host))
+     #
+     #   which will return [a lambda that produces arel that produces] sql
+     #
+     #     (SELECT "hosts"."name" FROM "hosts" WHERE "hosts"."id" = "vms"."host_id")
+ 
++>>>>>>> b1cc742... Merge pull request #10717 from kbrock/ar_virtual_foreign_key
      def virtual_delegate_arel(col, to, to_model, to_ref)
 -      # ensure the column has sql and the association is reachable via sql
 -      # There is currently no way to propagate sql over a virtual association
 +      # column has sql and the association is reachable via sql
 +      # no way to propagate sql over a virtual association
        if to_model.arel_attribute(col) && reflect_on_association(to)
          if to_ref.macro == :has_one
            lambda do |t|

@simaishi
Copy link
Contributor

Darga BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1375206

@miq-bot remove_label bugzilla needed

@kbrock
Copy link
Member Author

kbrock commented Sep 12, 2016

How I was able to cleanly merge:

PR merge sha comments
#8486 6f0e226 #used_disk_storage delegate to hardware. sql vs N+1.
#10653 a8ecd0d #used_disk_storage virtual delegate. (virtual_attribute :uses param)
#10754 c2d1bf7 virtual_delegate_arel docs and improvements
#10717 b1cc742 virtual_delegates belongs_to improvements

@chessbyte all of these have performance improvements and are minimum risk.
You want a single darga merge, or want me to get bzs into these?

@chessbyte
Copy link
Member

@simaishi @dmetzger57 if we want this backported to Darga, I think we need a 5.6.z BZ

@simaishi
Copy link
Contributor

simaishi commented Oct 7, 2016

chessbyte pushed a commit that referenced this pull request Oct 7, 2016
VirtualDelegate: Fix foreign key for belongs_to
(cherry picked from commit b1cc742)

https://bugzilla.redhat.com/show_bug.cgi?id=1375206
@chessbyte
Copy link
Member

Darga Backport details:

$ git log
commit b9672b0f07bc28fbb93883118ab7dd6237474c4c
Author: Jason Frey <fryguy9@gmail.com>
Date:   Fri Sep 9 12:47:36 2016 -0400

    Merge pull request #10717 from kbrock/ar_virtual_foreign_key

    VirtualDelegate: Fix foreign key for belongs_to
    (cherry picked from commit b1cc742a339b0bb3d766fb84ca7137a69d925faa)

    https://bugzilla.redhat.com/show_bug.cgi?id=1375206

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants