Use stRange to provide full-featured Range class

[benesch]

WIP. Problems:

• No errors when bounds invalid. This seems like a check that belongs in stRange.
• toJSON format is backwards-incompatible. @waisbrot is this a problem for WHOOP?

Fixes #3.

[moll]

Which invalid bounds do you have in mind? stRange doesn't enforce only numbers.

[benesch]

Oh, hi! Just submitted a PR: moll/js-strange#1.

[benesch]

Anyway, since whether or not to throw an error on invalid bounds seems like an upstream decision, @waisbrot this is ready to merge pending your approval and doc updates.

[waisbrot]

Hm, the JSON format actually is a problem. @butterflyhug what do you think? "lower" and "upper" aren't my favorite keys (I always reverse them) but do we really want to have to change all the client code?

[JoshRagem]

upper and lower correspond to the functions in the sql upper(range) and lower(range) so I think those are the right names

[moll]

There's indeed a handful of synonyms for range endpoints that depend on your axes' angles and context. I'm not against aliases in stRange...

[moll]

The toJSON of stRange originated from some of my own code that serializes all objects with toJSON when sending them to Pg. I found that a more pure approach than sprinkling toPostgres. If I'm not mistaken, it should actually work if you just alias toJSON to toPostgres.

[butterflyhug]

If we can use the updated library in API v5 and keep the old version in our legacy API v4, I don't think it'd be the end of the world. That change might hit @andrewkhatutsky harder. The web app aggressively upgrade ranges from that JSON format into Twix ranges, so most of our code is isolated from the wire format and we'd only have a focused bit of code to update. If Andrew similarly only deals with the wire format for HTTP requests => insert into Core Data, it might not actually be that bad all around.

[waisbrot]

toPostgres is converting the range to a Postgres-friendly string. We're talking about interacting with the JS range object by doing things like if (onething.during.upper < otherthing.during.upper) { where it's kind of nice to use the same words as Postgres. But I don't feel strongly about that; our JS code shouldn't need to look like our SQL code.

The other thing is that we serialize the range object to JSON to send it to clients (and also clients send us a serialization) so renaming the keys is a pretty big change.

[moll]

If I throw an upper and lower alias to stRange, that web app mentioned above could theoretically just map the input back to an instance of stRange with Range.parse and then pass that object to whatever code there was before.

I actually think the math-y serialization format beats the object format for precisely the reason the latter depends on key names.

[moll]

toPostgres is converting the range to a Postgres-friendly string.

Yep, and stRange's toJSON should result in the same Pg compatible format.

[butterflyhug]

toJSON in stRange, for reference: https://github.com/moll/js-strange/blob/master/index.js#L229-L259

[waisbrot]

I actually think the math-y serialization format beats the object format for precisely the reason the latter depends on key names.

Oh, I should actually read your fine documentation instead of just skimming Benesch's tests.

Yeah, I agree with you. I do like that more than an object with 2-3 keys.

[butterflyhug]

I haven't tested current behavior, but

  // FIXME: How to serialize an empty range with undefined endpoints?

worries me, since we regularly serialize ranges with undefined endpoints.

[moll]

worries me, since we regularly serialize ranges with undefined endpoints.

Right. I just saw that too. I'm glad I made a note of it when I implemented. I see your code previously stringified to empty. That's Postgres-specifc... It'd be nice if Pg understood (,) just as it does (,0).

[waisbrot]

We never use ranges where both ends are undefined. But are you sure that Postgres won't accept [,)? It was fine with it in psql (which is often more lax about types).

[butterflyhug]

Yeah, looks to me like Postgres does accept [,) (and canonicalizes that range on disk to (,) for tsrange). [EDIT: at least in 9.4]

Postgres uses its string syntax to serialize its ranges to JSON; I think that's another minor data point in favor of stRange's JSON.

[moll]

Ah, no, you're right, @waisbrot. I misremembered the Pg syntax. That comment refers to an empty range as opposed to an infinite range. The former Pg seems to represent by the string "empty", rather than something with []. That's what I was referring to:

I see your code previously stringified to empty.

[butterflyhug]

Ah, right. I don't think there's an alternative syntax for empty, but I'm not sure we especially care about empty ranges either.

[moll]

A typed language could represent the latter without resorting to English with (0,0) for integer ranges, but that won't work in JS.

SELECT '(0,0)'::int8range
int8range: empty

[benesch]

@moll to respond to your earlier point about aliasing toJSON as toPostgres—that doesn't quite work, because toJSON doesn't handle escaping properly—nor empty ranges. In case you'd be interested in "fixing" this upstream, these are the broken tests: https://travis-ci.org/WhoopInc/node-pg-range/jobs/103925652

[benesch]

Actually, I'm not sure this is solvable without toPostgres, because toPostgres needs to be recursively called on the endpoints of the range. (The logic for converting a JS date to a Postgres date is actually rather complicated.) Details: brianc/node-postgres#555 (comment)

[moll]

Ah, in that case, @benesch, indeed.

Though, in general, I'm not a fan of the OO toPostgres approach. It's too invasive in a language without built-in traits or type classes. For compatibility with code decoupled from persistence (i.e. code using classes directly, rather than their wrappers with toPostgres) it requires mapping (converting), and once you do that, you might as well handle serializing in that mapper (converter function). Though that discussion might be continued in brianc/node-postgres#786, an issue I've neglected. :)

[benesch]

I see your point, @moll, though I don't think it's particularly worse than littering everything with toJSON.

[paynecodes]

@benesch Safe to use this PR?

[benesch]

@jpdesigndev kind of. See the test failures here: https://travis-ci.org/WhoopInc/node-pg-range/jobs/103925652. If you don't need empty range support, or support for non-integral ranges, you're safe with this PR. (strange doesn't currently encode tsranges or ranges of custom types properly.)

[moll]

I'm not against adding a better serialization format for empty ranges than "[undefined,undefined]", but it'll probably be something mathematical, rather than the English string "empty". :)

One quicker "fix" for serializing objects whose toString doesn't return their canonical form would be for stRange to call toJSON (with JSON.stringify) on the endpoints. That would at least cover JavaScript Date — its toJSON returns the ISO 8601 format in UTC that PostgreSQL accepts. Up to year 9999 and above 0, though, but for most use-cases that's fine. :)

[benesch]

@moll here's my proposal: this library exposes a Range class that is a subclass of stRange.js's Range class with a toPostgres method tacked on. That means the Postgres-specific serialization
code is isolated from your class, and you're free to design whatever JSON serialization format you so choose. Thoughts? I've updated this PR for this approach, and I'm pretty happy with it!

If you ever do come up with an alternate approach for Postgres serialization that doesn't require a .toPostgres, let me know, and we can adjust this module to match. I'm perfectly happy with a world where the only interesting part of this module is parser.js.

@jpdesigndev this PR now passes all the tests, so it should be safe to use!

[paynecodes]

@benesch Awesome! I don't see anything changing other than docs in 7104302. What am I missing that caused the tests to pass?

[benesch]

Sorry, force-pushed for clarity! Here's the diff from the last time you looked at this PR:

diff --git a/lib/range.js b/lib/range.js
index 13abd62..7445a3b 100644
--- a/lib/range.js
+++ b/lib/range.js
@@ -1,6 +1,19 @@
 var Range = require("strange")
   , util = require("util");

+function formatBound(value, prepare) {
+  if (value === null) {
+    return "";
+  }
+
+  value = prepare(value);
+  if (/[()[\],"\\]/.test(value)) {
+    // quote bound only if necessary
+    value = "\"" + value.replace(/(\\|")/, "\\$1") + "\"";
+  }
+  return value;
+}
+
 function PGRange(begin, end, bounds) {
   if (!(this instanceof PGRange)) {
     return new PGRange(begin, end, bounds);
@@ -10,6 +23,16 @@ function PGRange(begin, end, bounds) {

 util.inherits(PGRange, Range);

-PGRange.prototype.toPostgres = PGRange.prototype.toJSON;
+PGRange.prototype.toPostgres = function (prepare) {
+  if (this.isEmpty()) {
+    return "empty";
+  }
+
+  return util.format("%s%s,%s%s",
+    this.bounds[0],
+    formatBound(this.begin, prepare),
+    formatBound(this.end, prepare),
+    this.bounds[1]);
+};

Basically this library has always been very anal about properly serializing JavaScript objects into PostgreSQL literals. stRange's .toJSON approximates the PostgreSQL format in 95% of cases (and indeed @moll recommends it for this purpose), so the first version of this PR deferred serialization to stRange. This update simply layers the original serialization logic on top of stRange, so you get all the handy behavior of stRange objects plus the battle-tested serialization logic this library has always used.

Expect to see this land in the next few days unless @moll has some serious concerns!

[paynecodes]

Thanks @benesch. This is extremely helpful!

[moll]

I'm all game. Implementations we can change anytime. ;)

[benesch]

Merged in 1.0.0! 🍻