search

dresende / node-sql-query

NodeJS SQL query builder
122 stars 103 forks source link

Postgres "Date" conversion gives errors #5

Closed mcannonbrookes closed 11 years ago

mcannonbrookes commented 11 years ago

Trying to insert a model with a Date type in Postgres results in the following error:

TypeError: Object Tue May 21 2013 13:12:53 GMT+0200 (CEST) has no method 'replace'
    at Object.exports.escapeVal (/Users/mike/dev/search-analytics/node_modules/orm/node_modules/sql-query/lib/Dialects/postgresql.js:35:19)
    at Object.InsertQuery.build (/Users/mike/dev/search-analytics/node_modules/orm/node_modules/sql-query/lib/Insert.js:26:24)
    at Driver.insert (/Users/mike/dev/search-analytics/node_modules/orm/lib/Drivers/DML/postgres.js:194:21)
    at /Users/mike/dev/search-analytics/node_modules/orm/lib/Instance.js:120:17
    at checkNextValidation (/Users/mike/dev/search-analytics/node_modules/orm/lib/Instance.js:42:12)
    at Instance.handleValidations (/Users/mike/dev/search-analytics/node_modules/orm/lib/Instance.js:66:10)
    at Instance.saveInstance (/Users/mike/dev/search-analytics/node_modules/orm/lib/Instance.js:95:3)
    at Object.Instance.Object.defineProperty.value (/Users/mike/dev/search-analytics/node_modules/orm/lib/Instance.js:336:7)
    at createNext (/Users/mike/dev/search-analytics/node_modules/orm/lib/Model.js:384:19)
    at Function.Model.model.create (/Users/mike/dev/search-analytics/node_modules/orm/lib/Model.js:413:3)

Looking in the source code, it seems that lib/Dialects/postrgresql.js method escapeVal doesn't handle Date objects properly yet.

Modifying the switch statement to look more like this:

switch (typeof val) {
        case "number":
            return val;
        case "boolean":
            return val ? "true" : "false";
        case "object": 
            if (val instanceof Date)
                return "'" + dateToString(val) + "'";
    }

seems to work - but I had to steal dateToString method from the node pg/lib/utils.js. This works but probably isn't the nicest solution so I won't send a PR.

Ideally this library would be a true dependency maybe and you could re-use their methods directly? (doesn't seem like your ORM library should be doing the escaping itself, that seems a little like the job of the core driver maybe?)

mcannonbrookes commented 11 years ago

Mate - any feedback or can I help on this one?

FYI I worked around it by converting the date to a PG acceptable String in my own code - but that seemed wrong?

dresende commented 11 years ago

I'm going to use your suggestion (and borrow code from pg for now). Thank you for your feedback.

dresende commented 11 years ago

About ORM escaping things, I'm looking to change the drivers in the future to use prepared statements and so avoid escaping both on ORM or node drivers. But for now I'll leave as it is.