postgresql orderBy() does not properly utilize NULLS third argument

[owenallenaz]

Environment

Knex version: 0.95.14
Database + version: postgres 14.0

Bug

1. The orderBy() command in postgres does not function properly for postgres. Currently, the command orderBy('field', 'desc', 'first') compiles to ORDER BY ("field" is NULL) desc but in postgres it needs to be ORDER BY "field" desc NULLS LAST.

2. Example generating incorrect order strings - https://runkit.com/embed/k2t79bssiic7

3. Documentation for the proper postgres syntax is at https://www.postgresql.org/docs/14/queries-order.html .

I'd be happy to attempt to fix the problem if someone could point me to the right files in the code that modified this behavior. I looked around the code but couldn't find an obvious place in the https://github.com/knex/knex/blob/master/lib/dialects/postgres/query/pg-querycompiler.js where it would go.

[OlivierCavadenti]

Check MSSQL specific syntax, it's the same idea :

knex/lib/dialects/mssql/query/mssql-querycompiler.js

Line 204 in e06a2c2

_formatGroupsItemValue(value, nulls) {

Order by null syntax is created in _formatGroupsItemValue function and it need to be overrided like MSSQL.

[OlivierCavadenti]

And please add integration test to test order of results in all DB. If we have the problem, it seems integration tests are not exhaustive.

[owenallenaz]

So if I add a function with the exact same name into the postgres-querycompiler I can expect the same input/output contract and would just need to make it spit out the postgres equivalent?

For the unit tests where do I go to add the unit tests and bootstrap in the test data accordingly? Is there an good example in the repo I can use as a blueprint?

[OlivierCavadenti]

So if I add a function with the exact same name into the postgres-querycompiler I can expect the same input/output contract and would just need to make it spit out the postgres equivalent?

Yes, if you put the function with the same name, like in mssql-querycompiler, knex will use it.

For the unit tests where do I go to add the unit tests and bootstrap in the test data accordingly? Is there an good example in the repo I can use as a blueprint?

Please see the test with name 'order by with null' in selects.spec.js (line 562). I made an integration test but if you have the bug, maybe it's not good enough.

For unit test, see in builder.js (in test/unit/query), line 5748 (first test is 'order by, null first').

[OlivierCavadenti]

@owenallenaz say me if you need help for unit test. You can also push a PR and I will help you.

[kibertoad]

Released in 1.0.2