cerebris / jsonapi-resources

A resource-focused Rails library for developing JSON:API compliant servers.
http://jsonapi-resources.com
MIT License
2.32k stars 532 forks source link

Filter values for related resources are wrapped in nested arrays? #1110

Open stevenharman opened 7 years ago

stevenharman commented 7 years ago

I'm a little confused as to what's going on, but upgrading from 0.8.3 to 0.9 broke some filters I have in place. e.g.:

VERIFY_RADIUS_FILTER = ->(value, context) {
  return NoLimit.new unless context[:current_location].present?

  value
}

FILTER_RADIUS_FROM_LOCATION = ->(records, value, options) {
  location = options.dig(:context, :current_location)
  limit = Array(value).first

  return records unless location.present? && limit.present?

  records.near(location, Float(limit))
}

filter :radius, default: 100.0,
                verify: VERIFY_RADIUS_FILTER,
                apply: FILTER_RADIUS_FROM_LOCATION

I think this commit my be the culprit? https://github.com/cerebris/jsonapi-resources/commit/3a691b29adb3e7aff0743ef59e44556bedb260c9

From what I can tell, this results in Resource#verify_resource being called twice. For a custom filter, at least (which is what I've got), that means the raw value gets doubly-nested in an array: https://github.com/cerebris/jsonapi-resources/blob/19f4d7b59f92b05fa28c4f925391cf5998123308/lib/jsonapi/resource.rb#L753

Instead I wonder if that line should be something like:

filter_values += raw.is_a?(String) ? CSV.parse_line(raw) : Array(raw)

To make sure we don't wrap an array?

/cc @hidde-jan

hidde-jan commented 7 years ago

From what I can tell, this results in Resource#verify_filters being called twice.

I think this is the bug. Resource#verify_filters should only be called once. Can you find out where it is called for a second time? If we make Resource#verify_filters idempotent (which it should be), without fixing the underlying issue than we just fix the symptoms but not the bug.

stevenharman commented 7 years ago

I think it's happening in the RequestProcessor. The change was originally made on master, here: https://github.com/cerebris/jsonapi-resources/pull/971/files, and it including changing the RequestProcessor#setup_get_related_resources_action so that it no longer called verify_filters b/c it was moved to the Processor#show_related_resources. But in the release-0-9 branch history, the functionality was cherry-picked over. But not all of the change made it - probably due to some extensive refactoring in the RequestProcessor. see: https://github.com/cerebris/jsonapi-resources/commit/3a691b29adb3e7aff0743ef59e44556bedb260c9

Anyhow, I think I have a handle on it now. I'm going to confirm things work as expected in master, and if so, open a PR against the release-0-9 branch to fix the back port. Sound reasonable?

hidde-jan commented 7 years ago

Sounds great 👍 the cherry pick indeed skipped over that file, but it's required for things to work properly.