You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Yes, this is an erroneous type inference, for the boolean type of JS, the serelizer to the postgres boolean type is always used, but this is not correct for JSON
On the meantime, maybe you can use sql.json instead?
Unfortunately, it is not possible to use the json method in my cases, the sql query is dynamically built and at the time of construction, I do not know the type of the parameter
Ok but your cast to ::jsonb will make postgres call JSON.stringify anyway, so where is the problem? sql.json just returns an object that tells postgres which oid to use in the query, so if you always cast to JSONB it should work I guess
@uasan oh shoot, that's actually not the only issue. Same goes for the other inferred types (Date, Buffer, Bigint) :-( I should have caught this for the v3 update and completely removed inferType (
It was really an assumption on my part wrt. handling of values after supporting the PostgreSQL feedback, but I guess you can say I might have caught it if I had had a test case for these json situations. Hindsight is 20/20 ;)
Currently a simple test case I have now is enabling this behaviour which prevents json from working out of the box:
sql`select ${ true } as x` // [{ x: true }]
So we can't really have both, but I think it would have been more correct to support the json case, and accept that input without explicit types just becomes strings? Eg this would have been better behavior:
sql`select ${ true } as x` // [{ x: 'true' }] notice `true` is a string
sql`select ${ true }::boolean as x` // [{ x: true }]
sql`select ${ true }::jsonb as x` // [{ x: true }]
But.. still initial thinking - might feel differently after sleeping on it :P
@AkeleyUA the issue is that we can't really change it without potentially breaking someone who relies on the implicit inference. It would require a new major, and I haven't thought through if that's the way to go yet. Could you maybe describe your specific issue / show an example query, and we can see if there's a way to work around it?
show an example query, and we can see if there's a way to work around it?
Dynamic query builder that knows the column name and values, but does not know the type of the column
sql`UPDATE table SET "${sql(name)}" = ${value}`
potentially breaking someone who relies on the implicit inference
I'm pretty sure no one is relying on true boolean when json is expected, I'm pretty sure this bug is not reproducible now because everyone is using the old sql.json() helper interface, I don't think fixing this bug will break backwards compatibility.
I think It's useful when calling functions that don't do the same implicit type casts as regular queries. I'm pretty sure someones setup will break if changing it, and the fact that it's an explicit functionality tested for, it would definitely require a major bump (but I don't mind if that's necessary).
So if we make that change, it would be removing all implicit type inference from js types supplied in the Parse message and rely completely on the PostgreSQL ParameterDescription. The actual code fix could just be changing the inferType function to x => x instanceof Parameter ? x.type : 0.
I'll try to see if I can find any good reasons not to do it 😉
Inference data type for Postgres from the data type of JS, has sense only if the ParameterDescription returned an unknown type, if I'm wrong, give an example that proves you are right, I don't argue, I'm even curious)
I think it's not possible because the types specified from our end are supplied in the Parse message which comes before the ParameterDescription message. So there is no way for us to tell Postgres "ok you don't know the type so we would actually like to use x".
Activity
Minigugus commentedon Jun 1, 2022
Maybe an issue with parameters types inference 🤔 On the meantime, maybe you can use
sql.json
instead?uasan commentedon Jun 1, 2022
Yes, this is an erroneous type inference, for the boolean type of JS, the serelizer to the postgres boolean type is always used, but this is not correct for JSON
Unfortunately, it is not possible to use the
json
method in my cases, the sql query is dynamically built and at the time of construction, I do not know the type of the parameterMinigugus commentedon Jun 1, 2022
Ok but your cast to
::jsonb
will makepostgres
callJSON.stringify
anyway, so where is the problem?sql.json
just returns an object that tellspostgres
which oid to use in the query, so if you always cast to JSONB it should work I guessuasan commentedon Jun 1, 2022
The problem is that the
postgres.js
does not callJSON.stringify
, it passes boolean valuestrue
as't'
, that's why I created this issue )porsager commentedon Jun 1, 2022
@uasan oh shoot, that's actually not the only issue. Same goes for the other inferred types (Date, Buffer, Bigint) :-( I should have caught this for the v3 update and completely removed inferType (
postgres/src/types.js
Line 213 in 218a7d4
uasan commentedon Jun 1, 2022
You, like many good developers, simply do not have enough tests for many cases, you need to think about how the community can help you write tests
porsager commentedon Jun 1, 2022
It was really an assumption on my part wrt. handling of values after supporting the PostgreSQL feedback, but I guess you can say I might have caught it if I had had a test case for these json situations. Hindsight is 20/20 ;)
Currently a simple test case I have now is enabling this behaviour which prevents json from working out of the box:
So we can't really have both, but I think it would have been more correct to support the json case, and accept that input without explicit types just becomes strings? Eg this would have been better behavior:
But.. still initial thinking - might feel differently after sleeping on it :P
uasan commentedon Jun 1, 2022
I think the best behavior in cases of
unknown
data type from postgres describe - direct mapping of JS types to Postgres types, i.e.AkeleyUA commentedon Jun 6, 2022
@porsager Hi. Should we expect a fix soon? If yes, how long?
P.S. I don't want to raise, but this is a pretty critical issue for us. Wish you happy coding.
porsager commentedon Jun 6, 2022
@AkeleyUA the issue is that we can't really change it without potentially breaking someone who relies on the implicit inference. It would require a new major, and I haven't thought through if that's the way to go yet. Could you maybe describe your specific issue / show an example query, and we can see if there's a way to work around it?
uasan commentedon Jun 6, 2022
Dynamic query builder that knows the column name and values, but does not know the type of the column
I'm pretty sure no one is relying on
true
boolean whenjson
is expected, I'm pretty sure this bug is not reproducible now because everyone is using the oldsql.json()
helper interface, I don't think fixing this bug will break backwards compatibility.porsager commentedon Jun 6, 2022
I think It's useful when calling functions that don't do the same implicit type casts as regular queries. I'm pretty sure someones setup will break if changing it, and the fact that it's an explicit functionality tested for, it would definitely require a major bump (but I don't mind if that's necessary).
So if we make that change, it would be removing all implicit type inference from js types supplied in the Parse message and rely completely on the PostgreSQL
ParameterDescription
. The actual code fix could just be changing theinferType
function tox => x instanceof Parameter ? x.type : 0
.I'll try to see if I can find any good reasons not to do it 😉
uasan commentedon Jun 6, 2022
Inference data type for Postgres from the data type of JS, has sense only if the
ParameterDescription
returned an unknown type, if I'm wrong, give an example that proves you are right, I don't argue, I'm even curious)porsager commentedon Jun 6, 2022
Yeah I'm very curious too☺️
I think it's not possible because the types specified from our end are supplied in the
Parse
message which comes before theParameterDescription
message. So there is no way for us to tell Postgres "ok you don't know the type so we would actually like to use x".17 remaining items