brianc / node-pg-types

Type parsing for node-postgres
268 stars 55 forks source link

Allow null values to be passed to type parser. #77

Closed 2fours closed 5 years ago

2fours commented 5 years ago

I'd like to be able to pass null values to the type parser so I can remap them to undefined so they don't appear as null properties in the json objects.

bendrucker commented 5 years ago

That's out of step with the way this parser works. It maps postgres column oids to functions that can convert raw data from PG into appropriate JS types. Handling specific value types is up to each oid -> JS parser. Handling nulls typically amounts to just returning them.

The "json objects" in this scenario sound like something you're serializing elsewhere in your code (i.e. before you send an HTTP response) and not JSON coming out of PG. That would be the best place to handle "remove nulls" instead of at database parsing time.

If you still think there's merit in handling this here and can make a stronger case for why, we'd be happy to hear it.

2fours commented 5 years ago

Type parsers are assigned by datatypeid. Some varchars in the database are null, some are not. I don't want the properties on the json object to exist if they are null in the database which can be achieved in one step by allowing null to trigger the parser which then writes undefined as the value. This saves a step of then having to scrub the json objects at the eg. response level as you suggest. Why even allow parsers at all? Based on your argmunt we should just convert the data at the JSON object level always.

The parsers are written to ignore NULL values, but NULL is in fact a value and should also be allowed to be manipulated in this way.

bendrucker commented 5 years ago

Based on your argmunt we should just convert the data at the JSON object level always.

That's not a good-faith assessment of this project. Simple JS types can easily and reliably handled after the fact via JS type checks. pg types would be unreasonable/impossible to detect without accompanying type information and the parsers themselves are sometimes extremely complex.

Alternative areas to add parsing logic are:

"Saving a step" is not a high priority compared to encouraging users to place their business logic in the most appropriate layer of their stack. If you'd like to make the case that this should be configurable at the DB level, the next step would be to submit a PR so we can at least discuss what this feature would look like.

2fours commented 5 years ago

I understand where you're coming from re. this case should probably be handled at a different layer.

Perhaps a better argument then is that the type parser behavior is inconsistent. If I setup a type parser for varchars it should be called for all varchars regardless of value. I was surprised when they were not called for null. In my mind I had setup a type parser for varchar which should be called for every value of varchar - null or not. To me the default behavior should be it calls the type parser for null as that is consistent, and that behavior can be turned off with a configuration option.

bendrucker commented 5 years ago

If that's the case, please direct me to specific types that have inconsistent behaviors (or send PRs if you can).

We wrap a lot of parsers to pass nulls through: https://github.com/brianc/node-pg-types/blob/3281edf6a5dad0f46c5e78f8676a39d984a5f437/lib/textParsers.js#L7-L12

There's no parser by default for text types that can just be passed through. But you should be able to register them.

2fours commented 5 years ago

My point is you are making a decision whether or not to call the parser based on the value of the column. This is inconsistent, the database driver should not be checking the values and deciding whether or not to call the parser. It should just call the parser.