From 1c6a980cae81efe0cf5cc4329135ca11f9b7aa50 Mon Sep 17 00:00:00 2001 From: Alex Le Date: Fri, 30 Oct 2026 15:22:01 +0000 Subject: [PATCH 1/3] fix(ruby): align FT search builder options with other GLIDE clients - CreateOptions: `stopwords: []` sends `STOPWORDS 0` instead of being dropped, matching Python, Java and Node (#386). - AggregateOptions: `clauses` defaults to nil, and nil means no clauses instead of raising NoMethodError (#387). - SearchOptions: `sort_order` without `sort_by` raises ArgumentError instead of being silently ignored, matching all other clients (#388). Signed-off-by: Alex Le --- lib/valkey/search/aggregate.rb | 4 +- lib/valkey/search/create_options.rb | 2 +- lib/valkey/search/query.rb | 10 +++-- test/unit/commands/search_aggregate_test.rb | 4 ++ test/unit/commands/search_builder_test.rb | 19 +++++++++ test/unit/commands/search_query_test.rb | 46 ++++++++++++++++----- 6 files changed, 68 insertions(+), 17 deletions(-) diff --git a/lib/valkey/search/aggregate.rb b/lib/valkey/search/aggregate.rb index 204e13f6..8ecacdc0 100644 --- a/lib/valkey/search/aggregate.rb +++ b/lib/valkey/search/aggregate.rb @@ -226,9 +226,9 @@ class AggregateOptions # @param slop [Integer, nil] emit `SLOP ` # @param dialect [Integer, nil] emit `DIALECT ` (only 2 is valid) # @param timeout [Integer, nil] emit `TIMEOUT ` - def initialize(clauses: [], load: nil, params: nil, verbatim: false, + def initialize(clauses: nil, load: nil, params: nil, verbatim: false, in_order: false, slop: nil, dialect: nil, timeout: nil) - @clauses = clauses + @clauses = clauses || [] unless @clauses.all?(AggregateClause) raise ArgumentError, "aggregate clauses must be Valkey::Search::AggregateClause objects" end diff --git a/lib/valkey/search/create_options.rb b/lib/valkey/search/create_options.rb index 3fd35d86..ff080d09 100644 --- a/lib/valkey/search/create_options.rb +++ b/lib/valkey/search/create_options.rb @@ -42,7 +42,7 @@ def to_args args.push("LANGUAGE", @language) unless @language.nil? args << "SKIPINITIALSCAN" if @skip_initial_scan args.push("MINSTEMSIZE", @min_stem_size) unless @min_stem_size.nil? - args.push("STOPWORDS", @stopwords.length, *@stopwords) if @stopwords && !@stopwords.empty? + args.push("STOPWORDS", @stopwords.length, *@stopwords) unless @stopwords.nil? args << "NOSTOPWORDS" if @no_stopwords args end diff --git a/lib/valkey/search/query.rb b/lib/valkey/search/query.rb index ab4b6bb2..aab89065 100644 --- a/lib/valkey/search/query.rb +++ b/lib/valkey/search/query.rb @@ -20,7 +20,7 @@ class SearchOptions # @param return_fields [Array, nil] field name String/Symbol or { name:, as: } Hash # @param params [Hash, nil] query parameters (PARAMS) # @param sort_by [String, nil] field to sort by - # @param sort_order [Symbol] :asc (default) or :desc + # @param sort_order [Symbol, nil] :asc or :desc (requires sort_by) # @param with_sort_keys [Boolean] emit WITHSORTKEYS (requires sort_by; cannot # be combined with no_content — the server discards the sort keys when # NOCONTENT is set, which desynchronizes the reply parser) @@ -28,10 +28,11 @@ class SearchOptions # @param shard_scope [Symbol, nil] :all_shards or :some_shards # @param consistency [Symbol, nil] :consistent or :inconsistent def initialize(limit: nil, return_fields: nil, params: nil, sort_by: nil, - sort_order: :asc, with_sort_keys: false, no_content: false, + sort_order: nil, with_sort_keys: false, no_content: false, verbatim: false, in_order: false, slop: nil, dialect: nil, timeout: nil, shard_scope: nil, consistency: nil) raise ArgumentError, "with_sort_keys requires sort_by" if with_sort_keys && sort_by.nil? + raise ArgumentError, "sort_order requires sort_by" if !sort_order.nil? && sort_by.nil? # The server honors NOCONTENT and silently drops the sort keys, but the # reply is still parsed as (key, sort_key) pairs — so the combination # yields mispaired documents or a parse error depending on how many @@ -46,7 +47,7 @@ def initialize(limit: nil, return_fields: nil, params: nil, sort_by: nil, @return_tokens = expand_return_fields(return_fields) @params = params @sort_by = sort_by - @sort_order = Search.lookup_token(Search::SORT_ORDERS, sort_order, "sort order") + @sort_order = sort_order && Search.lookup_token(Search::SORT_ORDERS, sort_order, "sort order") @with_sort_keys = with_sort_keys @no_content = no_content @verbatim = verbatim @@ -102,7 +103,8 @@ def expand_return_fields(return_fields) def append_sort_by(args) return if @sort_by.nil? - args.push("SORTBY", @sort_by, @sort_order) + args.push("SORTBY", @sort_by) + args << @sort_order unless @sort_order.nil? end def normalize_limit(limit) diff --git a/test/unit/commands/search_aggregate_test.rb b/test/unit/commands/search_aggregate_test.rb index f0e77f9c..ecb4514e 100644 --- a/test/unit/commands/search_aggregate_test.rb +++ b/test/unit/commands/search_aggregate_test.rb @@ -160,6 +160,10 @@ def test_aggregate_options_clause_order_preserved opts.to_args end + def test_aggregate_options_nil_clauses + assert_equal [], S::AggregateOptions.new(clauses: nil).to_args + end + def test_aggregate_options_top_level_before_clauses opts = S::AggregateOptions.new(verbatim: true, dialect: 2, clauses: [S::Filter.new("@x > 1")]) diff --git a/test/unit/commands/search_builder_test.rb b/test/unit/commands/search_builder_test.rb index 6b4dde2f..45035d26 100644 --- a/test/unit/commands/search_builder_test.rb +++ b/test/unit/commands/search_builder_test.rb @@ -128,6 +128,12 @@ def test_create_options_stopwords assert_equal ["NOSTOPWORDS"], Valkey::Search::CreateOptions.new(no_stopwords: true).to_args end + # An empty list turns stopword removal off (`STOPWORDS 0`) rather than being dropped. + def test_create_options_stopwords_empty + stopword_count = 0 + assert_equal ["STOPWORDS", stopword_count], Valkey::Search::CreateOptions.new(stopwords: []).to_args + end + def test_create_options_min_stem_size assert_equal ["MINSTEMSIZE", 4], Valkey::Search::CreateOptions.new(min_stem_size: 4).to_args end @@ -138,6 +144,12 @@ def test_create_options_rejects_conflicting_stopwords end end + def test_create_options_rejects_empty_stopwords_with_no_stopwords + assert_raises(ArgumentError) do + Valkey::Search::CreateOptions.new(stopwords: [], no_stopwords: true) + end + end + def test_create_options_rejects_unknown_data_type assert_raises(ArgumentError) { Valkey::Search::CreateOptions.new(on: :bogus) } end @@ -169,6 +181,13 @@ def test_ft_create_builder_without_options assert_equal %w[idx SCHEMA title TEXT], client.captured_args end + def test_ft_create_builder_with_empty_stopwords + client = FakeClient.new + stopword_count = 0 + client.ft_create("idx", [Valkey::Search::TextField.new("title")], stopwords: []) + assert_equal ["idx", "STOPWORDS", stopword_count, "SCHEMA", "title", "TEXT"], client.captured_args + end + def test_ft_create_builder_with_vector_field client = FakeClient.new client.ft_create("vec", [Valkey::Search::VectorField.hnsw("embedding", dim: 128, metric: :cosine)], diff --git a/test/unit/commands/search_query_test.rb b/test/unit/commands/search_query_test.rb index 18d5c6c6..d21dd73a 100644 --- a/test/unit/commands/search_query_test.rb +++ b/test/unit/commands/search_query_test.rb @@ -56,30 +56,54 @@ def test_return_fields_plain_and_aliased assert_equal ["RETURN", 4, "title", "loc", "AS", "location"], opts.to_args end - def test_sort_by_default_asc_and_desc - assert_equal %w[SORTBY price ASC], Valkey::Search::SearchOptions.new(sort_by: "price").to_args + def test_return_fields_empty + assert_equal [], Valkey::Search::SearchOptions.new(return_fields: []).to_args + end + + def test_sort_by_without_order_and_desc + assert_equal %w[SORTBY price], Valkey::Search::SearchOptions.new(sort_by: "price").to_args assert_equal %w[SORTBY price DESC], Valkey::Search::SearchOptions.new(sort_by: "price", sort_order: :desc).to_args end + def test_sort_order_asc + assert_equal %w[SORTBY p ASC], Valkey::Search::SearchOptions.new(sort_by: "p", sort_order: :asc).to_args + end + + def test_sort_order_nil + assert_equal [], Valkey::Search::SearchOptions.new(sort_order: nil).to_args + end + + # Any explicit order without a field is rejected, including an explicit :asc. + def test_sort_order_requires_sort_by + %i[desc asc].each do |sort_order| + err = assert_raises(ArgumentError) { Valkey::Search::SearchOptions.new(sort_order: sort_order) } + assert_match(/sort_order requires sort_by/, err.message) + end + end + def test_params_nargs_is_twice_pairs opts = Valkey::Search::SearchOptions.new(params: { vec: "BLOB", k: "5" }) assert_equal ["PARAMS", 4, "vec", "BLOB", "k", "5"], opts.to_args end + def test_params_empty + assert_equal [], Valkey::Search::SearchOptions.new(params: {}).to_args + end + def test_flags_and_scalars opts = Valkey::Search::SearchOptions.new( no_content: true, verbatim: true, in_order: true, slop: 2, sort_by: "price", dialect: 2, timeout: 500 ) assert_equal ["NOCONTENT", "VERBATIM", "INORDER", "SLOP", 2, - "SORTBY", "price", "ASC", "DIALECT", 2, "TIMEOUT", 500], + "SORTBY", "price", "DIALECT", 2, "TIMEOUT", 500], opts.to_args end def test_with_sort_keys_emission opts = Valkey::Search::SearchOptions.new(sort_by: "price", with_sort_keys: true) - assert_equal %w[SORTBY price ASC WITHSORTKEYS], opts.to_args + assert_equal %w[SORTBY price WITHSORTKEYS], opts.to_args end def test_shard_scope_and_consistency @@ -166,7 +190,7 @@ def test_ft_search_builder_with_options_object_returns_search_result result = client.ft_search("idx", "@title:hello", opts) assert_equal Valkey::RequestType::FT_SEARCH, client.captured_type - assert_equal ["idx", "@title:hello", "LIMIT", 0, 10, "SORTBY", "price", "ASC"], client.captured_args + assert_equal ["idx", "@title:hello", "LIMIT", 0, 10, "SORTBY", "price"], client.captured_args assert_instance_of Valkey::Search::SearchResult, result assert_equal 1, result.total_results assert_equal "doc:1", result.documents[0].key @@ -205,6 +229,13 @@ def test_ft_search_rejects_wrong_typed_options assert_raises(ArgumentError) { client.ft_search("idx", "q", "EXTRA") } end + def test_ft_search_sort_order_requires_sort_by + client = FakeClient.new + err = assert_raises(ArgumentError) { client.ft_search("idx", "*", sort_order: :desc) } + assert_match(/sort_order requires sort_by/, err.message) + assert_nil client.captured_args + end + # ---- Defensive option mitigations ---- # DIALECT is pinned to v2 on Valkey-native search. @@ -266,11 +297,6 @@ def test_from_raw_rejects_non_integer_count assert_raises(TypeError) { Valkey::Search::SearchResult.from_raw(["not-a-number", "doc:1", []]) } end - # STOPWORDS []: empty array must not emit "STOPWORDS 0". - def test_create_options_empty_stopwords_emits_nothing - assert_equal [], Valkey::Search::CreateOptions.new(stopwords: []).to_args - end - # the shared lookup_token helper backs the enum validations. def test_lookup_token_helper assert_equal "HASH", Valkey::Search.lookup_token({ hash: "HASH" }, :hash, "data type") From 3e1fc1f0d43aff7ed5eb9e4b5e30b94beedd8bce Mon Sep 17 00:00:00 2001 From: Alex Le Date: Fri, 30 Oct 2026 15:44:55 +0000 Subject: [PATCH 2/3] docs(ruby): drop redis-rb conventions rule from AGENTS.md Signed-off-by: Alex Le --- AGENTS.md | 1 - 1 file changed, 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 82850ffe..7c76f1bd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -272,7 +272,6 @@ cargo fmt --manifest-path ./Cargo.toml --all - **FFI dependency:** `ffi ~> 1.17.0`; do not break ABI without rebuilding native lib - **Blocking FFI bindings MUST pass `blocking: true`:** any `attach_function` in `lib/valkey/bindings.rb` that can wait — connect, command, batch, script, close. Ruby-FFI defaults to `blocking: false`, holding the GVL for the whole call: that stalls every other Ruby thread, and deadlocks if the call waits on something needing the GVL (a Pub/Sub callback). `create_client`/`create_client_from_uri` still need it. The `free_*`/`drop_*` and OTel span helpers don't — they never wait. - **Synchronous only:** No async client in this repo; do not add EventMachine/async patterns without design review -- **redis-rb conventions:** Prefer matching redis-rb method signatures and return types when implementing commands for familiarity. - **Command args:** All FFI args are strings; convert types in Ruby before `send_command` - **Pipeline transactions:** `MULTI`/`EXEC`/`DISCARD` in `pipelined` use sequential fallback; do not remove without fixing FFI batch stability - **OpenTelemetry:** Init once per process via `Valkey::OpenTelemetry.init`; spans created in FFI layer From 0e1291fbb01238cf2183da20186dfccdb7007d26 Mon Sep 17 00:00:00 2001 From: Alex Le <35547559+Aryex@users.noreply.github.com> Date: Fri, 30 Oct 2026 08:57:22 -0700 Subject: [PATCH 3/3] Removed comment Signed-off-by: Alex Le <35547559+Aryex@users.noreply.github.com> --- test/unit/commands/search_builder_test.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/commands/search_builder_test.rb b/test/unit/commands/search_builder_test.rb index 45035d26..aaac5837 100644 --- a/test/unit/commands/search_builder_test.rb +++ b/test/unit/commands/search_builder_test.rb @@ -128,7 +128,6 @@ def test_create_options_stopwords assert_equal ["NOSTOPWORDS"], Valkey::Search::CreateOptions.new(no_stopwords: true).to_args end - # An empty list turns stopword removal off (`STOPWORDS 0`) rather than being dropped. def test_create_options_stopwords_empty stopword_count = 0 assert_equal ["STOPWORDS", stopword_count], Valkey::Search::CreateOptions.new(stopwords: []).to_args