PHPORM-46 Throw an exception when Query\Builder::orderBy() is used with invalid direction #7
Conversation
src/Query/Builder.php
Outdated
| $direction = match (strtolower($direction)) { | ||
| 'asc' => 1, | ||
| 'desc' => -1, | ||
| default => throw new \InvalidArgumentException(sprintf('Order direction must be either "asc" or "desc", "%s" given.', $direction)), |
There was a problem hiding this comment.
mongosh throws this error:
db.getCollection('books').find().sort({'_id': 'descd'});
MongoInvalidArgumentError: Invalid sort direction: "descd"
db.getCollection('books').find().sort({'_id': 2});
MongoInvalidArgumentError: Invalid sort direction: 2
PHPC throw this error:
MongoDB\Driver\Exception\CommandException : $sort key ordering must be 1 (for ascending) or -1 (for descending)
We can decide that it's not necessary to add an exception in the query builder since there is already an exception. But here, the exception is thrown as early as possible to help debug.
There was a problem hiding this comment.
I think an exception is fine for this string conversion logic, since one could argue this is the Laravel library itself converting a value. Beyond that, though, we should rely on the server to validate (as will be the case with $meta for text score).
| */ | ||
| public function orderBy($column, $direction = 'asc') | ||
| { | ||
| if (is_string($direction)) { | ||
| $direction = (strtolower($direction) == 'asc' ? 1 : -1); |
There was a problem hiding this comment.
This is the most impactful change: any invalid value was converted to a "desc" direction. This seems wrong.
src/Query/Builder.php
Outdated
| @@ -505,11 +505,16 @@ public function distinct($column = false) | |||
|
|
|||
| /** | |||
| * @inheritdoc | |||
| * @param int|string $direction | |||
There was a problem hiding this comment.
When sorting, MongoDB supports sorting by text score metadata, which would have an invocation like so:
$query->orderBy('textScore', ['$meta' => 'textScore']);While it looks like the code may handle that just fine, the parameter type hint suggests this is invalid. Feel free to leave that change to a follow-up ticket, but I wanted to point it out.
There was a problem hiding this comment.
We will need a test to cover this use-case. I'll add one after rebasing on #6.
There was a problem hiding this comment.
Thanks for the doc, I added a test case for this specific example, and updated the method signature.
| 'asc', 'ASC' => 1, | ||
| 'desc', 'DESC' => -1, | ||
| default => throw new \InvalidArgumentException('Order direction must be "asc" or "desc".'), | ||
| }; | ||
| } | ||
|
|
||
| if ($column == 'natural') { |
There was a problem hiding this comment.
Isn't this problematic if a model/collection actually contains a natural field? Even $natural is a valid field name since the server relaxed restrictions on using dots and dollars in fields names (although I'm less concerned about that edge case).
src/Query/Builder.php
Outdated
| $direction = match (strtolower($direction)) { | ||
| 'asc' => 1, | ||
| 'desc' => -1, | ||
| default => throw new \InvalidArgumentException(sprintf('Order direction must be either "asc" or "desc", "%s" given.', $direction)), |
There was a problem hiding this comment.
I think an exception is fine for this string conversion logic, since one could argue this is the Laravel library itself converting a value. Beyond that, though, we should rely on the server to validate (as will be the case with $meta for text score).
…th invalid direction (#7) * Convert only strings, let the driver fail for int values * Add more tests on Builder::orderBy
Fix PHPORM-46
Consistent with Laravel Query Builder.