Skip to content
This repository has been archived by the owner on Mar 26, 2024. It is now read-only.

the other way #5

Closed
vitaly-t opened this issue Jul 30, 2016 · 8 comments
Closed

the other way #5

vitaly-t opened this issue Jul 30, 2016 · 8 comments

Comments

@vitaly-t
Copy link

Or go the other way...

It is worth noting that the other way won't work with any of the frameworks built on top of node-postgres, while the initial approach, by using the pg object directly will work with everything ;)

I even would recommend to simplify it by removing the other approach altogether, to avoid confusion ;)

@benesch
Copy link
Contributor

benesch commented Jul 30, 2016

This library works great with e.g. Bookshelf and Knex. Here's a fully-functional example that goes both ways:

var pg = require("pg"),
    Range = require("pg-range").Range;

require("pg-range").install(pg);

var knex = require('knex')({
  client: 'pg',
  connection: {
    'host': 'localhost',
  },
});

var bookshelf = require('bookshelf')(knex);

var Event = bookshelf.Model.extend({
  tableName: 'events',
})

knex.schema.dropTableIfExists('events')
  .then(function () {
    return knex.schema.raw('CREATE TABLE events (id serial, during tsrange)');
  })
  .then(function () {
    return new Event({ during: Range(new Date(Date.now() - 1), new Date()) })
      .save();
  })
  .then(function () {
    return Event.fetchAll();
  })
  .then(function (models) {
    range = models.at(0).get('during');
    console.log(range); // [Sat Jul 30 2016 11:36:58 GMT-0400 (EDT),Sat Jul 30 2016 11:36:58 GMT-0400 (EDT)]
    console.log(range instanceof Range); // true
    console.log(range.end.getFullYear()); // 2016
  })
  .catch(console.log);

So you'll have to be more specific! ;) Which frameworks does this module not work with? Seems like it's a problem with those frameworks.

@benesch
Copy link
Contributor

benesch commented Jul 30, 2016

Ah! Is your concern with the wording, perhaps? You still need to call require("pg-range").install("pg")—let me see if I can make that clearer.

@vitaly-t
Copy link
Author

There are lots of libraries out there that implement their own custom query formatting, and they won't understand syntax such as Range object being passed in as a parameter. That's what I meant.

@vitaly-t
Copy link
Author

vitaly-t commented Jul 30, 2016

I'm trying to use it with pg-promise:

var Range = require("pg-range").Range;
var format = require('pg-promise').as.format;
var s = format('INSERT INTO table VALUES ($1)', [Range(1, 3)]);
console.log(s);

and getting the following:

INSERT INTO table VALUES ('"[1,3]"')

I'm not sure this is the correct range presentation though. Or am I wrong?

@benesch
Copy link
Contributor

benesch commented Jul 30, 2016

Ah, you wrote your own query formatter! Of course this library is incompatible with a custom query formatter! It only claims to be compatible with node-postgres's query formatter. ;)

To answer your question: no, that's not quite the right format. That's the string format, which is almost the same as the Postgres format, except in a few important edge cases, like empty ranges. To get the proper string format, you'll need to call range.toPostgres(converter) (source), where converter is a function that can stringify the component type. So if you have a Range(new Date(), new Date()), converter will be called for each date to get the proper Postgres-formatted string for each date, and those strings will be properly concatenated together into a range literal.

So in order for pg-promise to be compatible with this library, pg-promise needs to check for the presence of a toPostgres method on each value passed in to format, and invoke it appropriately if it exists. Here's the PR I submitted to node-postgres several years ago to add this behavior: brianc/node-postgres#555.

@vitaly-t
Copy link
Author

I see now. In pg-promise I implemented support for Custom Type Formatting to make any type compatible with the query-formatting engine.

So, I've just tried this:

'use strict';

var pgRange = require("pg-range");
var format = require('pg-promise').as.format;

function range() {
    var args = arguments;
    return {
        _rawDBType: true,
        formatDBType: function () {
            var r = pgRange.Range.apply(null, args);
            return r.toPostgres();
        }
    };
}

var s = format('INSERT INTO table VALUES ($1)', [range(1, 3)]);

console.log(s);

but I got:

D:\NodeJS\tests\test1.js:11
            var r = new pgRange.Range.apply(null, args);
                    ^

TypeError: pgRange.Range.apply is not a constructor

Wat do you think should be the right way to implement the custom type for pg-promise? Function dbFormatType is expected to return just a text string.

@benesch
Copy link
Contributor

benesch commented Jul 31, 2016

I think you pasted the wrong error? I get this error using exactly your code:

/Users/benesch/Desktop/test-pg-range/node_modules/pg-range/lib/range.js:9
  value = prepare(value);

Anyway, the way to resolve that is simple! toPostgres takes one parameter, a function that knows how to convert range component types (integers, dates, whatever) to their proper Postgres string form. This seems to be require('pg-promise').format.value, and indeed, this patch makes your example succeed:

@@ -9,7 +9,7 @@
         _rawDBType: true,
         formatDBType: function () {
             var r = pgRange.Range.apply(null, args);
-            return r.toPostgres();
+            return r.toPostgres(require('pg-promise').as.value);
         }
     };
 }

I see two ways we can proceed. One is entirely on your end: you can check for toPostgres in pg-promise, and call it appropriately if it exists. I'm also more than happy to add a formatDBType method and a _rawDBType flag to the Range object in this library as you've done above. The one sticking point is that it needs a reference to the as.value function passed in as a parameter. I don't want a call to require("pg-promise") in this library, since that needlessly couples an otherwise general API to your library.

@benesch
Copy link
Contributor

benesch commented Dec 29, 2016

Closing due to inactivity. Feel free to reopen if you find some time!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants