From 4ad050734d608c8a5cc3e009f879d10f32292218 Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Mon, 19 May 2025 12:45:34 +0200 Subject: [PATCH 1/4] Extend test coverage for boolean query parameters The previous implementation is handling expected query parameters with a `false` value in an unintended manner. Rather than appending the parameter as expected, it is omitted. This is in contrast to parameters not included in the pattern, where a `false` value would be appended. In a test-driven development fashion, this commit adds a failing test first and thereby allows working on the bug more easily. --- spec/restify/relation_spec.rb | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/spec/restify/relation_spec.rb b/spec/restify/relation_spec.rb index 1a0da64..4fd2f55 100644 --- a/spec/restify/relation_spec.rb +++ b/spec/restify/relation_spec.rb @@ -59,6 +59,27 @@ def to_param it { expect(expanded.to_s).to eq 'http://test.host/resource/1,2' } end + context 'with nil query parameter' do + let(:pattern) { '/resource{?abc}' } + let(:params) { {abc: nil} } + + it { expect(expanded.to_s).to eq 'http://test.host/resource?abc' } + end + + context 'with false query parameter' do + let(:pattern) { '/resource{?abc}' } + let(:params) { {abc: false} } + + it { expect(expanded.to_s).to eq 'http://test.host/resource?abc=false' } + end + + context 'with true query parameter' do + let(:pattern) { '/resource{?abc}' } + let(:params) { {abc: true} } + + it { expect(expanded.to_s).to eq 'http://test.host/resource?abc=true' } + end + context 'with unknown additional query parameter' do let(:pattern) { '/resource{?a,b}' } let(:params) { {a: 1, b: 2, c: 3} } From 8318c08d343336e87f28a50a25d0092688320536 Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Mon, 19 May 2025 13:45:41 +0200 Subject: [PATCH 2/4] Extend test coverage for boolean query parameters The previous implementation is handling expected query parameters with a `false` value in an unintended manner. Rather than appending the parameter as expected, it is omitted. This is in contrast to parameters not included in the pattern, where a `false` value would be appended. In a test-driven development fashion, this commit adds a failing test first and thereby allows working on the bug more easily. --- spec/restify/relation_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/restify/relation_spec.rb b/spec/restify/relation_spec.rb index 4fd2f55..26611ec 100644 --- a/spec/restify/relation_spec.rb +++ b/spec/restify/relation_spec.rb @@ -63,7 +63,7 @@ def to_param let(:pattern) { '/resource{?abc}' } let(:params) { {abc: nil} } - it { expect(expanded.to_s).to eq 'http://test.host/resource?abc' } + it { expect(expanded.to_s).to eq 'http://test.host/resource' } end context 'with false query parameter' do From bde8dee056f90e6a5df7783b2b20810d3ac31200 Mon Sep 17 00:00:00 2001 From: Jan Graichen Date: Mon, 19 May 2025 13:59:38 +0200 Subject: [PATCH 3/4] fix: Restore literal false in parameter expansion Restore putting literal false values into query params. Previously, literal false values were ignored from template expansion, resulting in unexpected queries: get(params: {public: true, hidden: false}) -> /?public=true This commit changes the parameter expansion to include `false` as a literal value. This does include `false` in path params, which will result in e.g. `/resource/false` now, but that seems "more correct" too. --- CHANGELOG.md | 2 ++ lib/restify/relation.rb | 6 ++++-- spec/restify/relation_spec.rb | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78be1c8..40b8abc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixes +- Restore literal false in parameter expansion (#68) + ### Breaks ## 2.0.1 - (2025-02-16) diff --git a/lib/restify/relation.rb b/lib/restify/relation.rb index c5f0cb1..75a3a3c 100644 --- a/lib/restify/relation.rb +++ b/lib/restify/relation.rb @@ -113,8 +113,10 @@ def convert_param(value, nesting: true) def extract!(params) @template.variables.each_with_object({}) do |var, hash| - if (value = params.delete(var) { params.delete(var.to_sym) { nil } }) - hash[var] = value + if params.key?(var) + hash[var] = params.delete(var) + elsif params.key?(var.to_sym) + hash[var] = params.delete(var.to_sym) end end end diff --git a/spec/restify/relation_spec.rb b/spec/restify/relation_spec.rb index 26611ec..ab72329 100644 --- a/spec/restify/relation_spec.rb +++ b/spec/restify/relation_spec.rb @@ -38,7 +38,7 @@ def to_param context 'with false' do let(:params) { {id: false} } - it { expect(expanded.to_s).to eq 'http://test.host/resource/' } + it { expect(expanded.to_s).to eq 'http://test.host/resource/false' } end context 'with true' do From af29bb35b095e6c1cae3a3339beb79fec0744bc5 Mon Sep 17 00:00:00 2001 From: Jan Graichen Date: Mon, 19 May 2025 14:15:04 +0200 Subject: [PATCH 4/4] fix: Ensure mixed-keyed params are properly cleaned If a params hash contains template placeholders with string and symbol keys, both are removed but symbol keys preferred. Technically, the preferred key is an implementation detail in Addressable::Template, but for now the tests here depend on that. --- lib/restify/relation.rb | 6 ++++-- spec/restify/relation_spec.rb | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/lib/restify/relation.rb b/lib/restify/relation.rb index 75a3a3c..9b469cd 100644 --- a/lib/restify/relation.rb +++ b/lib/restify/relation.rb @@ -113,10 +113,12 @@ def convert_param(value, nesting: true) def extract!(params) @template.variables.each_with_object({}) do |var, hash| + sym = var.to_sym if params.key?(var) hash[var] = params.delete(var) - elsif params.key?(var.to_sym) - hash[var] = params.delete(var.to_sym) + end + if params.key?(sym) + hash[sym] = params.delete(sym) end end end diff --git a/spec/restify/relation_spec.rb b/spec/restify/relation_spec.rb index ab72329..c54f519 100644 --- a/spec/restify/relation_spec.rb +++ b/spec/restify/relation_spec.rb @@ -140,6 +140,38 @@ def to_param it { expect { expanded }.to raise_exception TypeError, /Can't convert Hash into String./ } end + + context 'with string-keyed params' do + context 'with value' do + let(:params) { {'id' => 1} } + + it { expect(expanded.to_s).to eq 'http://test.host/resource/1' } + end + end + + context 'with mixed-keyed params' do + context 'non-overlapping' do + let(:params) { {'id' => 1, q: 2} } + + it { expect(expanded.to_s).to eq 'http://test.host/resource/1?q=2' } + end + + context 'overlapping (1)' do + let(:params) { {'id' => 1, id: 2} } + + it 'prefers symbol value' do + expect(expanded.to_s).to eq 'http://test.host/resource/2' + end + end + + context 'overlapping (2)' do + let(:params) { {id: 2, 'id' => 1} } + + it 'prefers symbol value' do + expect(expanded.to_s).to eq 'http://test.host/resource/2' + end + end + end end shared_examples 'non-data-request' do |method|