Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

PHPORM-46 Throw an exception when Query\Builder::orderBy() is used with invalid direction #7

Merged
merged 3 commits into from
Jul 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/Query/Builder.php
Original file line number Diff line number Diff line change
Expand Up @@ -513,11 +513,16 @@ public function distinct($column = false)

/**
* @inheritdoc
* @param int|string|array $direction
*/
public function orderBy($column, $direction = 'asc')
{
if (is_string($direction)) {
$direction = (strtolower($direction) == 'asc' ? 1 : -1);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the most impactful change: any invalid value was converted to a "desc" direction. This seems wrong.

$direction = match ($direction) {
'asc', 'ASC' => 1,
'desc', 'DESC' => -1,
default => throw new \InvalidArgumentException('Order direction must be "asc" or "desc".'),
};
}

if ($column == 'natural') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Expand Down
82 changes: 82 additions & 0 deletions tests/Query/BuilderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
namespace Jenssegers\Mongodb\Tests\Query;

use DateTimeImmutable;
use Illuminate\Tests\Database\DatabaseQueryBuilderTest;
use Jenssegers\Mongodb\Connection;
use Jenssegers\Mongodb\Query\Builder;
use Jenssegers\Mongodb\Query\Processor;
Expand Down Expand Up @@ -63,6 +64,66 @@ public static function provideQueryBuilderToMql(): iterable
fn (Builder $builder) => $builder->limit(10)->offset(5)->select('foo', 'bar'),
];

/** @see DatabaseQueryBuilderTest::testOrderBys() */
yield 'orderBy multiple columns' => [
['find' => [[], ['sort' => ['email' => 1, 'age' => -1]]]],
fn (Builder $builder) => $builder
->orderBy('email')
->orderBy('age', 'desc'),
];

yield 'orders = null' => [
['find' => [[], []]],
function (Builder $builder) {
$builder->orders = null;

return $builder;
},
];

yield 'orders = []' => [
['find' => [[], []]],
function (Builder $builder) {
$builder->orders = [];

return $builder;
},
];

yield 'multiple orders with direction' => [
['find' => [[], ['sort' => ['email' => -1, 'age' => 1]]]],
fn (Builder $builder) => $builder
->orderBy('email', -1)
->orderBy('age', 1),
];

yield 'orderByDesc' => [
['find' => [[], ['sort' => ['email' => -1]]]],
fn (Builder $builder) => $builder->orderByDesc('email'),
];

/** @see DatabaseQueryBuilderTest::testReorder() */
yield 'reorder reset' => [
['find' => [[], []]],
fn (Builder $builder) => $builder->orderBy('name')->reorder(),
];

yield 'reorder column' => [
['find' => [[], ['sort' => ['name' => -1]]]],
fn (Builder $builder) => $builder->orderBy('name')->reorder('name', 'desc'),
];

/** @link https://www.mongodb.com/docs/manual/reference/method/cursor.sort/#text-score-metadata-sort */
yield 'orderBy array meta' => [
['find' => [
['$text' => ['$search' => 'operating']],
['sort' => ['score' => ['$meta' => 'textScore']]],
]],
fn (Builder $builder) => $builder
->where('$text', ['$search' => 'operating'])
->orderBy('score', ['$meta' => 'textScore']),
];

yield 'distinct' => [
['distinct' => ['foo', [], []]],
fn (Builder $builder) => $builder->distinct('foo'),
Expand All @@ -74,6 +135,27 @@ public static function provideQueryBuilderToMql(): iterable
];
}

/**
* @dataProvider provideExceptions
*/
public function testException($class, $message, \Closure $build): void
{
$builder = self::getBuilder();

$this->expectException($class);
$this->expectExceptionMessage($message);
$build($builder);
}

public static function provideExceptions(): iterable
{
yield 'orderBy invalid direction' => [
\InvalidArgumentException::class,
'Order direction must be "asc" or "desc"',
fn (Builder $builder) => $builder->orderBy('_id', 'dasc'),
];
}

private static function getBuilder(): Builder
{
$connection = m::mock(Connection::class);
Expand Down