Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translate pg sql NULL to javascript undefined #1274

Closed
arusakov opened this issue Apr 24, 2017 · 18 comments
Closed

Translate pg sql NULL to javascript undefined #1274

arusakov opened this issue Apr 24, 2017 · 18 comments

Comments

@arusakov
Copy link

I don't know, maybe current translation NULL to null have some reasons, but from my point of view undefined is more useful, because undefined omitted in JSON automatically:

JSON.stringify({ x: null }) // {"x":null}
JSON.stringify({ x: undefined }) // {}

@brianc
What do you think about it?

@brianc
Copy link
Owner

brianc commented Apr 24, 2017

I don't think it's a bad idea at all, but it has potential to be a very subtle very breaking change if people are relying on strict equality checks for null on some of their query results. I think at this point we should consider the ship sailed on using null instead of undefined for result values & then if you want to strip them off in your own app you could do something like rows.map(row => _.omit(row, (val, key) => val === null or something? That's a kinda unfortunate side-effect of having two empty values in JavaScript.

I tagged to version 7.x so I can think about it when I start working on some backwards incompat changes to the next breaking version....but I'm leaning towards "that is how it is" right now.

@charmander
Copy link
Collaborator

null is the correct value. If you want to omit it from JSON, that’s easy enough:

const replaceNull = (key, value) =>
    value === null ?
        undefined :
        value;

JSON.stringify({ x: null, y: 1.2 }, replaceNull) // {"y":1.2}

@arusakov
Copy link
Author

arusakov commented Apr 25, 2017

@charmander
Do you have real benefits of null usage? I don't know examples.
Also I know projects, that don't use null at all. For example TypeScript

@brianc
Maybe my proposition can be achieved through new setting?

@charmander
Copy link
Collaborator

I use null as an intentional “nothing” value, since undefined tends to be a mistake (given it’s what you get by reading a property that doesn’t exist).

Strongly −1 on implementing this, as a setting or otherwise.

@abenhamdine
Copy link
Contributor

abenhamdine commented Apr 25, 2017

Also -1 on this, null is imho the correct value. As charmander said, undefined is more interpreted as the absence of a given property than the absence of value for this property.

Edit: Also when you look at/manipulate your db response, you want to easily distinguish if a column is null or if the column is missing in the response (because of a mistake or because this column has not been included in the select).

@rpedela
Copy link
Contributor

rpedela commented Apr 29, 2017

NULL is a real value in the database and has a specific meaning. We shouldn't be tampering with data in a database client library. You can write your own custom type parser(s) if you want to override how NULL is handled.

@vitaly-t
Copy link
Contributor

vitaly-t commented Apr 30, 2017

https://www.youtube.com/watch?v=lITBGjNEp08

@jfsimon
Copy link

jfsimon commented May 16, 2017

It would be a huge BC break!

@caub
Copy link
Contributor

caub commented May 23, 2017

Strongly +1, it may be configurable, I don't know if it's there https://github.com/brianc/node-postgres/blob/master/lib/result.js#L60 but easy to make a configurable 'empty' value, I'll try a PR

a JSON.stringify callback is an overhead, and wanting null in json responses doesn't make sense

@charmander
Copy link
Collaborator

wanting null in json responses doesn't make sense

  • sure it does
  • producing JSON directly from rows with JSON.stringify is not the single use case for this module

@caub
Copy link
Contributor

caub commented May 24, 2017

sure it does

why?

reducing the data size sent is important (without overhead), not related to this lib of course, but null or undefined is arbitrary, the JS null doesn't have to correspond to the PG's NULL, they are unrelated

@sehrope
Copy link
Contributor

sehrope commented May 24, 2017

why?

Because otherwise you can't represent the existence of a key without a value. While null/undefined are semantically similar in most cases in JS, one big difference is how they're handled when enumerating properties or serializing an object to JSON. In both case the undefined value does not show up.

That means that {foo: null} would be serialized to {}.

reducing the data size sent is important, not related to this lib of course, but null or undefined is arbitrary, the JS null doesn't have to correspond to the PG's NULL, they are unrelated

It does have to correspond. Correctness comes before all else. Otherwise you can't even pass the test of round tripping your data (JS => PG => JS => PG ...). You'd lose information (the presence of keys with NULL values).

If you want to "slim things down" then handle it yourself either with a custom type processor or directly in you app layer.

@brianc
Copy link
Owner

brianc commented May 24, 2017

I thought about this for a bit, and agree with @charmander & @sehrope 100%. We will not change from null to undefined as the default value.

Unfortunately I don't think currently you can work around this with a custom type parser. There is a world somewhere in the future where we could make it possible to override this default in your app with a new setting, but since the effort involved is non-trivial and we have more important stuff to work on I don't know when that will happen.

Plus: it's super easy to strip nulls off your results after you get them from the library if that's how you want to go.

Just to reiterate to relax anyone w/ fears of this being a future breaking change: as of now there is no way we are changing from null to undefined. Not only as @sehrope said is it less correct, but the amount of breakage has the potential to be huge and the upside is minimal at best. I've removed this from consideration for 7.0. As always I'm open to a pull request to make this a setting if the changes are non-invasive, but I have some performance concerns since we turn values into null very deep within the parsing code in a very hot code path.

@georgir
Copy link

georgir commented May 25, 2017

Specifically for the purpose of JSON.stringify it is trivial to use its second argument and replace null with undefined yourself if that's how you want to roll... There's no point to add breaking changes to unrelated libraries for that purpose.

https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify
JSON.stringify({wtf:null},(key, value)=>value===null?undefined:value)

@brianc
Copy link
Owner

brianc commented Jun 16, 2017

Going to close this as we're not going to change from null to undefined Thanks for the discussion though!

@brianc brianc closed this as completed Jun 16, 2017
@caub
Copy link
Contributor

caub commented Jul 8, 2017

another point where undefined is more interesting is for destructuring and default values

const {rows:[{x={}, y={}}]} = await pool.query(`select 
    (select login from test where login='44' limit 1) AS x, 
    (select access from test20 limit 1) AS y`)
console.log(x, y) // default won't work when x or y is null

and again {x: undefined} is not the same than {}, so people who wants nulls could do JSON.stringify(rows, (k,v)=>v===undefined ? null : v) or the configurable way like discussed, but it's a pain

It's also consistent since undefined's are also converted to NULL when inserting/updating data

@rpedela
Copy link
Contributor

rpedela commented Jul 8, 2017

@caub Write a little code to override the behavior using the type parsers. That is your only option, the current default behavior isn't changing.

var pg = require('pg');

// json
pg.types.setTypeParser(114, function (value) {
    // TODO: replace with your custom logic
    return JSON.parse(value.toString());
});

// jsonb
pg.types.setTypeParser(3802, function (value) {
    // TODO: replace with your custom logic
    return JSON.parse(value.toString());
});

@caub
Copy link
Contributor

caub commented Jul 13, 2017

@rpedela thanks, I think I could also try json_strip_nulls, probably more performant than js's JSON.stringify(.., ...)

WITH stuff AS (
	SELECT * from (VALUES('1',null)) AS t(id,name)
)
select json_agg(json_strip_nulls(row_to_json(ss))) from (select * from stuff) ss;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants