lighthouse
lighthouse copied to clipboard
nuwave
Allow @updateOrCreate (and @upsert to find models by columns other than id?)
I'm not super familiar with Lighthouse's internals, but I'm definitely willing to help if pointed in the right direction.
What problem does this feature proposal attempt to solve?
It looks like Lighthouse doesn't currently support the native Model::updateOrCreate() and Model::upsert() methods. See here for docs and here for the current @upsert implementation.
As such, there is currently no way to upserting a model by querying columns other than the model's primary key. So for example, you can't upsert a user by email unless email is the primary column. This also applies to nested mutations.
This feature would solve that use case.
Which possible solutions should be considered?
I think something like this could work:
extend type Mutation {
updateOrCreateUser {
id: ID
email: String
newEmail: String @rename("email")
name: String
} @updateOrCreate(find: ["id", "email"])
Or this?
extend type Mutation {
updateOrCreateUser {
id: ID @eq
email: String @eq
newEmail: String @rename("email")
name: String
} @updateOrCreate
Also, nested mutations could be considered:
input CreateUserBelongsTo {
connect: ID
create: CreateUserInput
update: UpdateUserInput
upsert: UpsertUserInput
updateOrCreate: UpdateOrCreateUserInput
}
From there, using the feature would be rather straightforward:
mutation {
updateOrCreateUser(
email: "[email protected]"
newEmail: "[email protected]"
name: "Updated Name"
) {
email # "[email protected]"
name # "Updated Name"
}
}
To be honest, I'm not sure what an @upsert implementation would look like since 1) we probably don't want to write the "find" columns for every model we'd like to upsert and 2) Laravel's implementation looks a bit different from the current implementation, so backwards compatibility might be an issue.
I'm far from familiar with Lighthouse's internals and only somewhat familiar with Laravel's internals, so this proposed API might not work, but at least we have a starting point.
Please reformulate in terms of a feature request and use the appropriate template, this is clearly not a bug.
My bad! Originally started as a bug report and forgot to switch it over. This has been fixed.
As hinted at in our implementation, upsert() is only available from Laravel 8.10 onwards.
To properly support it, we will have to enable passing the appropriate metadata through the directive. For this example from the Laravel docs:
Flight::upsert([
['departure' => 'Oakland', 'destination' => 'San Diego', 'price' => 99],
['departure' => 'Chicago', 'destination' => 'New York', 'price' => 150]
], ['departure', 'destination'], ['price']);
I would imagine the schema to look something like this:
type Mutation {
upsertFlight(
departure: String!
destination: String!
price: Int!
): Flight! @upsert(uniqueBy: ["departure", "destination"], update: ["price"])
}
For a nested mutation, we could allow @upsert on the nested input:
input UpdateFlightsHasMany {
upsert: [UpsertFlightInput!]
}
input UpsertFlightInput @upsert(uniqueBy: ["departure", "destination"], update: ["price"]) {
departure: String!
destination: String!
price: Int!
}
That would be a great feature as I simply need to do by Uuid (string) using where clause but the @create @update @upsert and @delete use find() and throws primary key not found issue. cause it does: User::find("b528ecb4-0d21-4518-ae33-f81008f3e0f3")
Didn't find a way around other than making php artisan lighthouse:mutation which is redundant code and could be reused well for simple mutations.
User::where('uuid', "b528ecb4-0d21-4518-ae33-f81008f3e0f3")->first() instead of User::find(1).
Looking forward if this is done at some point.
Also having a similar problem with uuid, as mentioned the directives are using findOrFail($id->value) instead of ->where('id', $id->value)->firstOrFail()
https://github.com/nuwave/lighthouse/blob/6d4b582460ee04a498e4c15f219e1fe9a99b9e52/src/Execution/Arguments/UpdateModel.php#L41
Should I make a PR?
@mbalandis @eNzyOfficial Why don't you define the columns uuid or id respectively as the primary key for the model in question? Why have multiple identifiers in the first place?
@spawnia
@mbalandis @eNzyOfficial Why don't you define the columns
uuidoridrespectively as the primary key for the model in question? Why have multiple identifiers in the first place?
id is used internally for simplicity and UUID is generated for external use such as API's and routes so it wouldn't appear that we have very few rows, to be secure, etc. There are also numerous business logics involved and/or inability to remove the id that are auto-incremented as primary key to be honest. I tend to or sometimes am forced to use id's rather than replace it which helps with compatibility, simplicity with 3rd party libs, etc. Some others have other reasons and use cases I guess.
Got it, I see how that could be necessary.
Something we need to consider to avoid breakage is that find() or findOrFail() do not care about the name of the attribute from GraphQL. For example, the GraphQL field could be called id, but the primary key column could be called prefix_id. That combination would currently work. Perhaps we could take @rename into account to solve that.
In terms of the schema definition, I see two ways going forward. One would be to add arguments to directives that use the primary key to find a model, such as @update and @delete. Another would be to mark the argument to use with @primaryKey and have the directives use that.
That would be a great feature. In my case I have tags associations, and it would be convenient to utilize the new Model::upsert() for upserting tags based on other value(s) not just its primary key so they don't become duplicated every time. UUID point is also very valid. I noticed that this library supports Laravel from version 5.7 so for this reason alone Model::upsert() could be out of a question. I don't know if this lib plans or supports conditional features based on Laravel version, if so we can attack this. In my case using the connect in mutation instead of upsert may be wise.
