Aggregates can return unsafe integers

Summary

We have some sums of numeric columns that exceed the range of safe integers in JS. The sum aggregator and others build the JSON object with the number directly without seeing if it's within bounds.

Obviously postgraphile and this plugin aren't limited to just the JS ecosystem, but a large consumer of postgraphile are surely browsers.

Steps to reproduce

Query an aggregate such that it returns a number larger than 2^53 - 1.

Expected results

A result such as:

{ "@sum": { "@amount": "29531249370647025750" } }

Actual results

{ "@sum": { "@amount": 29531249370647025750 } }

Which when parsed in JS yields (note the lost precision):

> JSON.parse('{"@sum":{"@amount":29531249370647025750}}')
{ '@sum': { '@amount': 29531249370647028000 } }

Additional context

Possible Solution

I'm willing to submit a patch to fix this issue, but need some direction on how to best implement this functionality.

To me, it seems that numerics, due to their arbitrary length both before and after the decimal place, should always be cast to text before getting returned, but this isn't backwards compatible.

Alternatively, additional aggregates could be introduced that can be used to cast to text when the aggregates are likely to exceed safe bounds and it's up to the consumer to parse those into numbers.

Is the GraphQL type (BigInt/BigFloat/etc) correct? If so the issue is just that we’re not correctly casting it to text in postgres and fixing that should be non-breaking. If the types in the schema are wrong then it’s more problematic…

The type of the column in question is numeric(30,0). When querying this field with postgraphile, it is cast to text which seems to be the correct behavior. The aggregations of this column are not cast to text by this plugin.

The issue seems to be that the "tweaks" aren't being applied (where a fragment is cast to ::text or converted to JSON or whatever to make it safe).

build.pgTweakFragmentForTypeAndModifier is defined here:
https://github.com/graphile/graphile-engine/blob/516e8f7aab03b371626ee84bd2160b31d949dd55/packages/graphile-build-pg/src/plugins/PgTypesPlugin.js#L293

It takes fragment, type, typeModifier = null, resolveData. I can't remember why it needs resolveData... Hopefully you can ignore that for the aggregates, or pass a stub value. Please investigate though.

fragment would be the sqlAggregate here:

pg-aggregates/src/AddAggregateTypesPlugin.ts

Line 201 in 8c2acd1

const sqlAggregate = spec.sqlAggregateWrap(sqlColumn);

type and typeModifier are defined here:

pg-aggregates/src/AddAggregateTypesPlugin.ts

Line 175 in 8c2acd1

const [pgType, pgTypeModifier] = spec.pgTypeAndModifierModifier

You may also need to apply the same changes for procs, though I suspect this is already handled elsewhere.

Please make sure that these fixes are covered by tests - I'd recommend you start by writing failing tests (both of procs and regular SQL), commit that in its own commit, and then start applying the changes to fix the tests.

The type of the column in question is numeric(30,0). When querying this field with postgraphile, it is cast to text which seems to be the correct behavior. The aggregations of this column are not cast to text by this plugin.

To be clear, I was asking about the types that represent these aggregates in the GraphQL schema itself - i.e. what is the type of the aggregate field for this in GraphiQL. If those types are wrong then that's an issue with pgTypeAndModifierModifier, but I strongly suspect that the types in the GraphQL schema are correct and it's merely the generated SQL that is incorrect.

Ugh it seems that this module doesn't actually have any tests. 🤦‍♂️ Clearly ran out of time when implementing it. Okay skip the tests.

To be clear, I was asking about the types that represent these aggregates in the GraphQL schema itself - i.e. what is the type of the aggregate field for this in GraphiQL. If those types are wrong then that's an issue with pgTypeAndModifierModifier, but I strongly suspect that the types in the GraphQL schema are correct and it's merely the generated SQL that is incorrect.

Gotcha, that is BigFloat so that part is working correctly.

I'll look into adding some tweakers for the aggregates, thanks!

Pretty sure this is solved in the PostGraphile V5 version of this plugin.

Can confirm that this definitely returns a string in V5 👍

Great! Hope to check it out soon