[Bugfix][Oracle] path order in routing_policy working for null/empty values

[Martouta]

Test it locally with oracle DB for the test: test/unit/proxy_test.rb:103
For Oracle the empty string is saved as NULL.

[codecov]

Codecov Report

Merging #1173 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1173      +/-   ##
==========================================
+ Coverage   92.99%   93.03%   +0.04%     
==========================================
  Files        2454     2455       +1     
  Lines       80951    80986      +35     
==========================================
+ Hits        75281    75348      +67     
+ Misses       5670     5638      -32

Impacted Files	Coverage Δ
app/lib/backend_api_logic/routing_policy.rb	100% <100%> (ø)	⬆️
...pec/acceptance/api/line_item_variable_cost_spec.rb	100% <0%> (ø)
app/lib/three_scale/api/responder.rb	97.87% <0%> (+2.12%)	⬆️
lib/tasks/impersonation_admin_user.rake	100% <0%> (+10%)	⬆️
app/queries/new_accounts_query.rb	82.6% <0%> (+10.86%)	⬆️
lib/tasks/multitenant/tenants.rake	52.83% <0%> (+11.32%)	⬆️
app/models/application_record.rb	81.25% <0%> (+18.75%)	⬆️
config/initializers/access_token_authentication.rb	77.77% <0%> (+22.22%)	⬆️
.../initializers/backport_pg_10_support_to_rails_4.rb	80% <0%> (+50%)	⬆️
config/initializers/postgres.rb	100% <0%> (+50%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76b5ed5...7c0ba70. Read the comment docs.

[guicassolato]

We can use COALESCE instead (with sift) and make it Oracle only just in case.

[Martouta]

Spoke with Gui, and we discovered that baby squeel does not offer reorder construct yet: rzane/baby_squeel#73

[Martouta]

Coalesce is not ordering properly for me:

[Martouta]

backend_api_configs.reorder('CASE WHEN path IS NULL THEN 1 ELSE 0 END, path DESC') works for the 3 DBs, however it might be a better idea for speed in the other DBs (not sure, so share your opinions) doing the specific query of Oracle only on Oracle so the others do not execute stuff they do not need, like this:

reorder = System::Database.oracle? ? 'CASE WHEN path IS NULL THEN 1 ELSE 0 END, path DESC' : {path: :desc}
backend_api_configs.reorder(reorder)

[Martouta]

in fact then i could make it oracle specific like:

reorder = System::Database.oracle? ? 'path DESC NULLS LAST' : {path: :desc}
rules = backend_api_configs.reorder(reorder).each_with_object([]) do |config, rules|
# ...

[guicassolato]

I'd say let's make it specific for Oracle. Even though it works for all supported DBs to this day, a principle applies: the fix is meant for Oracle only.

One day we may have to support some weird DB to which this fix does NOT work and neither is necessary. Another thing that could happen is – who knows? – we drop support to Oracle one day. Then we'd end up with some ugly code for DBs that do need it.

[Martouta]

So these are the 2 options, and both work in all DBs:

rules = backend_api_configs.reorder('CASE WHEN path IS NULL THEN 1 ELSE 0 END, path DESC').each_with_object([]) do |config, rules|
# ...

reorder = System::Database.oracle? ? 'path DESC NULLS LAST' : {path: :desc}
rules = backend_api_configs.reorder(reorder).each_with_object([]) do |config, rules|
# ...

[Martouta]

Right, good 👍 So I will push the last once 👍 And it is also definitely more understandable at first glance 😄

[Martouta]

Just discovered that reordering exists now in baby squeel 😄because i tried it locally after seeing https://www.bountysource.com/issues/53307207-reordering-without-explicit-direction-doesn-t-work-with-last-in-rails-4
So I will do the sifter 😄

[guicassolato]

Nice job!

As a future improvement (in case we ever need it), we could make sifter :path_desc more generic so it works with any column name and both ways :asc and :desc.