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

Conversation

GavG
Copy link

@GavG GavG commented Jul 28, 2023

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

Adds optional identifyingColumns arg to upsert directive

Breaking changes

None

@spawnia spawnia changed the title Upsert fields directive Specify identifying columns on nested mutation upserts Aug 8, 2023
Copy link
Collaborator

@spawnia spawnia left a comment

Choose a reason for hiding this comment

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

Implementation and test make sense so far, after some tweaks I am fine with adding this.

Specify the columns by which to upsert the model.
This is optional, defaults to the ID or model Key.
"""
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!]

@@ -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

Comment on lines +12 to +13
/** @var array<string> */
protected array $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
/** @var array<string> */
protected array $identifyingColumns;
/** @var array<string>|null */
protected ?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;

src/Execution/Arguments/UpsertModel.php Show resolved Hide resolved
Comment on lines +269 to +271
$this->schema
/** @lang GraphQL */
.= '
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 */ '

Comment on lines +284 to +286
$this->graphQL(
/** @lang GraphQL */
'
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 */ '

Comment on lines +315 to +317
$this->graphQL(
/** @lang GraphQL */
'
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 */ '


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.

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.

@spawnia spawnia added the enhancement A feature or improvement label Aug 8, 2023
@GavG
Copy link
Author

GavG commented Aug 26, 2023

Thanks for the review @spawnia. Sorry I've been a bit quiet on this, recently had our first child so haven't had any free time 🙃

I've been giving the solution a bit more thought, I think supporting nested relationship upsert logic is a must, so I'll try and tackle that when I come to making the suggest changes.

I've also been considering my own use case again, where more complex upsert logic was required, and I think a cleaner solution to this might be to permit the specifying of a custom query class that would receive the input params in place of the package's query logic. I may have a go at this in a new fork later.

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

Successfully merging this pull request may close these issues.

3 participants