feat(pgsettings): Only use settings of type string or number

[skvale]

For example, instead of an undefined pgSetting becoming "undefined" turn it into null so set_config can still accept it

Fixes: #566

[benjie]

Thanks for raising this PR! 🙏 This is a good start, but I'd like a few tweaks in behaviour before it's ready to merge.

I'm thinking at the moment that this should go into v4 because it's effectively a breaking change; however if it's important to you that it goes into v3 we should discuss the comments I've made and come up with a non-breaking way forward (I'm thinking wherever the throws are we should instead output deprecation notices and then String(...) as you've done).

Let me know which way you want to go with this 👍

[benjie]

These colons should be commas.

[benjie]

I think it would be better to just completely omit the null values than to set them to null - it should be more efficient and should be equivalent (assuming the values are not set globally, for example).

[benjie]

This is not a safe way to check if the value should be excluded - they might deliberately be setting it to the empty string "" or to the number zero 0 and this will instead set null. A safer way would be pgSettings[key] != null ? String(...) : null; however I'd rather throw if the values are invalid:

strings and numbers should be passed through, null/undefined should be dropped, and everything else should throw an error.
-- #566 (comment)

So I'd like to see some typeof's around here. For anyone wondering: I'm deliberately suggesting we should throw on booleans, and on objects that have a toString value.

[skvale]

@benjie
That sounds good.

• I wasn't sure how to output deprecation warnings just console.warn?
• Still thinking of a better error/deprecation message "Error converting pgSetting: ${pgSetting} is an Object."
• Could also throw an error for Symbol types in response to

  everything else should throw an error

• Not sure why we should throw for booleans, but could add that check too

[benjie]

I wasn't sure how to output deprecation warnings just console.warn?

Yup; with DEPRECATED prefixed. Feel free to use chalk to make it more obvious!

Still thinking of a better error/deprecation message "Error converting pgSetting: ${pgSetting} is an Object."

Generally you should add what it is (as you have done) and what it should be - i.e. append "expected string or number" (no need to include null in the list IMO).

Could also throw an error for Symbol types in response to

Yes, literally everything that doesn't match (pgSetting == null || ['string', 'number'].includes(typeof pgSetting) should throw.

Not sure why we should throw for booleans, but could add that check too

Because in SQL it's not clear if you'd check for it to exist or be NULL, or for it to be "0" vs "1"; or "TRUE" vs "FALSE"; or "t" vs "f"; or "true" vs "false"

[skvale]

I based this PR off of v4 postgraphql:benjie/graphql-build

[skvale]

and addressed all of the requested changes, I think.

[benjie]

I'd still like to see this; but the tests need to pass on CI first 👍

[benjie]

Thanks @skvale!