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

Supercharge prepareValue #555

Merged
merged 7 commits into from Apr 6, 2014
Merged

Supercharge prepareValue #555

merged 7 commits into from Apr 6, 2014

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Mar 30, 2014

This PR addresses a few small issues:

  1. utils.prepareValue didn't have unit tests for existing functionality. Now it does!
  2. utils.prepareValue appears to have grown organically over the past few years, so the conditional was convoluted.
  3. Dates in an array were improperly converted to UTC. (Fixed for non-array dates by bde8717). arrayString duplicated prepareValue's logic and so didn't get the fix. I've refactored arrayString to drop down to prepareValue whenever possible so hopefully this doesn't happen again!
  4. Arrays of objects (Postgres type 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 la toJSON. If an object passed to prepareValue has a function property called toPostgres, it's invoked and the return value is used in place of JSON.stringify.

Even better, the value returned by an object's toPostgres is passed to prepareValue recursively, so toPostgres 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's instanceof operator fails hard. Supporting multiple prepareValue 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!

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.
@benesch
Copy link
Contributor Author

benesch commented Mar 31, 2014

Oh, and benchmarks.

before:

prepare-number-array: 14ms
prepare-string-array: 143ms
prepare-object-array: 94ms
prepare-object: 92ms
prepare-custom-type: 48ms

after:

prepare-number-array: 23ms
prepare-string-array: 117ms
prepare-object-array: 119ms
prepare-object: 96ms
prepare-custom-type: 123ms

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.

@brianc
Copy link
Owner

brianc commented Mar 31, 2014

Whoah this is pretty awesome! You're right on every point. Only thing I'm slightly curious about is

Dates in an array were improperly converted to UTC. (Fixed for non-array dates by bde8717). arrayString duplicated prepareValue's logic and so didn't get the fix. I've refactored arrayString to drop down to prepareValue whenever possible so hopefully this doesn't happen again!

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 .toPostgres thing as well because you're right - the instanceof operator does fail hard.

@brianc
Copy link
Owner

brianc commented Apr 3, 2014

@benesch just waiting for your feedback about the array dates. I'll get this merged soon afterwards. :)

@benesch
Copy link
Contributor Author

benesch commented Apr 3, 2014

@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 toPostgres function signature. Let me whip it up and send it your way.

Pass `toPostgres` type-coercers a reference to the `prepareValue`
function to ease constructing literals composed of other Postgres types.
@benesch
Copy link
Contributor Author

benesch commented Apr 3, 2014

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 toPostgres, as it's coded now, would look something like:

  • If a callable toPostgres (typeof obj.toPostgres === 'function') property exists on an object passed as a value to query, the value returned by calling that function is used in place of the JSON representation of the object.
  • The value returned by toPostgres is treated exactly as if that value had been passed directly to the query. That is, if toPostgres returns a Date instance, that date is converted to a Postgres date literal exactly as if that date had been passed directly to query.
  • To prevent any additional type-coercion,toPostgres should return a string or Buffer instance.
  • toPostgres receives an optional parameter prepare, which can be used to manually coerce component JS objects. For example, if constructing a date range literal (ex. '[2010-01-01T14:30, 2010-01-01T15:30)'), prepare could be called with each Date instance to return the Postgres date literal as a string.

@brianc
Copy link
Owner

brianc commented Apr 6, 2014

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!

👍 💃

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

Successfully merging this pull request may close these issues.

None yet

2 participants