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

Specify identifying columns on nested mutation upserts #2426

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
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
10 changes: 7 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ You can find and compare releases at the [GitHub release page](https://github.co

## Unreleased

### Added

- Add support for identifyingColumns on upserts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Add support for identifyingColumns on upserts
- Specify identifying columns on nested mutation upserts with `@upsert(identifyingColumns: ["foo", "bar"])` https://github.com/nuwave/lighthouse/pull/2426


## v6.15.0

### Added
Expand Down Expand Up @@ -524,7 +528,7 @@ You can find and compare releases at the [GitHub release page](https://github.co

### Fixed

- Distinguish between client-safe and non-client-safe errors in `TestResponse::assertGraphQLError()`
- Distinguish between client-safe and non-client-safe errors in `TestResponse::assertGraphQLError()`

## v5.46.0

Expand Down Expand Up @@ -831,7 +835,7 @@ You can find and compare releases at the [GitHub release page](https://github.co

### Fixed

- Avoid PHP 8.1 deprecation warning by implementing `__serialize()` and `__unserialize()` https://github.com/nuwave/lighthouse/pull/1987
- Avoid PHP 8.1 deprecation warning by implementing `__serialize()` and `__unserialize()` https://github.com/nuwave/lighthouse/pull/1987

### Deprecated

Expand Down Expand Up @@ -1107,7 +1111,7 @@ You can find and compare releases at the [GitHub release page](https://github.co

### Changed

- Improve performance through [`graphql-php` lazy field definitions](https://github.com/webonyx/graphql-php/pull/861) https://github.com/nuwave/lighthouse/pull/1851
- Improve performance through [`graphql-php` lazy field definitions](https://github.com/webonyx/graphql-php/pull/861) https://github.com/nuwave/lighthouse/pull/1851
- Load individual subscription fields lazily instead of loading them all eagerly https://github.com/nuwave/lighthouse/pull/1851
- Require `webonyx/graphql-php:^14.7` https://github.com/nuwave/lighthouse/pull/1851

Expand Down
35 changes: 29 additions & 6 deletions src/Execution/Arguments/UpsertModel.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,14 @@ class UpsertModel implements ArgResolver
/** @var callable|\Nuwave\Lighthouse\Support\Contracts\ArgResolver */
protected $previous;

/** @var array<string> */
protected array $identifyingColumns;
Comment on lines +12 to +13
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/** @var array<string> */
protected array $identifyingColumns;
/** @var array<string>|null */
protected ?array $identifyingColumns;


/** @param callable|\Nuwave\Lighthouse\Support\Contracts\ArgResolver $previous */
public function __construct(callable $previous)
public function __construct(callable $previous, ?array $identifyingColumns)
{
$this->previous = $previous;
$this->identifyingColumns = $identifyingColumns ?? [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$this->identifyingColumns = $identifyingColumns ?? [];
$this->identifyingColumns = $identifyingColumns;

}

/**
Expand All @@ -22,20 +26,39 @@ public function __construct(callable $previous)
public function __invoke($model, $args): mixed
{
// TODO consider Laravel native ->upsert(), available from 8.10
spawnia marked this conversation as resolved.
Show resolved Hide resolved
$id = $args->arguments['id']
?? $args->arguments[$model->getKeyName()]
?? null;
$existingModel = null;

if ($id !== null) {
if (! empty($this->identifyingColumns)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (! empty($this->identifyingColumns)) {
if (isset($this->identifyingColumns)) {

The directive should implement manipulateArgDefinition and manipulateFieldDefinition to validate the given array is not empty and throw a DefinitionException.

$existingModel = $model
->newQuery()
->find($id->value);
->firstWhere(
array_intersect_key(
$args->toArray(),
array_flip($this->identifyingColumns),
),
);

if ($existingModel !== null) {
$model = $existingModel;
}
}

if ($existingModel === null) {
$id = $args->arguments['id']
?? $args->arguments[$model->getKeyName()]
?? null;

if ($id !== null) {
$existingModel = $model
->newQuery()
->find($id->value);

if ($existingModel !== null) {
$model = $existingModel;
}
}
}

return ($this->previous)($model, $args);
}
}
11 changes: 10 additions & 1 deletion src/Schema/Directives/UpsertDirective.php
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update the directive definition in docs/master/api-reference/directives.md with the final definition.

Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ public static function definition(): string
"""
model: String

"""
Specify the columns by which to upsert the model.
This is optional, defaults to the ID or model Key.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This is optional, defaults to the ID or model Key.
Optional, by default `id` or the primary key of the model are used.

It is not like this argument itself defaults to those values, the mechanics behind it behave that way.

"""
identifyingColumns: [String!] = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
identifyingColumns: [String!] = []
identifyingColumns: [String!]


"""
Specify the name of the relation on the parent model.
This is only needed when using this directive as a nested arg
Expand All @@ -33,6 +39,9 @@ public static function definition(): string

protected function makeExecutionFunction(Relation $parentRelation = null): callable
{
return new UpsertModel(new SaveModel($parentRelation));
return new UpsertModel(
new SaveModel($parentRelation),
$this->directiveArgValue('identifyingColumns'),
);
}
}
145 changes: 145 additions & 0 deletions tests/Integration/Schema/Directives/UpsertDirectiveTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Illuminate\Container\Container;
use Nuwave\Lighthouse\Schema\TypeRegistry;
use Tests\DBTestCase;
use Tests\Utils\Models\Company;
use Tests\Utils\Models\Task;
use Tests\Utils\Models\User;

Expand Down Expand Up @@ -198,6 +199,150 @@ interface IUser
]);
}

public function testDirectUpsertByIdentifyingColumn(): void
{
$this->schema .= /** @lang GraphQL */ '
type User {
id: ID!
email: String!
name: String!
}

type Mutation {
upsertUser(name: String!, email: String!): User @upsert(identifyingColumns: ["email"])
}
';

$this->graphQL(/** @lang GraphQL */ '
mutation {
upsertUser(
email: "[email protected]"
name: "bar"
) {
name
email
}
}
')->assertJson([
'data' => [
'upsertUser' => [
'email' => '[email protected]',
'name' => 'bar',
],
],
]);

$user = User::firstOrFail();

$this->assertSame('bar', $user->name);
$this->assertSame('[email protected]', $user->email);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use variables to show the connection between the passed values.


$this->graphQL(/** @lang GraphQL */ '
mutation {
upsertUser(
email: "[email protected]"
name: "foo"
) {
name
email
}
}
')->assertJson([
'data' => [
'upsertUser' => [
'email' => '[email protected]',
'name' => 'foo',
],
],
]);

$user->refresh();

$this->assertSame('foo', $user->name);
$this->assertSame('[email protected]', $user->email);
}

public function testDirectUpsertByIdentifyingColumns(): void
{
$company = factory(Company::class)->create(['id' => 1]);
Copy link
Collaborator

Choose a reason for hiding this comment

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


$this->schema
/** @lang GraphQL */
.= '
Comment on lines +269 to +271
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$this->schema
/** @lang GraphQL */
.= '
$this->schema .= /** @lang GraphQL */ '

type User {
id: ID!
email: String!
name: String!
company_id: ID!
Copy link
Collaborator

Choose a reason for hiding this comment

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

No raw foreign keys in GraphQL, not even as a test case. Often, those test cases serve as an example, so I don't want them to implement anti-patterns.

}

type Mutation {
upsertUser(name: String!, email: String!, company_id:ID!): User @upsert(identifyingColumns: ["name", "company_id"])
}
';

$this->graphQL(
/** @lang GraphQL */
'
Comment on lines +284 to +286
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$this->graphQL(
/** @lang GraphQL */
'
$this->graphQL(/** @lang GraphQL */ '

mutation {
upsertUser(
email: "[email protected]"
name: "bar"
company_id: 1
) {
name
email
company_id
}
}
',
)->assertJson([
'data' => [
'upsertUser' => [
'email' => '[email protected]',
'name' => 'bar',
'company_id' => 1,
],
],
]);

$user = User::firstOrFail();

$this->assertSame('bar', $user->name);
$this->assertSame('[email protected]', $user->email);
$this->assertSame(1, $user->company_id);

$this->graphQL(
/** @lang GraphQL */
'
Comment on lines +315 to +317
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$this->graphQL(
/** @lang GraphQL */
'
$this->graphQL(/** @lang GraphQL */ '

mutation {
upsertUser(
email: "[email protected]"
name: "bar"
company_id: 1
) {
name
email
company_id
}
}
',
)->assertJson([
'data' => [
'upsertUser' => [
'email' => '[email protected]',
'name' => 'bar',
'company_id' => $company->id,
],
],
]);

$user->refresh();

$this->assertSame('bar', $user->name);
$this->assertSame('[email protected]', $user->email);
}

public static function resolveType(): Type
{
$typeRegistry = Container::getInstance()->make(TypeRegistry::class);
Expand Down