Nested mutations on 1-to-1 relationships require relation's PK

[CPSibo]

Slack thread: https://lighthouse-php.slack.com/archives/CB28A070S/p1579310262008300

Describe the bug

When using a nested mutation, such as update, you're required to pass in the related model's PK, even if the relationship is 1-1.

mutation {
    updateUser(input: {
        id: 1
        username: "test"
        profile: {
            update: {
                user_id: 2 # This is the undocumented bit and I wouldn't expect to be allowed to use the wrong PK here.
                email: "test@test.com"
            }
        }
    }){
        id
        username
        profile {
            email
        }
    }
}

1. This seems undocumented, since no update example is shown in the docs for the 1-1 relationship types.
2. This allows users to create malformed mutations that, at best, trigger laravel or db exceptions. Not sure if it's a valid vector for screwing up the db, though.

Expected behavior/Solution

1. It should at least be documented in the 1-1 portion of the nested mutation docs that the related model's PK is needed. That the related model instance isn't using the model's relationship method.

2. It'd seem natural that 1-1 relationships shouldn't need to specify all PKs involved. If a user model only has one profile relationship, that shouldn't need to be manually specified by the client.

3. I'd expect users of the API to not be allowed to supply malformed mutations that seem to break laravel's normal relationship rules.

   I don't have a proof-of-concept that this could lead to operating on models that don't actually belong to the root model but I wouldn't be surprised if it did. For instance, what if a relationship has extra restrictions set in the model's method that are ignored by supplying incorrect PKs, which Lighthouse seems to be taking at face value?

If nothing else, it's a side-channel attack that leaks which PKs exist in the related model's table, which could compromise slug-based or multi-tenant systems.

Steps to reproduce

1. Create schema per https://lighthouse-php.com/master/eloquent/nested-mutations.html#belongs-to
2. Call a nested update with incorrect PKs

   mutation {
   updateUser(input: {
       id: 1
       username: "test"
       profile: {
           update: {
               user_id: 2 # This is the undocumented bit and I wouldn't expect to be allowed to use the wrong PK here.
               email: "test@test.com"
           }
       }
   }){
       id
       username
       profile {
           email
       }
   }
   }

Output/Logs

Click to expand

``` For PK clashes: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '4' for key 'PRIMARY' (SQL: update `profiles` set `user_id` = 4, `email` = test@test.com where `user_id` = 3) For PKs that don't exist: No query results for model [App\\Models\\Profile] 5 ```

**Environment** Lighthouse Version: 4.8.1 Laravel Version: 6.11.0

[spawnia]

I agree that what you are describing is seldomly the wanted behaviour. It might be used to attach and update a related model in one go. While i have not used it for such in our practice, i imagine some users might do that.

Another reason why it is done this way currently is lazyness: the input type for updates can be easily shared this way. Since id is required for updates, it is also needed in nested mutations.

We should lay out a plan to move away from this way of doing things, as the potential for confusion, unexpected behaviour or misuse is apparant, great job on writing it down in the description.

We will have to decide how we want to deal with the use case of attaching + updating a new related model. There are a few ways to go forward.

We could change update to only allow related models. I haven't had a need for attaching and updating different models yet, and since nested mutations are now extensible, users who really want it can add it back in. We could add something like attachAndUpdate if we want to offer this capability as part of Lighthouse core. This is a client-facing breaking change.

As a backwards-compatible option, we could a new kind of nested mutation such as updateRelated that would simply ignore the passed ID and update the data of the related model. This is a non-breaking addition. What i like about this way of doing it is that it keeps update consistent between one-to-one and one-to-many relationships. It simply adds a simpler special case operation for the common one-to-one relations.

[lorado]

How about to tweak update in such way, that for one-to-one relations, if id or primary key is not passed in input, model will be fetched from relation? Sounds tricky, but it would be still a solution without breaking change and with new feature.

[spawnia]

How about to tweak update in such way, that for one-to-one relations, if id or primary key is not passed in input, model will be fetched from relation? Sounds tricky, but it would be still a solution without breaking change and with new feature.

That seems fine, yeah.

[pkatuwal]

I also having same issue while updating relational model for morphOne relationship. My Input Configuration

input UpdateEntityInputFieldMorphOne{
    update: UpdateEntityInput!
}
input UpdateEntityInput{
    # id:ID!
    modified_by:Int!
    description:String!
    status:String!
}
input updateResourceInput{
    id: ID!
    name:String @rules(apply:["nullable","max:256","unique:resources,name"])
    "if hierarchical, specify it"
    parent_id:Int @rules(apply:["nullable","exists:resources,id"])
    "Url of calling method"
    url:String @rules(apply:["nullable"])
    "service id specify your url where is derived from"
    service_id:Int @rules(apply:["nullable","exists:services,id"])
    "http method which defined in HttpVerbs"
    http_verbs:HttpVerbs @eq 
    "limit of data transfer,todo"
    data_transfer_restrictions:Int
    entity: UpdateEntityInputFieldMorphOne @morphOne (relation:"entity")
}

extend type Mutation{
    "create resource with relational entities"
    createResource(
        input: createResourceInput! @spread
    ):Resource @create #it works

    "Update Resource Model"
    updateResource(
        input:updateResourceInput! @spread
    ):Resource @update #it shows undefined index 'id'
}

Mutation

mutation {
  updateResource(
    input: {
      id: 31
      name: "pramod1"
      entity: { update: { modified_by: 2, description: "s", status: "er" } }
    }
  ) {
    id
    name
    parent_id
    url
    service_id
    http_verbs
    entity {
      created_by
      modified_by
      description
      status
    }
  }
}

Results

{
  "errors": [
    {
      "debugMessage": "Undefined index: id",
      "message": "Internal server error",
      "extensions": {
        "category": "internal"
      },
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "updateResource"
      ],
      "trace": [
        {
          "file": "/var/www/html/vendor/nuwave/lighthouse/src/Execution/Arguments/UpdateModel.php",

But adding id on relational arguments is working.

I tried to update in repo by following code

    $model=App\Models\Resource::find(31);
    $model->name='s';
    $model->entity->modified_by=10;
    $model->push();

and it works.

Please help me to find solution for this issue.

edit: And for now i add mutation class as

public function __invoke($rootValue, array $args, GraphQLContext $context, ResolveInfo $resolveInfo)
    {

        //primary key
        $primaryKey=$args['id'];

        //pull entity arguments
        $entityModificationObjects = \Illuminate\Support\Arr::pull($args, 'entity.update');

        //forget arguments that are not in used
        $this->eliminateKey($args);

        //fetch model name, else default is defined in .graphql Type
        $modelClass=$this->parseModelName($resolveInfo, $args);

        //find relational value
        $modelObject=$modelClass::find($primaryKey);

        // update main Model
        foreach ($args as $key => $value) {
            $modelObject->$key=$value;
        }

        //push to relational model
        foreach ($entityModificationObjects as $key => $value) {
            $modelObject->entity->$key=$value;
        }
        $modelObject->push();
        return $modelObject;
    }

    /**
     * remove entity keys
     */
    private function eliminateKey(&$args):void
    {
        \Illuminate\Support\Arr::forget($args, ['entity','directive','model_name','id']);
    }

    /**
     * Parse Model Class Name
     * else take argument as modal name
     */
    private function parseModelName(ResolveInfo $resolveInfo, array $args)
    {
        $modelName=($resolveInfo->returnType->config['name'])??$args['model_name'];
        return "App\Models\\$modelName";
    }

Above mutation class is generic for similar relation having same entity table as one to one model which i used to solve this issue temporarily.