[Api::Filter] Allow filtering '=' via arrays #1061

NickLaMuro · 2021-07-26T17:55:56Z

Adds functionality to the API to allow filtering a particular column by one of many types of values. This could be particularly useful when filtering via a type column that is an STI column.

The SQL (functionally) that would be generated from the filter would be the following:

SELECT *
  FROM miq_requests
 WHERE (type = "MiqProvisionRequest" OR type = "VmMigrateRequest")

This avoids having to make changes to MiqExpression that would allow for something similar to how where(:type => [a,b,c]) works already, and also ensures that we are using something that can be converted into SQL on the backend.

Links

Alternative proposal PR that doesn't quite work: [WIP] [POC][Api::Filter] Add AND filter priority #1059

QA

I tested this using the following script:

# Usage:  ruby fetch_requests.rb

require 'json'
require 'net/http'
require 'pp'

# Modify these vars if you want to change dates and such
days_ago = 7
states   = %w[pending_approval approved denied]
types    = %w[
               MiqProvisionConfiguredSystemRequest
               MiqProvisionRequest
               OrchestrationStackRetireRequest
               PhysicalServerProvisionRequest
               PhysicalServerFirmwareUpdateRequest
               ServiceRetireRequest
               ServiceReconfigureRequest
               ServiceTemplateProvisionRequest
               VmCloudReconfigureRequest
               VmMigrateRequest
               VmReconfigureRequest
               VmRetireRequest
           ]


miq_uri          = URI("http://localhost:3000")
http             = Net::HTTP.new(miq_uri.host, miq_uri.port)
headers          = { "accept" => "application/json" }

auth_hdr         = headers.merge("authorization" => "Basic " + ["admin:smartvm"].pack('m0'))
token            = JSON.parse(http.get("/api/auth", auth_hdr).body)["auth_token"]

req_hdrs         = headers.merge('X-Auth-Token' => token.to_s)
params           = URI.encode_www_form :attributes        => ["job_plays", "stdout"],
                                       :format_attributes => "stdout=html"

                   # equals:  'created_on>2000-01-01 00:00:00'

params           = "filter[]=created_on%3E#{(Time.now - days_ago * 24 * 60 * 60).to_s.gsub(/ /, '+')}"
                   # equals   'approval_state =~ /pending_approval|approved|denied/'
params          += "&filter[]=approval_state=[#{states.join(",")}]"
                   # equals   'approval_state =~ /MiqProvisionRequest|OrchestrationStackRetireRequest|.../'
params          += "&filter[]=type=[#{types.join(",")}]"
                   # equals:  'reason =~ Auto'
params          += "&filter[]=reason=~Auto"


services_url     = "/api/requests?expand=resources&#{params}"
response         = http.get(services_url, req_hdrs)

pp JSON.parse(response.body)

And confirmed in the log that I was getting a proper query in the database using these filters:

[----] D, [2021-07-26T11:54:18.342990 #97861:3fd074e1a29c] DEBUG -- :    (0.3ms)  SELECT "miq_product_features"."identifier" FROM "miq_product_features" INNER JOIN "miq_roles_features" ON "miq_product_features"."id" = "miq_roles_features"."miq_product_feature_id" WHERE "miq_roles_features"."miq_user_role_id" = $1  [["miq_user_role_id", 1]]
[----] D, [2021-07-26T11:54:18.348058 #97861:3fd074e1a29c] DEBUG -- :   MiqRequest Load (0.3ms)  SELECT "miq_requests".* FROM "miq_requests" WHERE (("miq_requests"."created_on" > '2021-07-19 16:54:18' AND (("miq_requests"."approval_state" = 'pending_approval' OR "miq_requests"."approval_state" = 'approved') OR "miq_requests"."approval_state" = 'denied') AND ((((((((((("miq_requests"."type" = 'MiqProvisionConfiguredSystemRequest' OR "miq_requests"."type" = 'MiqProvisionRequest') OR "miq_requests"."type" = 'OrchestrationStackRetireRequest') OR "miq_requests"."type" = 'PhysicalServerProvisionRequest') OR "miq_requests"."type" = 'PhysicalServerFirmwareUpdateRequest') OR "miq_requests"."type" = 'ServiceRetireRequest') OR "miq_requests"."type" = 'ServiceReconfigureRequest') OR "miq_requests"."type" = 'ServiceTemplateProvisionRequest') OR "miq_requests"."type" = 'VmCloudReconfigureRequest') OR "miq_requests"."type" = 'VmMigrateRequest') OR "miq_requests"."type" = 'VmReconfigureRequest') OR "miq_requests"."type" = 'VmRetireRequest')))
[----] D, [2021-07-26T11:54:18.352262 #97861:3fd074e1a29c] DEBUG -- :   MiqRequest Inst Including Associations (0.1ms - 1rows)
[----] D, [2021-07-26T11:54:18.354745 #97861:3fd074e1a29c] DEBUG -- :   MiqApproval Load (0.2ms)  SELECT "miq_approvals".* FROM "miq_approvals" WHERE "miq_approvals"."miq_request_id" = $1 ORDER BY "miq_approvals"."id" ASC LIMIT $2  [["miq_request_id", 3], ["LIMIT", 1]]
[----] D, [2021-07-26T11:54:18.354951 #97861:3fd074e1a29c] DEBUG -- :   MiqApproval Inst Including Associations (0.1ms - 1rows)
[----] D, [2021-07-26T11:54:18.356607 #97861:3fd074e1a29c] DEBUG -- :   CACHE MiqRequest Load (0.0ms)  SELECT "miq_requests".* FROM "miq_requests" WHERE (("miq_requests"."created_on" > '2021-07-19 16:54:18' AND (("miq_requests"."approval_state" = 'pending_approval' OR "miq_requests"."approval_state" = 'approved') OR "miq_requests"."approval_state" = 'denied') AND ((((((((((("miq_requests"."type" = 'MiqProvisionConfiguredSystemRequest' OR "miq_requests"."type" = 'MiqProvisionRequest') OR "miq_requests"."type" = 'OrchestrationStackRetireRequest') OR "miq_requests"."type" = 'PhysicalServerProvisionRequest') OR "miq_requests"."type" = 'PhysicalServerFirmwareUpdateRequest') OR "miq_requests"."type" = 'ServiceRetireRequest') OR "miq_requests"."type" = 'ServiceReconfigureRequest') OR "miq_requests"."type" = 'ServiceTemplateProvisionRequest') OR "miq_requests"."type" = 'VmCloudReconfigureRequest') OR "miq_requests"."type" = 'VmMigrateRequest') OR "miq_requests"."type" = 'VmReconfigureRequest') OR "miq_requests"."type" = 'VmRetireRequest')))
[----] D, [2021-07-26T11:54:18.360629 #97861:3fd074e1a29c] DEBUG -- :   MiqRequest Inst Including Associations (0.1ms - 1rows)

Inverts the "if" statement to remove the use of `target`, which is confusing to read. Instead the "expr" is built first, and then based on the `:logical_or` value, is put in the proper array. This makes it just a bit easier to logically parse as a human, but is functionally the same.

Makes the collections for `and_expressions` and `or_expressions` `attribute_readers`, so they are available outside of the `parse` method (for a future refactoring).

Makes `.composite_expression` it's own method. Does a few things: - Decreases the size of `.parse`, which is quite large already - Allows future refactoring and enhancement of that method in a more readable fashion

NickLaMuro · 2021-07-26T18:03:22Z

@miq-bot assign @Fryguy

Please review and let us know what you think. This is to help Melody with a form conversion, and doing this avoids having to bring everything back and filter in Ruby, but also avoids needing to touch MiqExpression to allow for this change (higher risk).

cc @kbrock @MelsHyrule

kbrock

This looks very nice Nick.

I'm amused that I want to get rid of a double use variable and introduce a single use one. Those do not have a strong reaction from me.

Really don't like the introduction of the two global variables.
We can discuss if you want, but not a hill I'd die on.

kbrock · 2021-07-26T18:18:46Z

lib/api/filter.rb

      {:logical_or => logical_or, :operator => method, :attr => filter_attr, :value => filter_value}
    end

+    def composite_expression


not a big fan. I liked parse only having local variables.

can you extract this logic while keeping the variables out of the global scope?
composite_expression(and_expressions, or_expressions)

Humbly disagree and don't plan to change. This is pretty idiomatic Ruby to use instance variables (not "global") like this and I think it makes definitions and callers less verbose and wordy.

For example, I picked puma as an example, and the first class I looked at does something pretty much like this with the first method I found:

https://github.com/puma/puma/blob/master/lib/puma/client.rb#L96-L98

In this case, the @ready var is called directly instead of using an attr_reader, but I think the general idea is the same.

So unless I get push back from others, I don't plan to change.

lol. you've made changes like this before. it is your style.

the or expressions and and expressions is only valid within the context of parse
I also am not that fond of how the parsing is being performed, so elevating the local variables to globals further sets it in stone.

Can you be bribed to pass those 2 variables into the local private method composite_expression and do away with these ~~global~~ class variables?

~~global~~ class variables

Instance variables... they are instance variables...

ugh. ^ what he said

and LGTM. lets merge this PR

lib/api/filter.rb

Adds functionality to the API to allow filtering a particular column by one of many types of values. This could be particularly useful when filtering via a `type` column that is an STI column. The SQL that would be generated from the filter would be the following: SELECT * FROM miq_requests WHERE (type = "MiqProvisionRequest" OR type = "VmMigrateRequest") This avoids having to make changes to `MiqExpression` that would allow for something similar to how `where(:type => [a,b,c])` works already, and also ensures that we are using something that can be converted into SQL on the backend.

miq-bot · 2021-07-26T20:01:07Z

Checked commits NickLaMuro/manageiq-api@4dbe1c9~...e607423 with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍪

Fryguy · 2021-09-02T18:55:09Z

Backported to morphy in commit d596a80.

commit d596a80313bc6e6a462d09d53ce6b6b301999e12
Author: Jason Frey <fryguy9@gmail.com>
Date:   Fri Jul 30 10:27:03 2021 -0400

    Merge pull request #1061 from NickLaMuro/array_based_filtering
    
    [Api::Filter] Allow filtering '=' via arrays
    
    (cherry picked from commit b6e5172323285a9dc6c2caf5242f81285b50646b)

[Api::Filter] Allow filtering '=' via arrays (cherry picked from commit b6e5172)

NickLaMuro added 3 commits July 23, 2021 16:29

[Api::Filter] Instance var for and/or expressions

913ede9

Makes the collections for `and_expressions` and `or_expressions` `attribute_readers`, so they are available outside of the `parse` method (for a future refactoring).

[Api::Filter] Add #composite_expression

66f7d85

Makes `.composite_expression` it's own method. Does a few things: - Decreases the size of `.parse`, which is quite large already - Allows future refactoring and enhancement of that method in a more readable fashion

NickLaMuro requested a review from bdunne as a code owner July 26, 2021 17:55

miq-bot assigned Fryguy Jul 26, 2021

NickLaMuro force-pushed the array_based_filtering branch 2 times, most recently from 4011e64 to 080bc01 Compare July 26, 2021 18:09

NickLaMuro mentioned this pull request Jul 26, 2021

[WIP] [POC][Api::Filter] Add AND filter priority #1059

Closed

kbrock reviewed Jul 26, 2021

View reviewed changes

NickLaMuro force-pushed the array_based_filtering branch from 080bc01 to e607423 Compare July 26, 2021 19:56

Fryguy added the enhancement label Jul 28, 2021

This was referenced Jul 28, 2021

[Najdorf] API Interface Changes #1016

Closed

Forms conversion to React ManageIQ/manageiq-ui-classic#6825

Open

Fryguy merged commit b6e5172 into ManageIQ:master Jul 30, 2021

chessbyte added the morphy/yes? label Aug 25, 2021

NickLaMuro mentioned this pull request Aug 25, 2021

[Morphy] API Interface Changes #1067

Closed

1 task

Fryguy added morphy/yes and removed morphy/yes? labels Sep 1, 2021

Fryguy added a commit that referenced this pull request Sep 2, 2021

Merge pull request #1061 from NickLaMuro/array_based_filtering

d596a80

[Api::Filter] Allow filtering '=' via arrays (cherry picked from commit b6e5172)

Fryguy added morphy/backported and removed morphy/yes labels Sep 2, 2021

chessbyte added this to the Morphy milestone Sep 2, 2021

chessbyte mentioned this pull request Jan 6, 2022

[Najdorf] Forms conversion to React ManageIQ/manageiq-ui-classic#8010

Closed

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Api::Filter] Allow filtering '=' via arrays #1061

[Api::Filter] Allow filtering '=' via arrays #1061

Uh oh!

NickLaMuro commented Jul 26, 2021 •

edited

Loading

Uh oh!

NickLaMuro commented Jul 26, 2021

Uh oh!

kbrock left a comment

Uh oh!

kbrock Jul 26, 2021

Uh oh!

NickLaMuro Jul 26, 2021

Uh oh!

kbrock Jul 26, 2021

Uh oh!

NickLaMuro Jul 27, 2021

Uh oh!

kbrock Jul 27, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miq-bot commented Jul 26, 2021

Uh oh!

Fryguy commented Sep 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[Api::Filter] Allow filtering '=' via arrays #1061

[Api::Filter] Allow filtering '=' via arrays #1061

Uh oh!

Conversation

NickLaMuro commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Links

QA

Uh oh!

NickLaMuro commented Jul 26, 2021

Uh oh!

kbrock left a comment

Choose a reason for hiding this comment

Uh oh!

kbrock Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

NickLaMuro Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

kbrock Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

NickLaMuro Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

kbrock Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miq-bot commented Jul 26, 2021

Uh oh!

Fryguy commented Sep 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NickLaMuro commented Jul 26, 2021 •

edited

Loading

kbrock Jul 27, 2021 •

edited

Loading