geddy / model

Datastore-agnostic ORM in JavaScript
265 stars 55 forks source link

Querying by property of type `number` doesn't work in postgres #191

Closed troyastorino closed 10 years ago

troyastorino commented 10 years ago

I have a model where one of the properties is of type number. Let's call the model Example, the property num, and say that there is an instance where num = 2.8.

Running:

geddy.model.Example.all({num: 2.8}, function (err, results) {
...
});

returns no results. I figured out that this is due to the way postgres treats columns with datatype real (that's what model makes property with type number). Model generates a query that looks something like this:

SELECT * FROM examples WHERE num = 2.8;

To make that query work properly for postgres, it needs to be changed to:

SELECT * FROM examples WHERE num::numeric = 2.8;

After that change the query returns the row that would be expected.

I figured this would be a generally desirable behavior, and so jumped into the model code to see if I could make it append ::numeric to the column names of properties with type number. I though that maybe the right approach would be to create lib/adapters/transformers/postgres and have it override some functions in lib/adapters/transformers/sql, but I couldn't figure out which function would need to be overridden.

Would this be a good general change, and any ideas of where to dive in?

mde commented 10 years ago

Thanks for finding this. It's a pretty significant bug with our Postgres adapter. I had a look at the Postgres docs for "numeric constants" here: http://www.postgresql.org/docs/9.2/static/sql-syntax-lexical.html#SQL-SYNTAX-CONSTANTS and it looks like the problem is both the column and the value have to be the same datatype, and Postgres is assuming the 2.8 is a numeric. I think a more correct fix would be to coerce the passed-in value to the same datatype as the column (not the other way around, as with your fix).

The docs describe two ways of doing it -- both seem to work with my installed version of Postgres 9.3.4:

SELECT * FROM foo WHERE bar = REAL '1.2';  -- string style
SELECT * FROM foo WHERE bar = 1.2:real;  -- PostgreSQL (historical) style

This is definitely something we need to fix, but we want to limit the impact of this sort of adapter-specific hacking as much as possible. I'll poke around tomorrow and see if I can give you some specific ideas about where this should go.

troyastorino commented 10 years ago

@mde did you have a chance to poke around at all?

mde commented 10 years ago

You'll want to make a change here:

https://github.com/geddy/model/blob/master/lib/adapters/transformers/sql.js#L9

The transformers are mixed onto the adapters, and executed there, so you can just check for "this.name" and append the ":real."

troyastorino commented 10 years ago

Appending ":real" didn't fly with the generated query, so I used the string style query instead.