diff --git a/CHANGELOG.md b/CHANGELOG.md index 9291ec1..bd993cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ ## Unreleased +## 1.1.0 + +- Add range filtering on JSONB keys via the `{"key":"a...b"}` form (e.g. `?filters[metadata]={"price":"10...100"}`) +- Add a `keys:` option to `filter_on` for `:jsonb` filters that declares the cast type per key, accepting either a Hash (`{ price: :decimal }`) or a callable (`->(key) { ... }`) resolved per request +- Supported range key types: `:int`, `:decimal`, `:date`, `:datetime`, `:time`. A range on an undeclared or unsupported key type now returns a clear validation error instead of broken SQL +- Fix a latent bug where a `...` inside a JSONB value was mis-parsed as a whole-string range + ## 1.0.0 - Bump version to 1.0.0, making it an official release diff --git a/docs/filters.md b/docs/filters.md index 195448a..a89d7ec 100644 --- a/docs/filters.md +++ b/docs/filters.md @@ -93,6 +93,8 @@ The following types support ranges: - time - datetime +Ranges are also supported against individual keys inside a `jsonb` column when the key's type is declared. See [Filter on JSONB Column](#filter-on-jsonb-column). + ## Mutating Filters Filters can be mutated before the filter is applied using the `tap` argument. This is useful, for example, if you need to adjust the time zone of a `datetime` range filter. @@ -199,6 +201,43 @@ Will return records with next values stored in the JSONB column `metadata`: { data_1: {another: 'information'} } # When the JSONB key "data_2" is not set. ``` +### Range filtering on JSONB keys + +A range (`a...b`) can target a key inside a `jsonb` column. Because the value stored in JSONB is text, you must declare the type of each range-capable key using the `keys:` option so Sift can cast the column for an inclusive `BETWEEN` comparison. + +```ruby +filter_on :metadata, type: :jsonb, keys: { price: :decimal } +``` + +The range is sent using the same JSON object form as other JSONB filters: + +- `?filters[metadata]={"price":"10...100"}` + +This emits `(metadata->>'price')::numeric BETWEEN 10 AND 100`. + +Supported range key types and their casts: + +| Declared type | Postgres cast | +| ------------- | ------------- | +| `:int` | `::integer` | +| `:decimal` | `::numeric` | +| `:date` | `::date` | +| `:datetime` | `::timestamptz` | +| `:time` | `::time` | + +`keys:` also accepts a callable that resolves the type per request. This is useful when keys are dynamic (for example, custom-field definition IDs): + +```ruby +SIFT_RANGE_TYPES = { "decimal" => :decimal, "date" => :date, "datetime" => :datetime }.freeze + +filter_on :custom_fields, type: :jsonb, keys: ->(definition_id) { + data_type = CustomFieldDefinition.where(id: definition_id).pick(:data_type) + SIFT_RANGE_TYPES[data_type] # nil for non-range types -> exact match / array overlap unchanged +} +``` + +Keys without a declared (or with an unsupported) type keep the existing exact-match / array-overlap behavior. Sending a range against an undeclared or unsupported key type returns a clear validation error rather than producing broken SQL. + ## Filter on JSON Array `int` type filters support sending the values as an array in the URL Query parameters. For example `?filters[id]=[1,2]`. This is a way to keep payloads smaller for GET requests. When URI encoded this will become `filters%5Bid%5D=%5B1,2%5D` which is much smaller the standard format of `filters%5Bid%5D%5B%5D=1&&filters%5Bid%5D%5B%5D=2`. diff --git a/lib/procore-sift.rb b/lib/procore-sift.rb index 71031ad..4e6a5b2 100644 --- a/lib/procore-sift.rb +++ b/lib/procore-sift.rb @@ -11,6 +11,7 @@ require "sift/validators/valid_int_validator" require "sift/validators/valid_date_range_validator" require "sift/validators/valid_json_validator" +require "sift/validators/valid_jsonb_validator" module Sift extend ActiveSupport::Concern @@ -66,8 +67,8 @@ def sort_fields end class_methods do - def filter_on(parameter, type:, internal_name: parameter, default: nil, validate: nil, scope_params: [], tap: nil) - filters << Filter.new(parameter, type, internal_name, default, validate, scope_params, tap) + def filter_on(parameter, type:, internal_name: parameter, default: nil, validate: nil, scope_params: [], tap: nil, keys: nil) + filters << Filter.new(parameter, type, internal_name, default, validate, scope_params, tap, keys) end def filters diff --git a/lib/sift/filter.rb b/lib/sift/filter.rb index 577c7ca..eab2316 100644 --- a/lib/sift/filter.rb +++ b/lib/sift/filter.rb @@ -4,12 +4,13 @@ module Sift class Filter attr_reader :parameter, :default, :custom_validate, :scope_params - def initialize(param, type, internal_name, default, custom_validate = nil, scope_params = [], tap = ->(value, _params) { value }) - @parameter = Parameter.new(param, type, internal_name) + def initialize(param, type, internal_name, default, custom_validate = nil, scope_params = [], tap = ->(value, _params) { value }, keys = nil) + @parameter = Parameter.new(param, type, internal_name, keys) @default = default @custom_validate = custom_validate @scope_params = scope_params @tap = tap + @keys = keys raise ArgumentError, "scope_params must be an array of symbols" unless valid_scope_params?(scope_params) raise "unknown filter type: #{type}" unless type_validator.valid_type? end @@ -41,7 +42,7 @@ def validation_field end def type_validator - @type_validator ||= Sift::TypeValidator.new(param, type) + @type_validator ||= Sift::TypeValidator.new(param, type, @keys) end def type diff --git a/lib/sift/parameter.rb b/lib/sift/parameter.rb index 358f898..720463d 100644 --- a/lib/sift/parameter.rb +++ b/lib/sift/parameter.rb @@ -1,12 +1,30 @@ module Sift # Value Object that wraps some handling of filter params class Parameter - attr_reader :param, :type, :internal_name + attr_reader :param, :type, :internal_name, :keys - def initialize(param, type, internal_name = param) + def initialize(param, type, internal_name = param, keys = nil) @param = param @type = type @internal_name = internal_name + @keys = keys + end + + # Resolves the declared cast type for a JSONB key, supporting both a Hash + # (`{ price: :decimal }`) and a callable (`->(key) { ... }`) that is + # evaluated per request. Returns nil when no type is declared for the key. + def key_type(key) + return nil if keys.nil? + + if keys.respond_to?(:call) + keys.call(key) + else + keys[key.to_sym] || keys[key.to_s] + end + end + + def range_key?(key) + !key_type(key).nil? end def parse_options diff --git a/lib/sift/type_validator.rb b/lib/sift/type_validator.rb index 5b171fb..7c3f939 100644 --- a/lib/sift/type_validator.rb +++ b/lib/sift/type_validator.rb @@ -4,7 +4,6 @@ class TypeValidator DATETIME_RANGE_PATTERN = { format: { with: /\A.+(?:[^.]\.\.\.[^.]).+\z/, message: "must be a range" }, valid_date_range: true }.freeze DECIMAL_PATTERN = { numericality: true, allow_nil: true }.freeze BOOLEAN_PATTERN = { inclusion: { in: [true, false] }, allow_nil: true }.freeze - JSON_PATTERN = { valid_json: true }.freeze WHITELIST_TYPES = [:int, :decimal, @@ -17,12 +16,13 @@ class TypeValidator :scope, :jsonb].freeze - def initialize(param, type) + def initialize(param, type, keys = nil) @param = param @type = type + @keys = keys end - attr_reader :param, :type + attr_reader :param, :type, :keys def validate case type @@ -35,7 +35,7 @@ def validate when :boolean BOOLEAN_PATTERN when :jsonb - JSON_PATTERN + { valid_json: true, valid_jsonb: { keys: keys } } end end diff --git a/lib/sift/validators/valid_jsonb_validator.rb b/lib/sift/validators/valid_jsonb_validator.rb new file mode 100644 index 0000000..3e2a6b7 --- /dev/null +++ b/lib/sift/validators/valid_jsonb_validator.rb @@ -0,0 +1,75 @@ +# Validates JSONB filter values that use the per-key range form +# (`{"price":"10...100"}`). JSON validity itself is handled by ValidJsonValidator; +# this validator only inspects keys whose string value contains "..." and +# ensures that the key has a declared, range-capable type and that both bounds +# cast to that type. This prevents broken SQL from reaching the database. +class ValidJsonbValidator < ActiveModel::EachValidator + SUPPORTED_RANGE_TYPES = [:int, :decimal, :date, :datetime, :time].freeze + + def validate_each(record, attribute, value) + parsed = parse_json(value) + return unless parsed.is_a?(Hash) + + parsed.each do |key, key_value| + next unless key_value.is_a?(String) && key_value.include?("...") + + key_type = resolve_key_type(key) + + if key_type.nil? || !SUPPORTED_RANGE_TYPES.include?(key_type) + record.errors.add(attribute, "range filtering on key '#{key}' requires a declared key type") + next + end + + validate_bounds(record, attribute, key, key_value, key_type) + end + end + + private + + def parse_json(value) + value = value.strip if value.is_a?(String) + JSON.parse(value) + rescue JSON::ParserError, TypeError + nil + end + + def resolve_key_type(key) + declared = options[:keys] + return nil if declared.nil? + + if declared.respond_to?(:call) + declared.call(key) + else + declared[key.to_sym] || declared[key.to_s] + end + end + + def validate_bounds(record, attribute, key, key_value, key_type) + bounds = key_value.split("...") + + bounds.each do |bound| + next if valid_bound?(bound, key_type) + + record.errors.add(attribute, "range bound '#{bound}' for key '#{key}' is not a valid #{key_type}") + end + end + + def valid_bound?(bound, key_type) + value = bound.to_s.strip + + case key_type + when :int + !!(/\A-?\d+\z/ =~ value) + when :decimal + !!Float(value) + when :date, :datetime + !!DateTime.parse(value) + when :time + !!Time.parse(value) + else + false + end + rescue ArgumentError, TypeError + false + end +end diff --git a/lib/sift/value_parser.rb b/lib/sift/value_parser.rb index 77ce831..cf829c6 100644 --- a/lib/sift/value_parser.rb +++ b/lib/sift/value_parser.rb @@ -32,10 +32,18 @@ def parse_json_and_values parsed_jsonb = parse_json(value) return parsed_jsonb if parsed_jsonb.is_a?(Array) || parsed_jsonb.is_a?(String) - parsed_jsonb.each_with_object({}) do |key_value, hash| - key = key_value.first - value = key_value.last - hash[key] = value.is_a?(String) ? parse_json(value) : value + parsed_jsonb.each_with_object({}) do |(key, key_value), hash| + hash[key] = parse_jsonb_value(key_value) + end + end + + def parse_jsonb_value(key_value) + return key_value unless key_value.is_a?(String) + + if supports_ranges && key_value.include?("...") + Range.new(*key_value.split("...")) + else + parse_json(key_value) end end @@ -53,7 +61,9 @@ def array_from_json attr_reader :value, :type, :supports_boolean, :supports_json, :supports_json_object, :supports_ranges def parse_as_range?(raw_value=value) - supports_ranges && raw_value.to_s.include?("...") + # Guard against jsonb objects: a `...` inside a jsonb value is a per-key + # range handled in parse_json_and_values, not a whole-string range. + supports_ranges && !supports_json_object && raw_value.to_s.include?("...") end def range_value diff --git a/lib/sift/version.rb b/lib/sift/version.rb index 5eceb55..af73d8d 100644 --- a/lib/sift/version.rb +++ b/lib/sift/version.rb @@ -1,3 +1,3 @@ module Sift - VERSION = "1.0.1".freeze + VERSION = "1.1.0".freeze end diff --git a/lib/sift/where_handler.rb b/lib/sift/where_handler.rb index 5b27cfd..f9f2a29 100644 --- a/lib/sift/where_handler.rb +++ b/lib/sift/where_handler.rb @@ -1,5 +1,13 @@ module Sift class WhereHandler + JSONB_RANGE_CASTS = { + int: "::integer", + decimal: "::numeric", + date: "::date", + datetime: "::timestamptz", + time: "::time" + }.freeze + def initialize(param) @param = param end @@ -18,19 +26,41 @@ def apply_jsonb_conditions(collection, value) return collection.where("#{@param.internal_name} @> ?", value.to_s) if value.is_a?(Array) value.each do |key, val| - collection = if val.is_a?(Array) - elements = Hash[val.each_with_index.map { |item, i| ["value_#{i}".to_sym, item.to_s] } ] - elements[:all_values] = val.compact.map(&:to_s) - main_condition = "('{' || TRANSLATE(#{@param.internal_name}->>'#{key}', '[]','') || '}')::text[] && ARRAY[:all_values]" - sub_conditions = val.each_with_index.map do |element, i| - "#{@param.internal_name}->>'#{key}' #{element === nil ? 'IS NULL' : "= :value_#{i}"}" - end.join(' OR ') - collection.where("(#{main_condition}) OR (#{sub_conditions})", elements) + collection = if val.is_a?(Range) + apply_jsonb_range(collection, key, val) + elsif val.is_a?(Array) + apply_jsonb_array(collection, key, val) else collection.where("#{@param.internal_name}->>'#{key}' = ?", val.to_s) end end collection end + + def apply_jsonb_array(collection, key, val) + elements = Hash[val.each_with_index.map { |item, i| ["value_#{i}".to_sym, item.to_s] }] + elements[:all_values] = val.compact.map(&:to_s) + main_condition = "('{' || TRANSLATE(#{@param.internal_name}->>'#{key}', '[]','') || '}')::text[] && ARRAY[:all_values]" + sub_conditions = val.each_with_index.map do |element, i| + "#{@param.internal_name}->>'#{key}' #{element.nil? ? 'IS NULL' : "= :value_#{i}"}" + end.join(" OR ") + collection.where("(#{main_condition}) OR (#{sub_conditions})", elements) + end + + def apply_jsonb_range(collection, key, range) + key_type = @param.key_type(key) + cast = JSONB_RANGE_CASTS.fetch(key_type) do + raise ArgumentError, "range filtering on JSONB key '#{key}' requires a declared key type" + end + + condition = "(#{@param.internal_name}->>'#{quote_jsonb_key(key)}')#{cast} BETWEEN ? AND ?" + collection.where(condition, range.begin, range.end) + end + + # Escapes a JSONB key for safe interpolation inside a single-quoted SQL + # string literal. Bounds are always passed as bind parameters. + def quote_jsonb_key(key) + key.to_s.gsub("'", "''") + end end end diff --git a/test/controller_test.rb b/test/controller_test.rb index ec3137f..02f25fd 100644 --- a/test/controller_test.rb +++ b/test/controller_test.rb @@ -306,6 +306,37 @@ class PostsControllerTest < ActionDispatch::IntegrationTest assert_mock instance_mock end + test "it filters on metadata by jsonb key range with a declared key type" do + post = Post.create!(metadata: { 'price' => 50 }.to_json) + Post.create!(metadata: { 'price' => 500 }.to_json) + + # Stubs are needed because the dummy app DB is not PostgreSQL + instance_mock = Minitest::Mock.new + instance_mock.expect :call, Post.where(id: post.id), [Post.all, Hash, ActionController::Parameters, Array] + + class_mock = Minitest::Mock.new + class_mock.expect :call, instance_mock, [Sift::Parameter] + + Sift::WhereHandler.stub :new, class_mock, [Sift::Parameter] do + get("/posts", params: { filters: { metadata_ranges: { 'price' => '10...100' }.to_json } }) + + json = JSON.parse(@response.body) + assert_equal 1, json.size + assert_equal post.id, json.first["id"] + end + + assert_mock class_mock + assert_mock instance_mock + end + + test "it rejects a jsonb key range on an undeclared key type" do + get("/posts", params: { filters: { metadata: { 'price' => '10...100' }.to_json } }) + + assert_equal 400, @response.status + json = JSON.parse(@response.body) + assert_includes json["errors"]["metadata"], "range filtering on key 'price' requires a declared key type" + end + test "it filters on metadata by jsonb array" do post = Post.create!(metadata: [1,2,3].to_json) Post.create!(metadata: [4,5,6].to_json) diff --git a/test/dummy/app/controllers/posts_controller.rb b/test/dummy/app/controllers/posts_controller.rb index 0eb847d..48fe8cc 100644 --- a/test/dummy/app/controllers/posts_controller.rb +++ b/test/dummy/app/controllers/posts_controller.rb @@ -15,6 +15,7 @@ class PostsController < ApplicationController filter_on :expired_before, type: :scope filter_on :expired_before_and_priority, type: :scope, scope_params: [:priority] filter_on :metadata, type: :jsonb + filter_on :metadata_ranges, type: :jsonb, internal_name: :metadata, keys: { price: :decimal } filter_on :french_bread, type: :string, internal_name: :title filter_on :body2, type: :scope, internal_name: :body2, default: ->(c) { c.order(:body) } diff --git a/test/filter_validator_test.rb b/test/filter_validator_test.rb index 529c047..ae5a4b1 100644 --- a/test/filter_validator_test.rb +++ b/test/filter_validator_test.rb @@ -263,4 +263,59 @@ class FilterValidatorTest < ActiveSupport::TestCase assert !validator.valid? assert_equal expected_messages, validator.errors.messages end + + test "it allows a jsonb range on a declared key type" do + filter = Sift::Filter.new(:metadata, :jsonb, :metadata, nil, nil, [], nil, { price: :decimal }) + + validator = Sift::FilterValidator.build( + filters: [filter], + sort_fields: [], + filter_params: { metadata: "{\"price\":\"10...100\"}" }, + sort_params: [], + ) + assert validator.valid? + assert_equal Hash.new, validator.errors.messages + end + + test "it rejects a jsonb range on an undeclared key" do + filter = Sift::Filter.new(:metadata, :jsonb, :metadata, nil) + expected_messages = { metadata: ["range filtering on key 'price' requires a declared key type"] } + + validator = Sift::FilterValidator.build( + filters: [filter], + sort_fields: [], + filter_params: { metadata: "{\"price\":\"10...100\"}" }, + sort_params: [], + ) + assert !validator.valid? + assert_equal expected_messages, validator.errors.messages + end + + test "it rejects a jsonb range whose bounds do not cast to the declared type" do + filter = Sift::Filter.new(:metadata, :jsonb, :metadata, nil, nil, [], nil, { price: :decimal }) + expected_messages = { metadata: ["range bound 'cheap' for key 'price' is not a valid decimal"] } + + validator = Sift::FilterValidator.build( + filters: [filter], + sort_fields: [], + filter_params: { metadata: "{\"price\":\"cheap...100\"}" }, + sort_params: [], + ) + assert !validator.valid? + assert_equal expected_messages, validator.errors.messages + end + + test "it allows a jsonb range using a callable key resolver" do + keys = ->(key) { key == "released_on" ? :date : nil } + filter = Sift::Filter.new(:metadata, :jsonb, :metadata, nil, nil, [], nil, keys) + + validator = Sift::FilterValidator.build( + filters: [filter], + sort_fields: [], + filter_params: { metadata: "{\"released_on\":\"2020-01-01...2020-12-31\"}" }, + sort_params: [], + ) + assert validator.valid? + assert_equal Hash.new, validator.errors.messages + end end diff --git a/test/parameter_test.rb b/test/parameter_test.rb new file mode 100644 index 0000000..68f9b06 --- /dev/null +++ b/test/parameter_test.rb @@ -0,0 +1,32 @@ +require "test_helper" + +class ParameterTest < ActiveSupport::TestCase + test "key_type resolves from a Hash with symbol keys" do + parameter = Sift::Parameter.new(:metadata, :jsonb, :metadata, { price: :decimal }) + + assert_equal :decimal, parameter.key_type("price") + assert_equal :decimal, parameter.key_type(:price) + end + + test "key_type resolves from a callable" do + parameter = Sift::Parameter.new(:custom_fields, :jsonb, :custom_fields, ->(key) { + key == "123" ? :date : nil + }) + + assert_equal :date, parameter.key_type("123") + assert_nil parameter.key_type("456") + end + + test "key_type is nil when no keys are declared" do + parameter = Sift::Parameter.new(:metadata, :jsonb) + + assert_nil parameter.key_type("price") + end + + test "range_key? reflects whether a key has a declared type" do + parameter = Sift::Parameter.new(:metadata, :jsonb, :metadata, { price: :decimal }) + + assert parameter.range_key?("price") + refute parameter.range_key?("color") + end +end diff --git a/test/type_validator_test.rb b/test/type_validator_test.rb index 4bb8abe..5c256bd 100644 --- a/test/type_validator_test.rb +++ b/test/type_validator_test.rb @@ -43,14 +43,22 @@ class TypeValidatorTest < ActiveSupport::TestCase test "it accepts a json array for type jsonb" do validator = Sift::TypeValidator.new("[1,10]", :jsonb) - expected_validation = { valid_json: true } + expected_validation = { valid_json: true, valid_jsonb: { keys: nil } } assert_equal expected_validation, validator.validate end test "it accepts a json object for type jsonb" do validator = Sift::TypeValidator.new("{\"a\":4}", :jsonb) - expected_validation = { valid_json: true } + expected_validation = { valid_json: true, valid_jsonb: { keys: nil } } + + assert_equal expected_validation, validator.validate + end + + test "it threads declared keys into the jsonb validation" do + keys = { price: :decimal } + validator = Sift::TypeValidator.new("{\"price\":\"10...100\"}", :jsonb, keys) + expected_validation = { valid_json: true, valid_jsonb: { keys: keys } } assert_equal expected_validation, validator.validate end diff --git a/test/value_parser_test.rb b/test/value_parser_test.rb index be209ed..df6eff4 100644 --- a/test/value_parser_test.rb +++ b/test/value_parser_test.rb @@ -141,6 +141,45 @@ class FilterTest < ActiveSupport::TestCase end end + test "jsonb per-key string range value is parsed into a Range" do + options = { + supports_ranges: true, + supports_json: true, + supports_json_object: true + } + parser = Sift::ValueParser.new(value: "{\"price\":\"10...100\"}", options: options) + + result = parser.parse + assert_instance_of Range, result["price"] + assert_equal "10", result["price"].begin + assert_equal "100", result["price"].end + end + + test "jsonb non-range keys are unchanged when a sibling key is a range" do + options = { + supports_ranges: true, + supports_json: true, + supports_json_object: true + } + parser = Sift::ValueParser.new(value: "{\"price\":\"10...100\",\"color\":\"red\"}", options: options) + + result = parser.parse + assert_instance_of Range, result["price"] + assert_equal "red", result["color"] + end + + test "jsonb whole-string range is not treated as a Range (bug fix)" do + options = { + supports_ranges: true, + supports_json: true, + supports_json_object: true + } + parser = Sift::ValueParser.new(value: "10...100", options: options) + + refute_instance_of Range, parser.parse + assert_equal "10...100", parser.parse + end + test "parses range for Date string range and normalizes DateTime values" do options = { supports_ranges: true diff --git a/test/where_handler_test.rb b/test/where_handler_test.rb index 0495fc8..97ff76d 100644 --- a/test/where_handler_test.rb +++ b/test/where_handler_test.rb @@ -12,4 +12,46 @@ class WhereHandlerTest < ActiveSupport::TestCase assert_same filtered_collection, result assert_mock collection end + + test "it filters jsonb key ranges with a type-driven cast and bound params" do + param = Sift::Parameter.new(:metadata, :jsonb, :metadata, { price: :decimal }) + collection = Minitest::Mock.new + filtered_collection = Object.new + collection.expect( + :where, + filtered_collection, + ["(metadata->>'price')::numeric BETWEEN ? AND ?", "10", "100"] + ) + + result = Sift::WhereHandler.new(param).call(collection, { "price" => Range.new("10", "100") }, {}, []) + + assert_same filtered_collection, result + assert_mock collection + end + + test "it maps each declared key type to the correct Postgres cast" do + casts = { + int: "::integer", + decimal: "::numeric", + date: "::date", + datetime: "::timestamptz", + time: "::time" + } + + casts.each do |key_type, cast| + param = Sift::Parameter.new(:metadata, :jsonb, :metadata, { value: key_type }) + collection = Minitest::Mock.new + filtered_collection = Object.new + collection.expect( + :where, + filtered_collection, + ["(metadata->>'value')#{cast} BETWEEN ? AND ?", "a", "b"] + ) + + result = Sift::WhereHandler.new(param).call(collection, { "value" => Range.new("a", "b") }, {}, []) + + assert_same filtered_collection, result + assert_mock collection + end + end end