From cf5350e26615dda0930bfd525f3dfd85f5b1c0e8 Mon Sep 17 00:00:00 2001 From: Alexandru Bucur Date: Mon, 23 Dec 2024 23:15:32 +0100 Subject: [PATCH] feat: implement a way of matching the fields to the jsonapi spec (#983) * feat: implement a way of matching the fields to the jsonapi spec * feat: introduce tests to cover the new jsonapi functionality * feat: introduce an easy way of running tests locally * feat: implement a way of matching the fields to the jsonapi spec * feat: introduce tests to cover the new jsonapi functionality * feat: introduce an easy way of running tests locally * fix: allow usage of the main table filtering the fields --- config/query-builder.php | 19 +++ database/factories/AppendModelFactory.php | 2 +- database/factories/TestModelFactory.php | 1 - src/Concerns/AddsFieldsToQuery.php | 50 +++++- src/Includes/IncludedRelationship.php | 21 ++- tests/FieldsTest.php | 120 ++++++++++++++ tests/TestCase.php | 4 +- tests/TestClasses/Models/TestModel.php | 13 ++ utils/run_tests.sh | 193 ++++++++++++++++++++++ 9 files changed, 409 insertions(+), 14 deletions(-) create mode 100755 utils/run_tests.sh diff --git a/config/query-builder.php b/config/query-builder.php index 36d3d9f5..94fca196 100644 --- a/config/query-builder.php +++ b/config/query-builder.php @@ -60,4 +60,23 @@ * GET /users?fields[userOwner]=id,name */ 'convert_relation_names_to_snake_case_plural' => true, + + /* + * By default, the package expects relationship names to be snake case plural when using fields[relationship]. + * For example, fetching the id and name for a userOwner relation would look like this: + * GET /users?fields[user_owner]=id,name + * + * Set this to one of `snake_case`, `camelCase` or `none` if you want to enable table name resolution in addition to the relation name resolution + * GET /users?include=topOrders&fields[orders]=id,name + */ + 'convert_relation_table_name_strategy' => false, + + /* + * By default, the package expects the field names to match the database names + * For example, fetching the field named firstName would look like this: + * GET /users?fields=firstName + * + * Set this to `true` if you want to convert the firstName into first_name for the underlying query + */ + 'convert_field_names_to_snake_case' => false, ]; diff --git a/database/factories/AppendModelFactory.php b/database/factories/AppendModelFactory.php index 3591b07e..26b38331 100644 --- a/database/factories/AppendModelFactory.php +++ b/database/factories/AppendModelFactory.php @@ -2,8 +2,8 @@ namespace Spatie\QueryBuilder\Database\Factories; -use Spatie\QueryBuilder\Tests\TestClasses\Models\AppendModel; use Illuminate\Database\Eloquent\Factories\Factory; +use Spatie\QueryBuilder\Tests\TestClasses\Models\AppendModel; class AppendModelFactory extends Factory { diff --git a/database/factories/TestModelFactory.php b/database/factories/TestModelFactory.php index 8b1001ad..8965abfb 100644 --- a/database/factories/TestModelFactory.php +++ b/database/factories/TestModelFactory.php @@ -16,4 +16,3 @@ public function definition() ]; } } - diff --git a/src/Concerns/AddsFieldsToQuery.php b/src/Concerns/AddsFieldsToQuery.php index 120c000e..905e1508 100644 --- a/src/Concerns/AddsFieldsToQuery.php +++ b/src/Concerns/AddsFieldsToQuery.php @@ -38,7 +38,16 @@ protected function addRequestedModelFieldsToQuery(): void $fields = $this->request->fields(); - $modelFields = $fields->has($modelTableName) ? $fields->get($modelTableName) : $fields->get('_'); + if (! $fields->isEmpty() && config('query-builder.convert_field_names_to_snake_case', false)) { + $fields = $fields->mapWithKeys(fn ($fields, $table) => [$table => collect($fields)->map(fn ($field) => Str::snake($field))->toArray()]); + } + + // Apply additional table name conversion based on strategy + if (config('query-builder.convert_relation_table_name_strategy', false) === 'camelCase') { + $modelFields = $fields->has(Str::camel($modelTableName)) ? $fields->get(Str::camel($modelTableName)) : $fields->get('_'); + } else { + $modelFields = $fields->has($modelTableName) ? $fields->get($modelTableName) : $fields->get('_'); + } if (empty($modelFields)) { return; @@ -49,23 +58,46 @@ protected function addRequestedModelFieldsToQuery(): void $this->select($prependedFields); } - public function getRequestedFieldsForRelatedTable(string $relation): array + public function getRequestedFieldsForRelatedTable(string $relation, ?string $tableName = null): array { - $tableOrRelation = config('query-builder.convert_relation_names_to_snake_case_plural', true) - ? Str::plural(Str::snake($relation)) - : $relation; + // Possible table names to check + $possibleRelatedNames = [ + // Preserve existing relation name conversion logic + config('query-builder.convert_relation_names_to_snake_case_plural', true) + ? Str::plural(Str::snake($relation)) + : $relation, + ]; + + $strategy = config('query-builder.convert_relation_table_name_strategy', false); + + // Apply additional table name conversion based on strategy + if ($strategy === 'snake_case' && $tableName) { + $possibleRelatedNames[] = Str::snake($tableName); + } elseif ($strategy === 'camelCase' && $tableName) { + $possibleRelatedNames[] = Str::camel($tableName); + } elseif ($strategy === 'none') { + $possibleRelatedNames = $tableName; + } + + // Remove any null values + $possibleRelatedNames = array_filter($possibleRelatedNames); $fields = $this->request->fields() - ->mapWithKeys(fn ($fields, $table) => [$table => $fields]) - ->get($tableOrRelation); + ->mapWithKeys(fn ($fields, $table) => [$table => collect($fields)->map(fn ($field) => config('query-builder.convert_field_names_to_snake_case', false) ? Str::snake($field) : $field)]) + ->filter(fn ($value, $table) => in_array($table, $possibleRelatedNames)) + ->first(); if (! $fields) { return []; } - if (! $this->allowedFields instanceof Collection) { - // We have requested fields but no allowed fields (yet?) + $fields = $fields->toArray(); + + if ($tableName !== null) { + $fields = $this->prependFieldsWithTableName($fields, $tableName); + } + if (! $this->allowedFields instanceof Collection) { throw new UnknownIncludedFieldsQuery($fields); } diff --git a/src/Includes/IncludedRelationship.php b/src/Includes/IncludedRelationship.php index 59e39927..9aca7d8d 100644 --- a/src/Includes/IncludedRelationship.php +++ b/src/Includes/IncludedRelationship.php @@ -3,6 +3,7 @@ namespace Spatie\QueryBuilder\Includes; use Closure; +use Exception; use Illuminate\Database\Eloquent\Builder; use Illuminate\Support\Collection; @@ -16,11 +17,27 @@ public function __invoke(Builder $query, string $relationship) $relatedTables = collect(explode('.', $relationship)); $withs = $relatedTables - ->mapWithKeys(function ($table, $key) use ($relatedTables) { + ->mapWithKeys(function ($table, $key) use ($relatedTables, $query) { $fullRelationName = $relatedTables->slice(0, $key + 1)->implode('.'); if ($this->getRequestedFieldsForRelatedTable) { - $fields = ($this->getRequestedFieldsForRelatedTable)($fullRelationName); + + $tableName = null; + $strategy = config('query-builder.convert_relation_table_name_strategy', false); + + if ($strategy !== false) { + // Try to resolve the related model's table name + try { + // Use the current query's model to resolve the relationship + $relatedModel = $query->getModel()->{$fullRelationName}()->getRelated(); + $tableName = $relatedModel->getTable(); + } catch (Exception $e) { + // If we can not figure out the table don't do anything + $tableName = null; + } + } + + $fields = ($this->getRequestedFieldsForRelatedTable)($fullRelationName, $tableName); } if (empty($fields)) { diff --git a/tests/FieldsTest.php b/tests/FieldsTest.php index 0f46d822..a8148b0a 100644 --- a/tests/FieldsTest.php +++ b/tests/FieldsTest.php @@ -83,6 +83,21 @@ expect($query)->toEqual($expected); }); +it('can fetch specific string columns jsonApi Format', function () { + config(['query-builder.convert_field_names_to_snake_case' => true]); + config(['query-builder.convert_relation_table_name_strategy' => 'camelCase']); + + $query = createQueryFromFieldRequest('firstName,id') + ->allowedFields(['firstName', 'id']) + ->toSql(); + + $expected = TestModel::query() + ->select("{$this->modelTableName}.first_name", "{$this->modelTableName}.id") + ->toSql(); + + expect($query)->toEqual($expected); +}); + it('wont fetch a specific array column if its not allowed', function () { $query = createQueryFromFieldRequest(['test_models' => 'random-column'])->toSql(); @@ -222,6 +237,81 @@ $this->assertQueryLogContains('select `related_through_pivot_models`.`id`, `related_through_pivot_models`.`name`, `pivot_models`.`test_model_id` as `pivot_test_model_id`, `pivot_models`.`related_through_pivot_model_id` as `pivot_related_through_pivot_model_id` from `related_through_pivot_models` inner join `pivot_models` on `related_through_pivot_models`.`id` = `pivot_models`.`related_through_pivot_model_id` where `pivot_models`.`test_model_id` in ('); }); +it('can fetch only requested string columns from an included model jsonApi format', function () { + config(['query-builder.convert_relation_table_name_strategy' => 'camelCase']); + RelatedModel::create([ + 'test_model_id' => $this->model->id, + 'name' => 'related', + ]); + + $request = new Request([ + 'fields' => 'id,relatedModels.name', + 'include' => ['relatedModels'], + ]); + + $queryBuilder = QueryBuilder::for(TestModel::class, $request) + ->allowedFields('relatedModels.name', 'id') + ->allowedIncludes('relatedModels'); + + DB::enableQueryLog(); + + $queryBuilder->first()->relatedModels; + + $this->assertQueryLogContains('select `test_models`.`id` from `test_models`'); + $this->assertQueryLogContains('select `related_models`.`name` from `related_models`'); +}); + +it('can fetch only requested string columns from an included model jsonApi format with field conversion', function () { + config(['query-builder.convert_field_names_to_snake_case' => true]); + config(['query-builder.convert_relation_table_name_strategy' => 'camelCase']); + + RelatedModel::create([ + 'test_model_id' => $this->model->id, + 'name' => 'related', + ]); + + $request = new Request([ + 'fields' => 'id,relatedModels.fullName', + 'include' => ['relatedModels'], + ]); + + $queryBuilder = QueryBuilder::for(TestModel::class, $request) + ->allowedFields('relatedModels.fullName', 'id') + ->allowedIncludes('relatedModels'); + + DB::enableQueryLog(); + + $queryBuilder->first()->relatedModels; + + $this->assertQueryLogContains('select `test_models`.`id` from `test_models`'); + $this->assertQueryLogContains('select `related_models`.`full_name` from `related_models`'); +}); + +it('can fetch only requested string columns from an included model through pivot jsonApi format', function () { + config(['query-builder.convert_relation_table_name_strategy' => 'camelCase']); + + $this->model->relatedThroughPivotModels()->create([ + 'id' => $this->model->id + 1, + 'name' => 'Test', + ]); + + $request = new Request([ + 'fields' => 'id,relatedThroughPivotModels.name', + 'include' => ['relatedThroughPivotModels'], + ]); + + $queryBuilder = QueryBuilder::for(TestModel::class, $request) + ->allowedFields('relatedThroughPivotModels.name', 'id') + ->allowedIncludes('relatedThroughPivotModels'); + + DB::enableQueryLog(); + + $queryBuilder->first()->relatedThroughPivotModels; + + $this->assertQueryLogContains('select `test_models`.`id` from `test_models`'); + $this->assertQueryLogContains('select `related_through_pivot_models`.`name`, `pivot_models`.`test_model_id` as `pivot_test_model_id`, `pivot_models`.`related_through_pivot_model_id` as `pivot_related_through_pivot_model_id` from `related_through_pivot_models`'); +}); + it('can fetch requested array columns from included models up to two levels deep', function () { RelatedModel::create([ 'test_model_id' => $this->model->id, @@ -246,6 +336,36 @@ expect($result->relatedModels->first()->testModel->toArray())->toEqual(['id' => $this->model->id]); }); +it('can fetch requested array columns from included models up to two levels deep jsonApi mapper', function () { + config(['query-builder.convert_field_names_to_snake_case' => true]); + config(['query-builder.convert_relation_table_name_strategy' => 'camelCase']); + + $relatedModel = RelatedModel::create([ + 'test_model_id' => $this->model->id, + 'name' => 'related', + ]); + + $relatedModel->nestedRelatedModels()->create([ + 'name' => 'nested related', + ]); + + $request = new Request([ + 'fields' => 'id,name,relatedModels.id,relatedModels.name,nestedRelatedModels.id,nestedRelatedModels.name', + 'include' => ['nestedRelatedModels', 'relatedModels'], + ]); + + + $queryBuilder = QueryBuilder::for(TestModel::class, $request) + ->allowedFields('id', 'name', 'relatedModels.id', 'relatedModels.name', 'nestedRelatedModels.id', 'nestedRelatedModels.name') + ->allowedIncludes('relatedModels', 'nestedRelatedModels'); + + DB::enableQueryLog(); + $queryBuilder->first(); + + $this->assertQueryLogContains('select `test_models`.`id`, `test_models`.`name` from `test_models`'); + $this->assertQueryLogContains('select `nested_related_models`.`id`, `nested_related_models`.`name`, `related_models`.`test_model_id` as `laravel_through_key` from `nested_related_models`'); +}); + it('can fetch requested string columns from included models up to two levels deep', function () { RelatedModel::create([ 'test_model_id' => $this->model->id, diff --git a/tests/TestCase.php b/tests/TestCase.php index c8777655..591c22b6 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -37,6 +37,7 @@ protected function setUpDatabase(Application $app) $table->increments('id'); $table->timestamps(); $table->string('name')->nullable(); + $table->string('full_name')->nullable(); $table->double('salary')->nullable(); $table->boolean('is_visible')->default(true); }); @@ -62,6 +63,7 @@ protected function setUpDatabase(Application $app) $table->increments('id'); $table->integer('test_model_id'); $table->string('name'); + $table->string('full_name')->nullable(); }); $app['db']->connection()->getSchemaBuilder()->create('nested_related_models', function (Blueprint $table) { @@ -92,7 +94,7 @@ protected function setUpDatabase(Application $app) protected function getPackageProviders($app) { return [ - RayServiceProvider::class, + // RayServiceProvider::class, QueryBuilderServiceProvider::class, ]; } diff --git a/tests/TestClasses/Models/TestModel.php b/tests/TestClasses/Models/TestModel.php index 16797919..e182b028 100644 --- a/tests/TestClasses/Models/TestModel.php +++ b/tests/TestClasses/Models/TestModel.php @@ -8,6 +8,7 @@ use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\Relations\BelongsToMany; use Illuminate\Database\Eloquent\Relations\HasMany; +use Illuminate\Database\Eloquent\Relations\HasManyThrough; use Illuminate\Database\Eloquent\Relations\MorphMany; use Illuminate\Support\Carbon; @@ -27,6 +28,18 @@ public function relatedModel(): BelongsTo return $this->belongsTo(RelatedModel::class); } + public function nestedRelatedModels(): HasManyThrough + { + return $this->hasManyThrough( + NestedRelatedModel::class, // Target model + RelatedModel::class, // Intermediate model + 'test_model_id', // Foreign key on RelatedModel + 'related_model_id', // Foreign key on NestedRelatedModel + 'id', // Local key on TestModel + 'id' // Local key on RelatedModel + ); + } + public function otherRelatedModels(): HasMany { return $this->hasMany(RelatedModel::class); diff --git a/utils/run_tests.sh b/utils/run_tests.sh new file mode 100755 index 00000000..97d633bc --- /dev/null +++ b/utils/run_tests.sh @@ -0,0 +1,193 @@ +#!/bin/bash + +# Exit on any error +set -euo pipefail + +# Define constants +readonly PREFIX="laravel-query-builder" +readonly DEFAULT_PHP_VERSION="8.3" +readonly DEFAULT_LARAVEL_VERSION="11.*" + +# Function to display help message +show_help() { + echo "Usage: $0 [options]" + echo "" + echo "Options:" + echo " -h, --help Display this help message" + echo " -v, --version Display version information" + echo " -p PHP_VERSION Set the PHP version (default: ${DEFAULT_PHP_VERSION})" + echo " -l LARAVEL_VERSION Set the Laravel version (default: ${DEFAULT_LARAVEL_VERSION})" + echo " --filter FILTER Specify test filter(s)" + echo "" + echo "Example:" + echo " $0 --filter FieldsTest" +} + +# Parse command-line arguments +while [[ $# -gt 0 ]]; do + case "$1" in + -h|--help) + show_help + exit 0 + ;; + -v|--version) + echo "Version: 1.0" + exit 0 + ;; + -p|--php-version) + shift + PHP_VERSION="$1" + ;; + -l|--laravel-version) + shift + LARAVEL_VERSION="$1" + ;; + --filter) + shift + FILTER="$1" + ;; + *) + echo "Unknown option: $1" >&2 + exit 1 + ;; + esac + shift +done + +# Set default values if not provided +PHP_VERSION="${PHP_VERSION:-$DEFAULT_PHP_VERSION}" +LARAVEL_VERSION="${LARAVEL_VERSION:-$DEFAULT_LARAVEL_VERSION}" + +# Ensure we're in the project root +cd "$(dirname "$0")" + +# Create a custom Docker network +DOCKER_NETWORK_NAME="${PREFIX}-network" +docker network create "${DOCKER_NETWORK_NAME}" || true + +# Function to remove and recreate a container if it exists +recreate_container() { + local container_name="$1" + + # Remove the container if it exists (forcefully) + if docker ps -a --format '{{.Names}}' | grep -q "^${container_name}$"; then + echo "Removing existing container: ${container_name}" + docker rm -f "${container_name}" + fi +} + +# Prepare container names with prefix +MYSQL_CONTAINER_NAME="${PREFIX}-mysql" +REDIS_CONTAINER_NAME="${PREFIX}-redis" +TEST_RUNNER_IMAGE_NAME="${PREFIX}-test-runner" +TEST_CONTAINER_NAME="${PREFIX}-test-runner-container" + +# Recreate containers +recreate_container "${MYSQL_CONTAINER_NAME}" +recreate_container "${REDIS_CONTAINER_NAME}" +recreate_container "${TEST_CONTAINER_NAME}" + +# Set project root (parent of script directory) +PROJECT_ROOT="$(dirname "$(pwd)")" + +# Build the Docker image +docker build -t "${TEST_RUNNER_IMAGE_NAME}" -f - "$PROJECT_ROOT" </dev/null; then + echo "MySQL is ready!" + break + fi + sleep 4 + tries=$((tries+1)) +done + +if [ $tries -eq $max_tries ]; then + echo "MySQL did not become ready in time" + exit 1 +fi + +# Run tests in Docker +docker run --rm \ + --name "${TEST_CONTAINER_NAME}" \ + --network "${DOCKER_NETWORK_NAME}" \ + -e DB_HOST="${MYSQL_CONTAINER_NAME}" \ + -e DB_PORT=3306 \ + -e DB_USERNAME=user \ + -e DB_PASSWORD=secret \ + -e REDIS_HOST="${REDIS_CONTAINER_NAME}" \ + -e REDIS_PORT=6379 \ + "${TEST_RUNNER_IMAGE_NAME}" \ + vendor/bin/pest ${FILTER:+--filter "$FILTER"} + +# Cleanup containers +docker stop "${MYSQL_CONTAINER_NAME}" "${REDIS_CONTAINER_NAME}" "${TEST_CONTAINER_NAME}" +docker rm "${MYSQL_CONTAINER_NAME}" "${REDIS_CONTAINER_NAME}" "${TEST_CONTAINER_NAME}" +docker network rm "${DOCKER_NETWORK_NAME}" + +echo "Tests completed successfully!" \ No newline at end of file