Fix client-level string_fastpath: true being silently ignored#1101
Merged
Conversation
Closes #1100 `use_string_fastpath?` only checked `req_options`, so `Dalli::Client.new(servers, string_fastpath: true)` had no effect on normal `set` calls — the fast path was never taken unless the option was also passed per-request. Add `string_fastpath` to `ValueSerializer::DEFAULTS`/`OPTIONS` so it is stored in `@serialization_options` at init time, and fall back to that value in `use_string_fastpath?` when the per-request option is absent. Per-request still takes full precedence (true overrides client-level false, and false overrides client-level true). Tighten the existing integration test with a serializer spy so it actually verifies the fast path is taken rather than just that the round-trip produces the right value (Marshal also preserves encoding, so the old test passed even when the fast path was silently skipped). Add an integration test for the per-request override behavior and unit tests covering all four combinations of client-level and per-request option values. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1100.
Problem
Dalli::Client.new(servers, string_fastpath: true)had no effect.use_string_fastpath?only checkedreq_options, so the fast path was never taken for normalclient.set(key, value)calls — only whenstring_fastpath: truewas passed as a per-request option on each individualset.The existing integration test at line 17 of
test_serializer.rbdidn't catch this because it only verified the round-trip result. Marshal also preserves string encoding, so the test passed whether or not the fast path was actually taken.Fix
Add
string_fastpathtoValueSerializer::DEFAULTSandOPTIONSso it is stored in@serialization_optionsat init time. Inuse_string_fastpath?, fall back to the client-level value when the per-request option is absent. Per-request still takes full precedence in both directions:trueoverrides client-levelfalsefalseoverrides client-leveltrueTests
ValueSerializer#store.Test plan
bundle exec ruby -Itest test/integration/test_serializer.rb— 6 runs, 40 assertions, 0 failuresbundle exec ruby -Itest test/protocol/test_value_serializer.rb— 32 runs, 68 assertions, 0 failuresbundle exec rubocop— no offenses🤖 Generated with Claude Code