api sort on sql friendly virtual attributes #13409

jntullo · 2017-01-09T17:26:00Z

This updates the sort directive to allow for sorting on sql friendly virtual attributes. Adds an additional error message to indicate the collection cannot be sorted by the non-sql friendly virtual attribute.

GitHub issue API sort-by should work with sql friendly virtual attributes #13380
Pivotal Issue

@miq-bot assign @abellotti
@miq-bot add_label api, enhancement

cc: @jeff-phillips-18

imtayadeway · 2017-01-09T18:30:33Z

app/controllers/api/base_controller/parameters.rb

@@ -161,6 +161,8 @@ def sort_params(klass)
        options = String(params['sort_options']).split(",")
        params['sort_by'].split(",").zip(orders).collect do |attr, order|
          if klass.attribute_method?(attr) || klass.method_defined?(attr) || attr == klass.primary_key
+            raise BadRequestError,
+                  "#{klass.name} cannot be sorted by #{attr}" unless klass.attribute_supported_by_sql?(attr)


Just wondering if attribute_supported_by_sql? implies the condition on 163? Could this be simplified?

@imtayadeway no it's not implied on 163. I will try to simplify it, but I also want to send back that specific error message, rather than the one below that indicates it is not an attribute (since it is, just not a sortable one)

imtayadeway · 2017-01-09T18:34:01Z

app/controllers/api/base_controller/parameters.rb

-        sort_item << " ASC"  if order && order.downcase.start_with?("asc")
-        sort_item << " DESC" if order && order.downcase.start_with?("desc")
-        sort_item
+        if options.map(&:downcase).include?("ignore_case")


Is this a change of behavior or a refactoring?

@imtayadeway change of behavior. Format needs to be { :attribute => order } rather than the <attribute> <ORDER>

Does this preserve the behavior of doing ignore case/descending?

@imtayadeway good catch I'll update it

@jntullo if this was green we must have insufficient test coverage here....can you add some to verify existing behavior? Thanks!

@imtayadeway so, afaik (spent a bit of time diving into this) it's not possible to add the LOWER to the hash that's needed to sort on the virtual_attributes. Because it was getting a little icky, I decided to account for that by adding in another method for sorting of virtual attributes, but not sure how I feel about that. Thoughts, suggestions?

Maybe @kbrock can provide some insight into case-insensitive sort on virtual attributes? reorder(LOWER(attr) does not work, and it requires the format .reorder({:attr => 'asc'}). Is there a way to add case insensitive sort to this?

kbrock

This is looking great.

Just one main comment.
Possibly, it will allow you to keep all in sort_directive and not have to implement 2 different methods for this to work

kbrock · 2017-01-10T22:40:54Z

app/controllers/api/base_controller/parameters.rb

+
+      def sort_virtual_attr_directive(klass, attr, order, options)
+        raise BadRequestError,
+              "#{klass.name} cannot be sorted by #{attr}" unless klass.attribute_supported_by_sql?(attr)


This is great.

In reports/views, we do support sorting by non sql friendly attributes.
It is a major memory hit and very slow.

kbrock · 2017-01-10T22:41:43Z

app/controllers/api/base_controller/parameters.rb

@@ -175,6 +177,18 @@ def sort_directive(attr, order, options)
        sort_item << " DESC" if order && order.downcase.start_with?("desc")
        sort_item
      end
+
+      def sort_virtual_attr_directive(klass, attr, order, options)


darn, was hoping that virtual and regular ones can be implemented together.

kbrock · 2017-01-10T22:45:37Z

app/controllers/api/base_controller/parameters.rb

+        raise BadRequestError,
+              "#{klass.name} cannot be sorted with ignored case" if options.map(&:downcase).include?("ignore_case")
+        if order
+          { attr.to_sym => order.downcase }


maybe something like:

klass.arel_attribute(attr).downcase.send(order.downcase)

I wonder if there is a way for us to reuse miq_report/search.rb#get_order_info

Also, a while ago, arel_attribute().lower was not supported for either attributes or virtual attributes - can't quite remember. So instead of the Arel::Nodes::Lower you may want to just try .lower and see if it will work

@kbrock that worked! Thank you!

kbrock · 2017-01-11T15:00:06Z

kicking

kbrock · 2017-01-11T15:01:19Z

app/controllers/api/base_controller/parameters.rb

@@ -160,20 +160,24 @@ def sort_params(klass)
        orders = String(params['sort_order']).split(",")
        options = String(params['sort_options']).split(",")
        params['sort_by'].split(",").zip(orders).collect do |attr, order|
-          if klass.attribute_method?(attr) || klass.method_defined?(attr) || attr == klass.primary_key
-            sort_directive(attr, order, options)
+          if klass.virtual_attribute?(attr) && !klass.attribute_supported_by_sql?(attr)


you can probably drop klass.virtual_attribute?(attr)

heck, can you just do:

if klass.attribute_supported_by_sql?(attr) sort_directive(klass, attr, order, options) else raise BadRequestError, "#{klass.name} cannot be sorted by #{attr}" end

(I probably missed something obvious here - e.g.: may need attr == klass.primary_key)

@kbrock would definitely be more simplistic, but I thought it might be a good idea to have both the is not a valid attribute and cannot be sorted by messages separate for clarification

ok

I don't think this part is necessary:

klass.attribute_method?(attr) || klass.method_defined?(attr) || attr == klass.primary_key

@abellotti you ok just using klass.attribute_supported_by_sql?(attr) instead and dropping the ever neat previous one?

Did we need to support any ruby method or only fields?

While the simpler path is more elegant, I like what @jntullo did to have the 2 different error messages. I'm ok with this approach here as this is low count iteration.

kbrock · 2017-01-11T15:02:15Z

app/controllers/api/base_controller/parameters.rb

-        sort_item << " ASC"  if order && order.downcase.start_with?("asc")
-        sort_item << " DESC" if order && order.downcase.start_with?("desc")
-        sort_item
+      def sort_directive(klass, attr, order, options)


whoa - now THAT is what I'm talking about

imtayadeway · 2017-01-11T15:53:46Z

app/controllers/api/base_controller/parameters.rb

-        sort_item
+      def sort_directive(klass, attr, order, options)
+        if options.map(&:downcase).include?("ignore_case") && order
+          klass.arel_attribute(attr).lower.send(order.downcase)


Seems unlikely, but could send be exploited? At the very least I think public_send might be better.

abellotti

👍 very nice enhancement @jntullo. Thanks !!

abellotti · 2017-01-12T20:31:34Z

spec/requests/api/querying_spec.rb

+      run_get vms_url, :sort_by => 'host_name', :sort_order => 'desc', :expand => 'resources'
+
+      expect_query_result(:vms, 2, 2)
+      expect_result_resources_to_match_hash([{'name' => 'vm_bar'}, {'name' => 'vm_foo'}])


if we're sorting in descending order by host_name, shouldn't the list coming back be vm_foo then vm_bar ?

Huh. Is there a bug in the core for sorting?

Curious what the sql is for this query?

@kbrock oh...yikes...I totally missed that

@abellotti turned out to be a mistake in the test. a facepalm moment. good to go now 👍

abellotti · 2017-01-12T20:43:08Z

app/controllers/api/base_controller/parameters.rb

@@ -160,20 +160,24 @@ def sort_params(klass)
        orders = String(params['sort_order']).split(",")
        options = String(params['sort_options']).split(",")
        params['sort_by'].split(",").zip(orders).collect do |attr, order|
-          if klass.attribute_method?(attr) || klass.method_defined?(attr) || attr == klass.primary_key
-            sort_directive(attr, order, options)
+          if klass.virtual_attribute?(attr) && !klass.attribute_supported_by_sql?(attr)


While the simpler path is more elegant, I like what @jntullo did to have the 2 different error messages. I'm ok with this approach here as this is low count iteration.

changing from send to public send fix spec simplifying sort params to use supported by sql

imtayadeway · 2017-01-13T23:41:27Z

app/controllers/api/base_controller/parameters.rb

-        sort_item << " DESC" if order && order.downcase.start_with?("desc")
-        sort_item
+      def sort_directive(klass, attr, order, options)
+        if options.map(&:downcase).include?("ignore_case") && order


Nit, but I noticed all three branches had the same code, and two of them contained the same condition. I don't know if something like this is any better:

arel = klass.arel_attribute(attr) if order if options.map(&:downcase).include?("ignore_case") arel = arel.lower end arel = arel.asc if order.downcase == "asc" arel = arel.desc if order.downcase == "desc" # more verbose, but lower cognitive load than dynamic method sending end arel

Think arel.asc is not necessary.

So you can skip the entire line: arel = arel.asc if order.downcase == "asc"

abellotti · 2017-01-16T14:56:43Z

app/controllers/api/base_controller/parameters.rb

+        if order
+          if options.map(&:downcase).include?("ignore_case") && order
+            arel = arel.lower
+          end


no need for && order above, you can replace lines 176-178 with:

arel = arel.lower if options.map(&:downcase).include?("ignore_case")

abellotti · 2017-01-16T15:11:55Z

Thanks @jntullo for the update, LGTM!! Waiting for 🍏

miq-bot · 2017-01-16T15:12:28Z

Checked commits jntullo/manageiq@f1f932d~...3f7034b with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. ⭐

imtayadeway · 2017-01-16T15:55:10Z

spec/requests/api/querying_spec.rb

+      run_get vms_url, :sort_by => 'host_name', :sort_order => 'desc', :expand => 'resources'
+
+      expect_query_result(:vms, 2, 2)
+      expect_result_resources_to_match_hash([{'name' => 'vm_foo'}, {'name' => 'vm_bar'}])


From the outside, it looks like this query returned your objects in the order you created them. I think to prove that they were ordered successfully it may be necessary to create three objects out of order, so that you can see that the result put them into some order in a way that couldn't be an accident.

@jntullo can you create another PR to enhance the tests as per @imtayadeway ? Thanks.

@abellotti will do!

imtayadeway · 2017-01-16T15:55:44Z

spec/requests/api/querying_spec.rb

+    it 'supports sql friendly virtual attributes' do
+      host_foo =  FactoryGirl.create(:host, :name => 'foo')
+      host_bar =  FactoryGirl.create(:host, :name => 'bar')
+      vm_foo = FactoryGirl.create(:vm, :name => 'vm_foo')


Did you try :host => host_foo here?

kbrock · 2017-01-16T17:27:21Z

🎉

sort on sql friendly virtual attributes

f1f932d

miq-bot assigned abellotti Jan 9, 2017

miq-bot added api enhancement labels Jan 9, 2017

imtayadeway reviewed Jan 9, 2017

View reviewed changes

add sort virtual attr directive

83abfb9

kbrock reviewed Jan 10, 2017

View reviewed changes

kbrock closed this Jan 11, 2017

kbrock reopened this Jan 11, 2017

kbrock reviewed Jan 11, 2017

View reviewed changes

imtayadeway reviewed Jan 11, 2017

View reviewed changes

abellotti reviewed Jan 12, 2017

View reviewed changes

use arel_attribute in sort_directive

6370928

changing from send to public send fix spec simplifying sort params to use supported by sql

imtayadeway reviewed Jan 13, 2017

View reviewed changes

abellotti reviewed Jan 16, 2017

View reviewed changes

simplify sort_directive

3f7034b

abellotti merged commit bd3834b into ManageIQ:master Jan 16, 2017

abellotti added this to the Sprint 52 Ending Jan 16, 2017 milestone Jan 16, 2017

imtayadeway reviewed Jan 16, 2017

View reviewed changes

jntullo mentioned this pull request Jan 17, 2017

Update querying specs for virtual attribute sort #13542

Merged

abellotti mentioned this pull request Jan 17, 2017

API sort-by should work with sql friendly virtual attributes #13380

Closed

jntullo deleted the enhancement/sort_on_virtual_attributes branch November 28, 2017 19:43

api sort on sql friendly virtual attributes #13409

api sort on sql friendly virtual attributes #13409

Conversation

jntullo commented Jan 9, 2017 • edited by miq-bot Loading

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

kbrock 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

kbrock commented Jan 11, 2017

Choose a reason for hiding this comment

kbrock Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Jan 12, 2017 • edited Loading

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

abellotti 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

kbrock Jan 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Jan 16, 2017

miq-bot commented Jan 16, 2017

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

kbrock commented Jan 16, 2017

jntullo commented Jan 9, 2017 •

edited by miq-bot

Loading

kbrock Jan 11, 2017 •

edited

Loading

kbrock Jan 12, 2017 •

edited

Loading

kbrock Jan 16, 2017 •

edited

Loading