Restore literal false in parameter expansion

[MrSerth]

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.

---

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.

---

The bug investigated (and covered through the test) has led to real-world failures. Let me take an example from openHPI. Consider a course that is hidden:

In a public export for MOOChub, this course should not be shown:

def courses
  @courses ||= course_api
    .rel(:courses)
    .get({
      public: true,
      exclude_external: true,
      hidden: false,
      alphabetic: true,
      page:,
      active_after: 3.years.ago.to_date.iso8601,
    }).value!
end

The request made to the respective service, however, does not include the hidden query parameter:

Consequently, the course is shown on <moochub.org>:

[jgraichen]

Thanks for reporting.

I took a brief look, and it seems to get more complicated, since false already has conflicting tests:

    context 'with false' do
      let(:params) { {id: false} }

      it { expect(expanded.to_s).to eq 'http://test.host/resource/' }
    end

I tend to change this to include false too... 🤔

[MrSerth]

Thanks for checking! I saw this "conflicting" test too, but wouldn't consider it a conflict necessarily: The test you cited is regarding the path parameters, not regarding query parameters. Theoretically, one (path) could behave differently than the other (query). Of course, I understand that this might be even more misleading...

Also, I was unsure about the inclusion of nil as a query parameter. I followed the path syntax (where the value is ommitted as well), but added the key. Of course, this decision can be discussed, too. I think, it is rather a design-decision how the interface should look like.

[jgraichen]

Unfortunately, Restify does not parse and fill URI templates, but explicitly delegates to Addressable::Template#example only. Restify cannot differ between path and non-path placeholders, but only between URI templates variables and extra query params.

Addressable::Template also hard-skips nils: https://github.com/sporkmonger/addressable/blob/main/lib/addressable/template.rb#L759.

[MrSerth]

Alright. Then, I'll refactor the nil test, so that the key is omitted as well.

[jgraichen]

Thanks! I do like ?abc though, but I really do not want to compile the template on my own 🙈 ...

[MrSerth]

No problem -- I liked ?abc, too, but I agree that compiling the template is not worth the effort. 👍

[jgraichen]

?abc only works, when the URI template does not include a {abc} placeholder. Then they are added as query values directly:

let(:pattern)  { '/resource/{id}{?q*}' }

context 'with additional nil parameters' do
  let(:params) { {id: '5', abc: nil} }

  it { expect(expanded.to_s).to eq 'http://test.host/resource/5?abc' }
end

[MrSerth]

Right, this part is also covered in the specs already:

restify/spec/restify/relation_spec.rb

Lines 75 to 79 in fe526fe

	context 'with additional nil parameters' do
	let(:params) { {id: '5', abc: nil} }
	it { expect(expanded.to_s).to eq 'http://test.host/resource/5?abc' }
	end

I would still suggest adding the explicit test for the placeholder (especially since it is removed completely), just to have this behavior documented in the code: https://github.com/MrSerth/restify/blob/8318c08d343336e87f28a50a25d0092688320536/spec/restify/relation_spec.rb#L62-L67

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.04%. Comparing base (fe526fe) to head (af29bb3).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
+ Coverage   95.88%   96.04%   +0.15%     
==========================================
  Files          21       21              
  Lines         681      683       +2     
==========================================
+ Hits          653      656       +3     
+ Misses         28       27       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[MrSerth]

Thanks for taking care of fixing the errounous behavior and specifying the behavior of overlapping keys. I really appreciate your efforts for this! 👍