Raise Ransack::InvalidSearchError instead of ArgumentError on unknown conditions

[sherifnada]

Summary

Currently, when Ransack encounters invalid search attributes (those not included in ransackable_attributes) while using Model.ransack!() it raises a generic ArgumentError. This makes it difficult to provide helpful feedback to an API consumer that their search params are incorrect.

This PR adds a custom exception type Ransack::InvalidSearchError and raises it instead of ArgumentError in cases where there are invalid attributes used in a search.

✅ Backwards compatible

The solution in this PR should be perfectly backwards compatible. The new error inherits from ArgumentError, and the test cases assert this.

Problem

Assume I have the following code in my controller

def index
  # this will raise (ArgumentError:  Invalid search term frist_name)
  results = User.ransack!(frist_name_eq: 'John').result 
  render json: to_json(result)
end

As a result this will return 5xx to the consumer. But since this is a user error, i want to return 4xx.

I could fix this by adding the following:

def index
  scope = User
  begin 
    scope = scope.ransack(frist_name_eq: 'John')
  rescue ArgumentError e
    render json: {message: e.message}, status: :bad_request
  end
  render json: to_json(scope.result)

but the problem is that I have to add this everywhere in my controllers.

I could make this cleaner by having a helper, for example defining a rescuable_ransack method to extend active record e.g:

module RescuableRansackable
  class InvalidRansackSearch < StandardError;
  
  def rescuable_ransack(params)
    ransack!(params)
  rescue ArgumentError e
    raise InvalidRansackSearch, e.message
  end
end

ActiveRecord::Base.extend(RescuableRansackable)

then using rescuable_ransack in my controllers:

# users_controller.rb
def index
  results = User.rescuable_ransack(frist_name_eq: 'John').result
  render json: to_json(result)
end

then add something to my application controller to catch all such exceptions:

class Api::V1::ApplicationController < ApplicationController
  rescue_from InvalidRansackSearch do |e|
    render json: { error: e.message }, status: :bad_request
  end
end

which would work, but this also seems pretty useful to have in ransack itself. Hence this suggestion to add it to the library.

Proposed Solution

Add a specialized exception class (e.g., Ransack::InvalidSearchError) that would be raised in cases where ignore_unknown_conditions currently raises an ArgumentError. This would then allow me to add something in my ApplicationController like the following:

class Api::V1::ApplicationController < ApplicationController
  rescue_from Ransack::InvalidSearchError do |e|
    render json: { error: e.message }, status: :bad_request
  end
end

This would be a very concise way to catch and handle such problems in controller actions.

[sherifnada]

@scarroll32 @deivid-rodriguez @gregmolnar I added this PR to resolve #1460. The tests are passing locally with rails 7.1. Could you take a look and allow CI tests to run?

[gregmolnar]

@sherifnada Thanks for this! Can you squash you commits?

[sherifnada]

@gregmolnar done.

Tests are succeeding locally but only when I run with rails 7.1:

RAILS=7-1-stable bundle exec rake
====================================================================================
Running Ransack specs with SQLite, Active Record 7.1.5.1, Arel 10.0.0 and Ruby 3.2.0
====================================================================================
..........................................................................................................................................................................................................................................................................................................................................................................*............................................

Pending: (Failures listed here are expected and do not affect your suite's status)

  1) Ransack::Search#build preserves (inverts) default scope and conditions for negative subqueries
     # spec should pass, but I do not know how/where to fix lib code
     Failure/Error: expect(s.result.to_sql).to include 'default_scope'
       expected "SELECT \"people\".* FROM \"people\" WHERE \"people\".\"id\" NOT IN (SELECT \"articles\".\"person_id\...d\" = \"people\".\"id\" AND NOT (\"articles\".\"title\" != 'Test')) ORDER BY \"people\".\"id\" DESC" to include "default_scope"
     # ./spec/ransack/search_spec.rb:183:in `block (3 levels) in <module:Ransack>'

Finished in 0.72318 seconds (files took 1.17 seconds to load)
407 examples, 0 failures, 1 pending

Coverage report generated for RSpec to /Users/sherifnada/code/ransack/coverage.
Line Coverage: 96.31% (1617 / 1679)

Rails 7.2 is failing with the error that can be seen in CI logs

[sherifnada]

is the rails 7.2 failure a known issue? it is happening on the main branch. happy to help with that one too @gregmolnar

[gregmolnar]

@sherifnada can you rebase your branch against main please?

[gregmolnar]

And can you also add a changelog entry please?

[sherifnada]

@gregmolnar done

[gregmolnar]

Thank you!

[gregmolnar]

@sherifnada Can you also add a changelog entry please?

[sherifnada]

@gregmolnar apologies didn't push it correctly, should be good now

[gregmolnar]

Thank you!