Sift supports specifying types of scope arguments #48

andrew-wheeler · 2020-05-05T16:06:30Z

Please check out the README.md changes for the idea. I'm extremely open to feedback.

I was motivated to do this as a feature request from @bradleyrzeller

There's some significant refactoring in this PR. I went through several attempts before I arrived at these changes. Hopefully they seem ok. I'll add comments about my rationale.

andrew-wheeler · 2020-05-05T16:07:30Z

.rubocop.yml

@@ -372,7 +372,7 @@ Style/StringLiterals:
 Style/TrailingCommaInArguments:
  Description: 'Checks for trailing comma in argument lists.'
  StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#no-trailing-array-commas'
-  EnforcedStyleForMultiline: comma
+  EnforcedStyleForMultiline: no_comma


This was driving me nuts, but I'm definitely willing to revert this.

andrew-wheeler · 2020-05-05T16:12:40Z

lib/procore-sift.rb

@@ -66,7 +66,16 @@ def sort_fields

  class_methods do
    def filter_on(parameter, type:, internal_name: parameter, default: nil, validate: nil, scope_params: [])
-      filters << Filter.new(parameter, type, internal_name, default, validate, scope_params)
+      if type.respond_to?(:key?)


I'm definitely open to feedback on how to structure supplying the scope types.

andrew-wheeler · 2020-05-05T16:15:42Z

lib/sift/filter.rb

      else
-        handler.call(collection, parameterize(value), params, scope_params)
+        handler(value, params).call(collection)


A fairly large change here: handlers are initialized with data from the request, but they are always called with just the collection. This allows us to treat the default handler, where handler, and scope handler as having the same interface once they are constructed without passing any unused values.

andrew-wheeler · 2020-05-05T16:16:19Z

lib/sift/filter.rb

@@ -38,7 +39,7 @@ def validation_field
    end

    def type_validator
-      @type_validator ||= Sift::TypeValidator.new(param, type)
+      @type_validator ||= Sift::TypeValidator.new(param, validation_type)


This allows us to validate the "chief" parameter to scope filters.

andrew-wheeler · 2020-05-05T16:18:52Z

lib/sift/filter.rb

@@ -51,10 +52,6 @@ def param

    private

-    def parameterize(value)


The handlers take over responsibility for parsing the raw values from the request. This allows the scope handler to use its types to choose the right parsing strategy.

andrew-wheeler · 2020-05-05T16:20:22Z

lib/sift/filter.rb

@@ -73,12 +70,44 @@ def valid_scope_params?(scope_params)
      scope_params.is_a?(Array) && scope_params.all? { |symbol| symbol.is_a?(Symbol) }
    end

-    def handler
-      parameter.handler


We can't have Parameter be responsible for the handler, because scope filters need more than just the "chief" parameter to handle.

andrew-wheeler · 2020-05-05T16:29:53Z

lib/sift/parameter.rb

@@ -8,35 +8,5 @@ def initialize(param, type, internal_name = param)
      @type = type
      @internal_name = internal_name
    end
-


Parameter loses responsibility for parsing values because we want to parse things other than just the "chief" parameter.

andrew-wheeler · 2020-05-05T16:32:31Z

lib/sift/value_parser.rb

-      @supports_boolean = options.fetch(:supports_boolean, false)
-      @supports_ranges = options.fetch(:supports_ranges, false)
-      @supports_json = options.fetch(:supports_json, false)
+    def initialize(value:, type: nil)


ValueParser can be responsible for determining supports_boolean, supports_ranges, and supports_json from the type, so it doesn't need to be passed in. This helps us parse values for things other than the "chief" parameter.

andrew-wheeler · 2020-05-05T16:52:21Z

lib/sift/version.rb

@@ -1,3 +1,3 @@
 module Sift
-  VERSION = "0.13.0".freeze
+  VERSION = "0.14.0".freeze


I'm bumping the minor here because if anything is using the Sift internals (Sift::ValueParser, Sift::Parameter) it could be broken.

amayer171

I think the term "chief parameter" could use some clarification, but I think I know what you are saying its the root type.

amayer171 · 2020-05-05T17:04:57Z

lib/procore-sift.rb

+          raise ArgumentError, "filter_on: Only type: :scope can have subtypes. Expecting the form `type: {scope: [type, {param: type}]}`"
+        end


Maybe move this validation into the Filter class since it has access to type and scope_types it could do this validation there pretty easily such as in the validate_scope_types! method.

Good idea. I'll make this change unless we decide on some other filter_on interface.

amayer171 · 2020-05-05T17:08:46Z

lib/sift/filter.rb

+      if type != :scope || scope_types.empty?
+        type
+      else
+        scope_types.first


Only a single scope type is allowed right? If thats the case should the argument be changed to scope_type with a default value of nil?

Another thought is that scope_type param is unrelated to other filters. Should we create a different filter class to have better argument cohesion for scopes and so that way we aren't complicating other filters with logic specific to scopes?

scope_types can have something like [:int, {foo: :datetime}] or [:int]. I can definitely see making some kind of inheritance / mixin relationship between new classes WhereFilter and ScopeFilter. I think they both support the default: option so there's some overlap. I totally agree that the frequently unused scope_params and scope_types arguments are a smell.

What do you think of this @amayer171 ?
#49

Make subclasses of Sift::Filter for Where and Scope

README.md

njbbaer · 2020-05-05T23:08:09Z

README.md

+  end
+end
+```
+Passing `?filters[with_priority]=[1,2]` will call the `with_priority` scope with the array, `[1, 2]`, instead of `"[1,2]"`


There doesn't seem to be much consensus over how arrays should be used in query parameters.
https://medium.com/raml-api/arrays-in-query-params-33189628fa68

But =1,2 appears to be a more common pattern than =[1, 2]

Sift uses the JSON standard for array parameters. My(unpopular?) opinion is that we should use either the Rails standard filters[foo][]=1&filters[foo][]=2 or POST JSON and use the X-http-method-override header to specify GET simulation.

I do think this PR is not affected by that decision, however. We could change this code to parse however we want:

https://github.com/procore/sift/blob/master/lib/sift/value_parser.rb#L24-L33

Not a fan of filters[foo][]=1&filters[foo][]=2 is that really a Rails standard? Can you explain X-http-method-override with GET simulation in more detail?

@njbbaer
By setting a header with key X-Http-Method-Override and value get in a JSON POST request (for example) you can specify that the request should be handled as a GET in servers that support this header (Rails servers).

So you can POST with a body of {"filters": {"foo": [1,2]}} with rails treating this as a GET with params of { filters: {foo: [1,2]}}. This sidesteps all issues with URL length, and allows your request to use actual JSON instead of mixing URL parameters with JSON. In practice it works really well. Here is example code from the vector_processing repo:

def large_get(path, params, timeout: 180) url = vapid_uri_base + path retries = 0 begin raw_response = RestClient::Request.execute( url: url, method: :post, headers: {'Accept' => 'application/json;charset=utf-8', 'Content-Type' => 'application/json', 'Authorization' => authorization.authorization_header, 'X-Http-Method-Override' => 'get'}, open_timeout: 3, timeout: timeout, verify_ssl: VectorProcessing::Vapid::Authorization.verify_ssl?, payload: params.to_json ) JSON.parse(raw_response.body.force_encoding('UTF-8')) rescue RestClient::Unauthorized retry if authorization.refresh_success? && (retries+=1) < 5 rescue RestClient::ExceptionWithResponse => rcewr raise ErrorResponseException.new("Error Response #{rcewr.response&.code || 'missing'} to X-Http-Method-Override 'get' on POST '#{url}'") end end

This requires no change to Vapid; it's supported by Rails out-of-the-box.

Co-authored-by: Nate Baer <[email protected]>

Sift supports specifying types of scope arguments

79ccd3b

andrew-wheeler added the needs code review label May 5, 2020

andrew-wheeler self-assigned this May 5, 2020

andrew-wheeler commented May 5, 2020

View reviewed changes

andrew-wheeler added 2 commits May 5, 2020 09:55

More README.md documentation

cd0c05c

Fix small bug

32b8368

amayer171 reviewed May 5, 2020

View reviewed changes

andrew-wheeler added 2 commits May 5, 2020 14:28

Make subclasses of Sift::Filter for Where and Scope

5bd2dd5

Merge pull request #49 from procore/aw/scope_arrays_2

f79299c

Make subclasses of Sift::Filter for Where and Scope

andrew-wheeler marked this pull request as ready for review May 5, 2020 21:55

njbbaer reviewed May 5, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

njbbaer reviewed May 5, 2020

View reviewed changes

Update README.md

4016e7a

Co-authored-by: Nate Baer <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sift supports specifying types of scope arguments #48

Sift supports specifying types of scope arguments #48

andrew-wheeler commented May 5, 2020 •

edited

Loading

andrew-wheeler May 5, 2020

andrew-wheeler May 5, 2020

andrew-wheeler May 5, 2020 •

edited

Loading

andrew-wheeler May 5, 2020

andrew-wheeler May 5, 2020 •

edited

Loading

andrew-wheeler May 5, 2020 •

edited

Loading

andrew-wheeler May 5, 2020

andrew-wheeler May 5, 2020

andrew-wheeler May 5, 2020

amayer171 left a comment

amayer171 May 5, 2020

andrew-wheeler May 5, 2020

amayer171 May 5, 2020

amayer171 May 5, 2020

andrew-wheeler May 5, 2020 •

edited

Loading

andrew-wheeler May 5, 2020

andrew-wheeler May 5, 2020

njbbaer May 5, 2020

andrew-wheeler May 6, 2020

andrew-wheeler May 6, 2020

njbbaer May 6, 2020

andrew-wheeler May 13, 2020 •

edited

Loading

		raise ArgumentError, "filter_on: Only type: :scope can have subtypes. Expecting the form `type: {scope: [type, {param: type}]}`"
		end

Sift supports specifying types of scope arguments #48

Are you sure you want to change the base?

Sift supports specifying types of scope arguments #48

Conversation

andrew-wheeler commented May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-wheeler May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-wheeler May 5, 2020 • edited Loading

Choose a reason for hiding this comment

andrew-wheeler May 5, 2020 • 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

amayer171 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

andrew-wheeler May 5, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-wheeler May 13, 2020 • edited Loading

Choose a reason for hiding this comment

andrew-wheeler commented May 5, 2020 •

edited

Loading

andrew-wheeler May 5, 2020 •

edited

Loading

andrew-wheeler May 5, 2020 •

edited

Loading

andrew-wheeler May 5, 2020 •

edited

Loading

andrew-wheeler May 5, 2020 •

edited

Loading

andrew-wheeler May 13, 2020 •

edited

Loading