Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,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
Expand Down
4 changes: 2 additions & 2 deletions lib/valkey/search/aggregate.rb
Original file line number Diff line number Diff line change
Expand Up @@ -226,9 +226,9 @@ class AggregateOptions
# @param slop [Integer, nil] emit `SLOP <n>`
# @param dialect [Integer, nil] emit `DIALECT <n>` (only 2 is valid)
# @param timeout [Integer, nil] emit `TIMEOUT <ms>`
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
Expand Down
2 changes: 1 addition & 1 deletion lib/valkey/search/create_options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 6 additions & 4 deletions lib/valkey/search/query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,19 @@ 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)
# @param no_content [Boolean] emit NOCONTENT (keys only)
# @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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions test/unit/commands/search_aggregate_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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")])
Expand Down
18 changes: 18 additions & 0 deletions test/unit/commands/search_builder_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,11 @@ def test_create_options_stopwords
assert_equal ["NOSTOPWORDS"], Valkey::Search::CreateOptions.new(no_stopwords: true).to_args
end

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
Expand All @@ -138,6 +143,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
Expand Down Expand Up @@ -169,6 +180,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)],
Expand Down
46 changes: 36 additions & 10 deletions test/unit/commands/search_query_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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")
Expand Down
Loading