chrisfrank / rack-reducer Goto Github PK

Hi @chrisfrank

I just stumbled across an issue with Rails's ActionController::Parameters in filters with nested parameters. Example:

->(date:) { between(date[:from], date[:to]) }

This results in nil values for date[:from] and date[:to], because Rails is converting ActionController::Parameters to HashWithIndifferentAccess objects when calling #to_unsafe_h in https://github.com/chrisfrank/rack-reducer/blob/master/lib/rack/reducer.rb#L60. Which doesn't trigger the Hash#symbolize_keys from the refinements but the default implementation, causing the nested parameters to be converted back to strings.

pry(main)> ActionController::Parameters.new({ date: { 'from' => '2019-01-01' } }).to_unsafe_h.symbolize_keys
=> {:date=>{"from"=>"2019-01-01"}}

Not sure how to solve this yet, but I'm looking for a solution.

Hey Chris! Any tips on how to filter with optional min/max query parameters? Optional meaning users could query one, both, or none.

For example:

count_min=5 -> would return all results greater than 5
count_min=5?count_max=10 -> would return all results between 5 and 10 inclusive

Hey @chrisfrank

First of all, thanks for this amazing gem. Started using v1.0.1 at the beginning of the year and was able to clean up a lot of index actions 👍😉

Tried v1.1.0 in a new project today and noticed a slight change in how default filters are working (or not working) due to the addition in https://github.com/chrisfrank/rack-reducer/blob/master/lib/rack/reducer/reduction.rb#L22 which causes them to be skipped when params are empty. Not sure if this is intentional. I used them for defining default ordering so far.

I'd like to propose two API changes for 2.0, aimed at improving performance, simplifying the docs, and making it easier to integrate Rack::Reducer consistently across different Rack stacks.

1. Unify the functional and mixin-style APIs
2. Drop or move the middleware API

These changes could be mostly backward-compatible, but they'd be cleaner to implement as breaking changes. I'm eager for input before I make a decision.

1. Unify the functional and mixin APIs.

Rack::Reducer’s functional style is verbose, and it inefficiently encourages allocating a new array of filter functions on every request. How inefficient this is depends on your setup, but from rough benchmarks it's ~10% slower and ~30% less memory efficient than the mixin style.

The mixin style, on the other hand, is tightly coupled to Rails-ish models, and I don't like that it defines reduce on a class that in practice often returns an an Enumerable, which already has its own implementation of reduce.

I propose unifying the two APIs into one. It should be terse and efficient like the mixin style, and self-contained like the functional style. Here's what I have in mind:

# Proposed unified API for 2.0
class App < SinatraLike::Base
  class Artist < SomeORM::Model
  end

  # Instantiate a "reducer" once, on app boot
  ArtistsReducer = Rack::Reducer.new(
    Artist.all,
    ->(genre:) { where(genre: genre) },
    ->(sort:) { order(sort.to_sym) },
  )

  get '/artists' do
    # Call the reducer on each request
    @artists = ArtistsReducer.call(params)
    @artists.all.to_json
  end
end

# Current 1.0 functional style, for comparison
class FunctionalStyle < SinatraLike::Base
  class Artist < SomeORM::Model
  end

  get '/artists' do
    # this allocates a new array of filters on each request :(
    @artists = Rack::Reducer.call(params, dataset: Artist.all, filters: [
      ->(genre:) { where(genre: genre) },
      ->(sort:) { order(sort.to_sym) },
    ])
    @artists.all.to_json
  end
end

# Current 1.0 mixin style, for comparison
class MixinStyle < SinatraLike::Base
  class Artist < SomeORM::Model
    extend Rack::Reducer
    reduces self.all, filters: [
      ->(genre:) { where(genre: genre) },
      ->(sort:) { order(sort.to_sym) },
    ]
  end

  get '/artists' do
    @artists = Artist.reduce(params)
    @artists.all.to_json
  end
end

With this unified API in place, we could either leave the 1.0 APIs intact for backward compatibility, or drop them in pursuit of simplicity and enforcing fast defaults. I would prefer to drop them, but would be happy to be convinced otherwise.

2. Drop or move the Middleware API

Rack mounts middleware by calling ::new(app, options), but I want to use Rack::Reducer.new for the API outlined above.

I’m tempted to drop the middleware API entirely, because I've never needed to use it in a real app. Any I time I could have used middleware, it has been more practical to call Rack::Reducer as a function.

If you've found a useful case for Rack::Reducer as middleware, I'd be open to including it in 2.0 under a slightly different API:

# config.ru

# 1.0 (not supported in 2.0)
# use Rack::Reducer
# run MyApp

# 2.0 (proposed)
use Rack::Reducer::Middleware
run MyApp

Thanks in advance for your input.

Hello,

I am using Rack::Reducer to develop a Rails application where I want to filter books by various attributes. I have two attributes :status and :pubtypewhich are integers on the database level, but implemented as enums in my Book model. I have written both filters in exactly the same way:

# Code in Book model
Reducer = Rack::Reducer.new(
  self.all,
  # some more attribute filters here...   
  ->(status:) { status == "" ? where("status = IFNULL(@status, status) OR (status IS NULL)") : by_status(status) },
  ->(pubtype:) { pubtype == "" ? where("pubtype = IFNULL(@pubtype, pubtype) OR (pubtype IS NULL)") : by_pubtype(pubtype) },
)

With this code and a corresponding form and view, :status is correctly filtered and only books with the desired status are shown in my view. (It is also possible to combine the filter with others like :author, :genre etc.) Trying to filter by :pubtype, however, produces a view with no books.

I have been trying for several hours to get both filters to work. Cutting out all the "noise" (from the other filters etc.), it seems to boil down to this: Only the integer filter that comes first works. In the code above, only the :status filter works correctly. If I reverse the two lines, only the :pubdate filter works.

I do not experience this behavior with my string filters - I have lots of them and they all work just fine.

Tell me if you need more info and/or code snippets and I will see that I can provide them.

Looking forward to your answer!

In the README example (pasted below), ArtistReducer is not refreshed with latest Artist.all when a new artist is created. To reproduce the issue:

• visit index page with no parameters. Expect all artists to appear.
• create a new record
• revisit index page. Expect new record to not appear, when it should appear.

# app/controllers/artists_controller.rb
class ArtistsController < ApplicationController

  # Step 1: Instantiate a reducer
  ArtistReducer = Rack::Reducer.new(
    Artist.all,
    ->(name:) { where('lower(name) like ?', "%#{name.downcase}%") },
    ->(genre:) { where(genre: genre) },
  )

  # Step 2: Apply the reducer to incoming requests
  def index
    @artists = ArtistReducer.apply(params)
    render json: @artists
  end
end