-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Supercharge prepareValue
#555
Conversation
Prefer positive tests; group tests for specific objects.
`arrayString` duplicated too much of `prepareValue`'s logic, and so didn't receive bugfixes for handling dates with timestamps. Defer to `prepareValue` whenever possible. This change enforces double-quote escaping of all array elements, regardless of whether escaping is necessary. This has the side-effect of properly escaping JSON arrays.
Attempt to call a `toPostgres` method on objects passed as query values before converting them to JSON. This allows custom types to convert themselves to the appropriate PostgreSQL literal. This strategy is fully backwards-compatible and uses the same pattern as the `toJSON` override.
Oh, and benchmarks. before:
after:
Introduces some slight overhead when preparing (very) large number/object arrays, but actually speeds up string arrays. Overhead of custom types is due to circular reference checking. In any case, this difference is likely insignificant in real-world use cases. |
Whoah this is pretty awesome! You're right on every point. Only thing I'm slightly curious about is
Is that going to be a backwards compatibility break? It's fine if it is, but I just need to document it more carefully if that's a "breaking" change. Other than that, this is awesome. I like the |
@benesch just waiting for your feedback about the array dates. I'll get this merged soon afterwards. :) |
@brianc sorry! Got tied up with work. You're very right about date array timezone handling being a breaking change. Is it easier for you if I rebase that out of this PR? Also, there's a quick change I want to make to the |
Pass `toPostgres` type-coercers a reference to the `prepareValue` function to ease constructing literals composed of other Postgres types.
Okay, done! See above. That makes it a little easier to construct, say, a date range literal DateRange.prototype.toPostgres = function (prepare) {
return util.format("[%s,%s)", prepare(this.lower), prepare(this.upper));
} since I don't have to duplicate the logic for constructing a timezone-adjusted date literal. So the specification for
|
This is awesome! The backwards compatible breaking change is fine - I'll release a new major version with this change & document it accordingly. Thanks a ton! 👍 💃 |
This PR addresses a few small issues:
utils.prepareValue
didn't have unit tests for existing functionality. Now it does!utils.prepareValue
appears to have grown organically over the past few years, so the conditional was convoluted.arrayString
duplicatedprepareValue
's logic and so didn't get the fix. I've refactoredarrayString
to drop down toprepareValue
whenever possible so hopefully this doesn't happen again!json[]
) were improperly escaped. The fix for examples don't seem to work out-of-the-box #3 actually fixed this too.And one big one:
0008c53 adds support for a
toPostgres
method, a latoJSON
. If an object passed toprepareValue
has a function property calledtoPostgres
, it's invoked and the return value is used in place ofJSON.stringify
.Even better, the value returned by an object's
toPostgres
is passed toprepareValue
recursively, sotoPostgres
doesn't need to return a Postgres literal, just a more primitive JS type.For context, I've been trying to implement support for range types. Parsing range literals into objects on the way out couldn't be easier (thanks!), but there's no sane way to override
utils.prepareValue
to convert a JS range object to a Postgres range literal.toPostgres
is the simplest strategy for custom type-coercion on the way in I could think of. A type registry—register preparers for JS types—is tricky because JS'sinstanceof
operator fails hard. Supporting multipleprepareValue
handlers is possible, but ordering would be a nightmare. Would love to hear your thoughts! The code comments seem to indicate you're not thrilled about the way prepareValue works currently.I've tried to split my commits sensibly so you can pick-and-choose as you see fit. Cheers!