Closed jurko-gospodnetic closed 9 years ago
listOnEmpty
is there to give a hint to nesthydration
when it has no information about whether a object or set of objects was expected because it only has an empty array for data. If it had the data it would also have column name and would be able determine the correct return type from those.
But in the case of postgres
where we need to gather information about the columns anyways for other reasons, and so we don't need any hints for the return type because even in the case of empty data the column are inspected by nesthydration
and the correct type returned.
Or at least that is how I remember it.
I think the given reasoning is flawed or perhaps we misunderstood each other about what it is I'm reporting in this issue.
knexnest
does some custom internal tweaking when the knex
connection given to it is connected to a Postgres database. But it does so behind the scenes, and only so some 'too long column names' could be handled correctly.
So I expect that if I give it a simple SQL statement (especially one using only short column names) that returns no data, e.g. SELECT id FROM some_table WHERE 1 = 0;
, it should return the same result, whether running on Postgres or some other database.
The current code base does not. It returns an empty array []
when connected to a Postgres database or null
when connected to any other database.
While this library did start as just a mapper for long column names and compatibility with postgres the assertion that SELECT id FROM some_table WHERE 1 = 0
should return an empty array is incorrect.
A query like SELECT id FROM some_table
doesn't prefix column names with _
and therefore specifies an object
or null
result type. If an empty dataset is passed into nesthydration, null
will be the result. Likewise using knexnest with SELECT id FROM some_table WHERE 1 = 0
for either a postgres or other db connection will return null
.
A query like SELECT id AS "_id" FROM some_table
does prefix column names with _
and therefore specifies an array
result type. If an empty dataset is passed into nesthydration, and empty array should be the result. However on its own this is not enough information for nesthydration and it will return its default of null
. Now if the hint structPropToColumnMap
param is specified as either true or with the actual column names then nesthydration has enough information to return an array
result for the empty input data.
It likewise follows that knexnest uses what additional information it learns about a query to return the correct type. So for postgres and the query SELECT id AS "_id" FROM some_table WHERE 1 = 0
it correctly returns an empty array. Unfortunately for non postgres connections knexnest learns no additional information and results in the nesthydration default of null
.
I believe the bug is that the documentation doesn't specify that if using knexnest for code that is intended to be cross database compatible then the listOnEmpty
is necessary. If using it only for postgres
or in the future other databases that are supported then it is not necessary.
Sounds like this could benefit from some unit tests. ^_^
I think the problem is it needs documentation more than unit tests. The behaviour is correct (I think) but might not be intuitive.
Ok, seems my original query SELECT id FROM some_table WHERE 1 = 0
was indeed too simple, but even with it, and assuming the current implementation is doing everything as designed, doesn't this happen:
listOnEmpty
set to true
--> knexnest
returns []
listOnEmpty
set to true
--> knexnest
returns null
And does that not conflict with:
I believe the bug is that the documentation doesn't specify that if using knexnest for code that is intended to be cross database compatible then the listOnEmpty is necessary.
It is true that
when used with non-postgress database and listOnEmpty set to true --> knexnest returns []
but this would be user error because you are instructing it to give you a object in the formation of the query and then asking it for an array with the listOnEmpty hint.
this scenario
when used with postgress database and listOnEmpty set to true --> knexnest returns null
should throw an error, I don't think it does and that should be rectified
when using postgress conection and listOnEmpty while doing a query for an object now throw an error. Version 0.3.1.
KnexNest
takes an explicitlistOnEmpty
parameter, but only passes it onto the underlyingNestHydrationJS.nest()
call if it is not dealing with a Postgres database connection.If it is dealing with a Postgres database connection, then it ignores the passed
listOnEmpty
parameter value & always returns an empty list instead ofnull
when there is no data to return.Since the special Postgres database connection handling is supposed to be just a little internal tweak related to the maximum column length allowed by Postgres, this seems like unwanted behaviour, and causes breakage when switching between database types.
My suggestion is to do away with the
listOnEmpty
parametrization all together and simply have the function always use an empty array to indicate an empty result set. I see no reason to cause issues with returning a completely different data type in that case.