nuwave/lighthouse

Handle incompatible aggregate function and column in `@orderBy` directive

Opened this issue · 3 comments

What problem does this feature proposal attempt to solve?

The orderBy directive provides 5 aggregate functions: AVG, MIN, MAX, SUM, COUNT. Not all of these functions can be used with all column types. This should be handled and should return a user friendly message in the response.

Specifically, AVG and SUM only work on numeric column types.

Which possible solutions should be considered?

Handling the specific issue mentioned with AVG and SUM on non-numeric types, could be solved simply with:

$relationModel = $builder->getModel()->$relation()->getRelated()->whereNotNull($relationColumn)->first();

if (! is_numeric($relationModel->$relationColumn) && in_array($aggregate, ['sum', 'avg'])) {
    throw new Error("$relation cannot be aggregated by SUM or AVG, please choose a different aggregate and try again.");
}

However, a more robust way of testing whether the aggregate function actually works with the specified column would be better.

Instead of performing an additional database query, can we catch the thrown error and wrap it?

Instead of performing an additional database query, can we catch the thrown error and wrap it?

The only benefit of testing before doing the query is that you could still return results without the ordering applied and return an error in the errors object. But honestly, I don't think that's hugely beneficial, so catching the thrown error should be fine.

Alright. The first step towards implementing this would be to add a test that reproduces the error and create a pull request with it.