Invalid filters return unfiltered results

[sienic]

Expected Behavior

When making requests with an invalid filter, I would expect to have an error raised.
This can be set on Ransack with the following configuration:

Ransack.configure do |config|
  # Raise errors if a query contains an unknown predicate or attribute.
  # Default is true (do not raise error on unknown conditions).
  config.ignore_unknown_conditions = false
end

Although this does not seem to work.

Actual Behavior

When making a request with an invalid filter, the filter is not applied at all,
payments?filter[id_eqblabla]=1,2,3,4,5 will return all payments. This is far from ideal.

This happens regardless of the configuration above. JSONAPI::Filtering configures the opposite value (not sure what value has precedence over the other):

jsonapi.rb/lib/jsonapi/patches.rb

Lines 3 to 6 in a28f6d4

	Ransack.configure do |config|
	# Raise errors if a query contains an unknown predicate or attribute.
	# Default is true (do not raise error on unknown conditions).
	config.ignore_unknown_conditions = true

I have tried to change that line and see what happens as well without success.
Not sure if this is an issue with the way JSONAPI integrates with Ransack, or something not working as expected on Ransack.

Steps

• Already described above.

Specifications

• Version: latest
• Ruby version: 2.6.6

[NeilSlater-Clixifix]

This appears to be the case because jsonapi.rb does not use the normal entry points to ransack, in order to support json-api's naming convention for params (and possibly other reasons). As a result, jsonapi.rb manipulates Ransack objects in a way that means Ransack's own validation is not called, even when configured.

I have worked around this for now by pre-validating filter and sort params, basing the logic on how JSONAPI::Filtering works. As a solution it is not ideal though, as both jsonapi.rb and my project are using Ransack methods directly in order to fit to json-api conventions - it would be cleaner if only jsonapi.rb did that.

[stas]

Hmm, I don't think jsonapi.rb scope is to filter work even work with every possible ransack predicate. The project offers a solid and good-enough out-of-the-box functionality to get you started, but it's up to you if you want to validate or disable specific filters/predicates.

@NeilSlater-Clixifix would you be kind to share a better example of what do you think jsonapi.rb should provide? Also, keep in mind that JSON:API spec does not enforce specifications on how filtering works (which is making me reconsider if this issue is relevant at all tbh 😃 )

[NeilSlater-Clixifix]

Well the issue to me is that bad filter params (that don't work with the chosen filter library) are being silently ignored, whilst a well-behaved API should respond with a 400 error. This becomes important when considering that the filter params using Ransack are constructed, so a client application should get some kind of feedback when they have been constructed incorrectly.

I think this became clear to me when writing documentation for the API (in my case using rswag) and realising I would have to document either the "if the client requests a bad filter, it will not be applied" or "if the client requests a bad filter, a 400 error will be returned" behaviour. In my case, the decision to validate and return 400 seemed better.

Given the param validation, then having two components that understand how Ransack has been adjusted to work within json-api to implement filtering and sorting is not ideal. I think it follows that something that is part of jsonapi.rb or an extension to it would be a good place to put code for this out of consistency. Perhaps one approach would be to float off the whole Ransack dependency and filter implementation into a separate lib - it's already a soft dependency, and not the only way that someone might want to provide filter, sort options (an obvious alternative would be a list of named filters managed via has_scope instead of ransack)

My validation methods are implemented in a Rails helper module that looks a bit like the code below. I have adjusted some things here as I also have error helpers and error renderer in my api controller base class that are no relevant to this conversation. The undefined Exception class ApiError stands in for that.

module ApiListParamValidators
  def api_validate_filter_and_sort(allowed_filters)
    validate_filters(params[:filter], allowed_filters)
    validate_sort(params[:sort], allowed_filters)
  end

  private

  def validate_filters(filter_params, allowed_filters)
    return if filter_params.blank?

    unless filter_params.is_a? ActionController::Parameters
      raise ApiError, 'Filter Parameter Error', 400, 'filter must be an object'
    end

    filter_params.each_key do |filter_command|
      validate_filter_syntax(filter_command, allowed_filters)
    end
  end

  def validate_filter_syntax(filter_command, allowed_filters)
    predicates = []
    to_process = filter_command.to_s.dup

    while Ransack::Predicate.detect_from_string(to_process).present?
      predicates << Ransack::Predicate.detect_and_strip_from_string!(to_process)
    end

    if predicates.blank?
      raise ApiError, 'Filter Parameter Error', 400, "filter[#{filter_command}] not supported, unrecognised predicate"
    end

    validate_filter_attribute_names(filter_command, to_process, allowed_filters)
  end

  def validate_filter_attribute_names(filter_command, command_without_predicates, allowed_filters)
    command_without_predicates.split(/_and_|_or_/).each do |attribute|
      unless allowed_filters.include?(attribute)
        raise ApiError, 'Filter Parameter Error', 400, "filter[#{filter_command}] unsupported attribute '#{attribute}'"
      end
    end
  end

  def validate_sort(sort_param, allowed_filters)
    return if sort_param.blank?

    unless sort_param.is_a?(String)
      raise ApiError, 'Sort Parameter Error', 400, 'sort must be a string of comma-separated attributes'
    end

    sort_param.split(/\s*,\s*/).each do |sort_field|
      attribute = sort_field.sub(/\A-/, '')
      unless allowed_filters.include?(attribute)
        raise ApiError, 'Sort Parameter Error', 400, "sort by unsupported attribute '#{attribute}'"
      end
    end
  end
end

I have similar business logic for validating page[] params, but implemented inside the jsonapi_page_size method that jsonapi.rb already supports, so that was a lot easier.

[stas]

Thanks @NeilSlater-Clixifix

Though I'm still unsure how jsonapi.rb is the issue here? Again, this is a library you can use to build/shape the API, the goal is not to provide you with an API out-of-the-box. Am I missing something?

Btw, consider using the built-in error renderer instead of returning arbitrary error payloads. Eg.
https://github.com/stas/jsonapi.rb/blob/master/lib/jsonapi/errors.rb#L50-L68

render jsonapi_errors: [error], status: :unprocessable_entity

[NeilSlater-Clixifix]

Thanks @NeilSlater-Clixifix

Though I'm still unsure how jsonapi.rb is the issue here? Again, this is a library you can use to build/shape the API, the goal is not to provide you with an API out-of-the-box. Am I missing something?

Btw, consider using the built-in error renderer instead of returning arbitrary error payloads. Eg. https://github.com/stas/jsonapi.rb/blob/master/lib/jsonapi/errors.rb#L50-L68

render jsonapi_errors: [error], status: :unprocessable_entity

Well the issue from a code design point of view is that jsonapi.rb wraps functionality of Ransack with a new custom behaviour (to support json-api), thus ideally should be responsible for following that through where that customisation has prevented the Ransack config from working (specifically the config for raising an error when the filters are not correctly specified). Having said that, now I have a solution for me and my employer, it's a "nice to have", not some kind of deal-breaker.

I am using the error renderers by handling exceptions in the base controller and calling render jsonapi_errors: there. So the error payloads are all json-api compliant from the example, but the rendering is not shown.

[NeilSlater-Clixifix]

If there was come kind of callbacks for filter and sort params, similar to jsonapi_page_size that provided the calculated Ransack filters associated with each input param, then maybe that could work.

Service developer could then add validation and some kind of 40x response, and jsonapi.rb could remain neutral about what kind of validation should apply to filtering and sorting params.

[stas]

I am using the error renderers by handling exceptions in the base controller and calling render jsonapi_errors: there. So the error payloads are all json-api compliant from the example, but the rendering is not shown.

Nice!

If there was come kind of callbacks for filter and sort params, similar to jsonapi_page_size that provided the calculated Ransack filters associated with each input param, then maybe that could work.

Service developer could then add validation and some kind of 40x response, and jsonapi.rb could remain neutral about what kind of validation should apply to filtering and sorting params.

Hmm, so JSONAPI::Filtering#jsonapi_filter_params() is available from with-in the controller inheriting the mixin. Afaik, one could just overwrite the implementation and use super to carry on, no?

Obviously PRs are welcome!