Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle incompatible aggregate function and column in orderBy directive #2487

Open
jdjoshuadavison opened this issue Dec 14, 2023 · 3 comments
Labels
enhancement A feature or improvement

Comments

@jdjoshuadavison
Copy link

jdjoshuadavison commented Dec 14, 2023

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.

@spawnia spawnia added the enhancement A feature or improvement label Dec 15, 2023
@spawnia
Copy link
Collaborator

spawnia commented Dec 15, 2023

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

@jdjoshuadavison
Copy link
Author

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.

@spawnia
Copy link
Collaborator

spawnia commented Dec 20, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or improvement
Projects
None yet
Development

No branches or pull requests

2 participants