From 4afe29bde4a8d9aa7eb6ff629493b1cc58c4a066 Mon Sep 17 00:00:00 2001 From: Jason Karns Date: Thu, 1 Sep 2022 23:17:44 -0400 Subject: [PATCH 1/2] Enum should allow the conventionally case-sensitive operators By default (ie, for strings), `eq` is a case-insensitive match and `eql` is case-sensitive. `string_enum` is documented as being "like string but only with case-sensitive `eq`/`not_eq` operators". These two statements feel like contradictions to me. If `string_enum` is basically like `string` but always does case-sensitive filtering, then shouldn't it support the existing case-sensitive operator? Some more rationale why I think this should be the case: polymorphic associations have a _discrete list_ of possible values for the as_type attribute. So to me it's only natural that the as_type attribute be defined as a `string_enum`. (strict case matching!) However, using the polymorphic_has_* helpers results in an relationship filter using the `eql` (case-sensitive) operator. So we have an attribute type that expects to be matched strictly; and a relationship filter that matches strictly. But the two are incompatible! (because string_enum only defines `eq` operator, and polymorphic_has_* helpers define `eql` operator; despite both ostensibly wishing to compare strictly case-sensitve.) So this PR proposes that the enum types _also_ define `eql` operators (which are already expected to be case-sensitive for strings) essentially as duplicates of the existing `eq` operators. Doing so will allow polymorphic as_type attributes to be defined as string_enum and still leverage the `eql` filter applied by the polymorphic_has_* helper. **Warning** no tests yet. This PR primarily for discussion. --- lib/graphiti/adapters/abstract.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/graphiti/adapters/abstract.rb b/lib/graphiti/adapters/abstract.rb index 92f789cf..8b3dfb96 100644 --- a/lib/graphiti/adapters/abstract.rb +++ b/lib/graphiti/adapters/abstract.rb @@ -39,7 +39,7 @@ def self.default_operators :not_match ], uuid: [:eq, :not_eq], - enum: [:eq, :not_eq], + enum: [:eq, :not_eq, :eql, :not_eql], integer_id: numerical_operators, integer: numerical_operators, big_decimal: numerical_operators, From 14618c56213fa14cb06a7769fd18411082f8bd51 Mon Sep 17 00:00:00 2001 From: Jeff Keen Date: Mon, 18 Mar 2024 10:01:59 -0500 Subject: [PATCH 2/2] Add some tests for string_enum:eql and string_enum:not_eql --- lib/graphiti/adapters/active_record.rb | 2 ++ spec/filtering_spec.rb | 27 ++++++++++++++++++++++++++ spec/fixtures/poro.rb | 13 +++++++++++++ 3 files changed, 42 insertions(+) diff --git a/lib/graphiti/adapters/active_record.rb b/lib/graphiti/adapters/active_record.rb index 3cedfae6..c290f92c 100644 --- a/lib/graphiti/adapters/active_record.rb +++ b/lib/graphiti/adapters/active_record.rb @@ -26,6 +26,7 @@ def filter_eq(scope, attribute, value) alias_method :filter_boolean_eq, :filter_eq alias_method :filter_uuid_eq, :filter_eq alias_method :filter_enum_eq, :filter_eq + alias_method :filter_enum_eql, :filter_eq def filter_not_eq(scope, attribute, value) scope.where.not(attribute => value) @@ -37,6 +38,7 @@ def filter_not_eq(scope, attribute, value) alias_method :filter_boolean_not_eq, :filter_not_eq alias_method :filter_uuid_not_eq, :filter_not_eq alias_method :filter_enum_not_eq, :filter_not_eq + alias_method :filter_enum_not_eql, :filter_not_eq def filter_string_eq(scope, attribute, value, is_not: false) column = column_for(scope, attribute) diff --git a/spec/filtering_spec.rb b/spec/filtering_spec.rb index 613aa947..d206ed22 100644 --- a/spec/filtering_spec.rb +++ b/spec/filtering_spec.rb @@ -580,6 +580,33 @@ def self.name end end + context "when filtering on an string_enum field" do + before do + resource.config[:filters] = {} + resource.filter :first_name, :string_enum, single: true, allow: ["William", "Harold"] do + eq do |scope, value| + scope[:conditions][:first_name] = value + scope + end + end + end + + it "accepts values in the allowlist with eq operator" do + params[:filter] = {first_name: {eq: "William"}} + expect(records.map(&:id)).to eq([employee3.id]) + end + + it "accepts values in the allowlist with eql operator" do + params[:filter] = {first_name: {eql: "Harold"}} + expect(records.map(&:id)).to eq([employee4.id]) + end + + it "accepts values in the allowlist with not_eql operator" do + params[:filter] = {first_name: {not_eql: "Harold"}} + expect(records.map(&:id)).to eq([employee1.id, employee2.id, employee3.id]) + end + end + context "when only allowing single values" do before do resource.filter :first_name, :string, single: true do diff --git a/spec/fixtures/poro.rb b/spec/fixtures/poro.rb index 31b0d842..f7d0d64e 100644 --- a/spec/fixtures/poro.rb +++ b/spec/fixtures/poro.rb @@ -74,6 +74,8 @@ def apply_filtering(records, params) end if value.is_a?(Array) value.include?(db_value) + elsif value.is_a?(Hash) && value[:not] + db_value != value[:not] else db_value == value end @@ -305,6 +307,13 @@ def filter(scope, name, value) scope[:conditions][name] = value scope end + + def filter_not_eq(scope, name, value) + scope[:conditions] ||= {} + scope[:conditions][name] = {not: value} + scope + end + alias_method :filter_integer_eq, :filter alias_method :filter_string_eq, :filter alias_method :filter_big_decimal_eq, :filter @@ -314,6 +323,10 @@ def filter(scope, name, value) alias_method :filter_boolean_eq, :filter alias_method :filter_hash_eq, :filter alias_method :filter_array_eq, :filter + alias_method :filter_enum_eq, :filter + alias_method :filter_enum_not_eq, :filter_not_eq + alias_method :filter_enum_eql, :filter + alias_method :filter_enum_not_eql, :filter_not_eq # No need for actual logic to fire def count(scope, attr)