tobyzerner / json-api-server

A JSON:API server implementation in PHP.
https://tobyzerner.github.io/json-api-server/
MIT License
63 stars 21 forks source link

How to remove ToOne relationship? #74

Closed bertramakers closed 11 months ago

bertramakers commented 11 months ago

I'm trying to remove a ToOne relationship from a resource by sending a PATCH request to the resource with the ToOne relationship like this:

PATCH /api/v2/clusters/17

{
    "data": {
        "type": "clusters",
        "id": "17",
        "relationships": {
            "teamLead": {
                "data": null
            }
        }
    }
}

This results in a TypeError with the following message: obyz\JsonApiServer\Schema\Field\Relationship::findResourceForIdentifier(): Argument #1 ($identifier) must be of type array, null given, called in /var/www/vendor/tobyz/json-api-server/src/Schema/Field/ToOne.php on line 59

Is this a bug, or should I remove the ToOne relationship in a different way?

bertramakers commented 11 months ago

Looking more closely at the source code of the ToOne relationship, I learned I can make it work by calling ->nullable() on the field.

However, this now also includes the relationship with its null value every time I fetch the resource, which is not what I necessarily wanted.

I think that setting the relationship to null to remove it, and actually including the null value in the response are two separate contexts.

In the case of removing the relationship, it would probably be better to check if it's required() or not vs nullable().

tobyzerner commented 11 months ago

The fact that it results in a TypeError is a bug – it should result in a nicer validation error. I'll look into a fix.

nullable() means "this field can have a null value", whereas required() means "this field MUST be present when creating a new resource". So if I'm understanding correctly, your relationship should indeed be marked as nullable.

By default, ToOne relationships have resource linkage enabled, meaning that the data member for the relationship will be present in the response (whether it is null or not). If you want to disable resource linkage, use withoutLinkage().

bertramakers commented 11 months ago

I'm not sure I completely understand resource linkage, but my issue with nullable is this:

  1. When I create a new resource of a type that has an optional ToOne relationship, and I don't specify a value for that relationship, the resource that is returned afterwards does not contain the ToOne relationship at all (not even with {"data": null})
  2. When I update the resource to set a value for the relationship, the returned resource then obviously contains a value for the ToOne relationship
  3. When I update the resource to remove the value of the relationship (only possible if nullable()), the returned resource now contains a literal null value for the relationship. While when it was initially created, it didn't have any value for this relationship, not even null

This is confusing for our frontend developers because in some cases when a resource does not have a value for the relationship, the relationship will not be present at all, while in others it will be included and set to null (depending on whether the relationship was set before but removed, or never set at all)

tobyzerner commented 11 months ago

The behaviour in your first point seems odd - as long as linkage is enabled on the relationship (as it is by default), it should be present in the response, null or not. Will have a look into it.

bertramakers commented 11 months ago

I double-checked and it was a mistake on my end 🤦‍♂️ Because I was testing how to remove the relationship at the time, I accidentally tested the first point without the nullable() on the relationship, and then I added it to make it possible to remove the relationship and obviously it would become null in the third step. Since the behavior is now consistent (always null if it's not set), this is okay for me. Sorry for wasting your time on this one! 😅

bertramakers commented 11 months ago

Ah I'll reopen the issue for the TypeError when you try to remove a relationship that is not nullable().