graphile / crystal

🔮 Graphile's Crystal Monorepo; home to Grafast, PostGraphile, pg-introspection, pg-sql2 and much more!
https://graphile.org/
Other
12.63k stars 572 forks source link

Can't call PostgreSQL functions with arguments of type "money" #468

Closed farant closed 6 years ago

farant commented 7 years ago
CREATE FUNCTION test (money)
RETURNS integer AS $$
BEGIN
  RETURN 123;
END;
$$ LANGUAGE plpgsql;

SELECT test(1.0); -- Error, can't find function.
SELECT test(1 + 0.0); -- Error, can't find function.
SELECT test(1.0::money); -- works

It looks like the value passed to postgraphql is translated into "($1 + 0.0)" for the SQL used to call the database function. This converts the value to a "numeric" type which isn't specific enough for the function to be found.

Until this is fixed anyone should be able to work around the problem by using the decimal type for their function arguments instead.

Took a long time to figure out what was going on. 😬

DealerUplift-DavidJohnston commented 7 years ago

I'm a bit lost here but the money type is not an intrinsic number so it cannot be represented without single quotes in PostgreSQL. select(1.0::money) constructs a numeric (the default non-integer, not-quoted, type) and then casts it to money. select('1.0'::money') bypasses the numeric altogether and passing the literal '1.0' to money's input function. I'm not sure how this system handles something like a date literal but the treatment of money probably shouldn't be any different.

benjie commented 7 years ago

I've got a bit of a hack in v4 for dealing with this kind of thing:

https://github.com/postgraphql/graphql-build/blob/7165585cea78c3f1f8192841de4199fdc3db4f2b/packages/graphql-build-pg/src/plugins/PgTypesPlugin.js#L215-L225

Are you saying that casting a float directly to ::money fixes it, or do we need to handle money more carefully? If we need to change it to a string, for instance, then v4 would be the time to do it because it's already going to be a breaking change.

DealerUplift-DavidJohnston commented 7 years ago

I'd say its reasonable to map the PostgreSQL money type to the GraphQL string type since mapping it to the GraphQL float type is lossy.

In the original example: select test('1.0'); executes without complaint; as does select test('$1.0'). No explicit casting needed (absent function overloading).

Hope this helps.

benjie commented 7 years ago

According to the PostgreSQL documentation:

The money type stores a currency amount with a fixed fractional precision; see Table 8-3. [...] Input is accepted in a variety of formats, including integer and floating-point literals, as well as typical currency formatting, such as '$1,000.00'. Output is generally in the latter form but depends on the locale.

I then goes on to say how you can cast floats to money via numeric but that you should not do so due to the potential for rounding errors.

I see we have three options:

  1. integer pennies - involves multiplying by 100 which makes values "weird", also does not work for every currency
  2. float units - could have rounding errors
  3. strings - very awkward to deal with (ref: https://github.com/postgraphql/graphql-build/blob/c04b156ef6b041823c19c4175874a2400cec7d40/packages/graphql-build-pg/src/plugins/PgTypesPlugin.js#L315-L325)

v4 is flexible enough that you can override its typing; so I think we'll go with option 2 and trust that anyone using money in the database is aware of the caveats around treating money as a float, and that if it's a problem for them they can use a plugin that replaces the float implementation with string/int depending on their preference.

DealerUplift-DavidJohnston commented 7 years ago

I suppose, though, that the real question is meeting the GraphQL spec requirement: All response formats must support string representations, and that representation must be used here. http://facebook.github.io/graphql/#sec-String

Now, I don't exactly understand how that works here since it implies that all non-string outputs have a secondary string form (which is accessed how?), but practically speaking a user of the money type is likely OK with having it treated as an alias for numeric in most cases - and in theory could request the string representation for situations where that is not the case.

The fact that money type doesn't actually store the reported currency means that practically speaking it is the same as numeric/decimal expect with a bit more liberal/useful input parsing routine. Thus option 2 indeed seems like an adequate solution. But I do believe you need to consider the string aspect too, in order to make all this work bi-directionally. Pretending that money is a float when talking to PostgreSQL isn't going to work since PostgreSQL doesn't agree with that supposition - it wants to see the input data provided as a string (specifically, a quoted literal) - which is what the original complaint message is pointing out.

benjie commented 7 years ago

I'm assuming that refers to the fact you can specify raw values in the GraphQL query string.

We'll follow the docs on casting to money via numeric.