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

[Laravel] POST/PUT/PATCH don't work for embedded relations #6882

Open
llei4 opened this issue Dec 20, 2024 · 7 comments
Open

[Laravel] POST/PUT/PATCH don't work for embedded relations #6882

llei4 opened this issue Dec 20, 2024 · 7 comments
Labels

Comments

@llei4
Copy link

llei4 commented Dec 20, 2024

API Platform version(s) affected: 4.0.12

Description

When trying to create, replace or update an embbeded relation using POST, PUT or PATCH Operations, that embedded relation is not modified.

As far as I know, using denormalization we should be able to create, replace or update and embbeded relation using its fields instead of IRI.

How to reproduce
Having a couple of simple Models, Father and Son, defined as following:

<?php

namespace App\Models;

use ApiPlatform\Metadata\ApiProperty;
use ApiPlatform\Metadata\ApiResource;
use Illuminate\Database\Eloquent\Model;
use Symfony\Component\Serializer\Annotation\Groups;
use Illuminate\Database\Eloquent\Relations\HasMany;
use Illuminate\Database\Eloquent\Collection;

#[ApiResource(
    shortName: 'Father',
    normalizationContext: ['groups' => ['father:read']],
    denormalizationContext: ['groups' => ['father:write']],
)]
class Father extends Model
{
    protected $table = 'fathers';
    protected $primaryKey = 'id_father';
    protected $fillable = ['name'];

    #[ApiProperty(writable: true)]
    #[Groups(['father:read','father:write','son:read','son:write'])]
    private ?string $name = null;

    #[Groups(['father:read','father:write','son:write'])]
    private ?Collection $sons = null;

    /**
     * Sons
     *
     * @return HasMany
     */
    public function sons(): HasMany
    {
        return $this->hasMany(Son::class, 'father_id', 'id_father');
    }
}

and

<?php

namespace App\Models;

use ApiPlatform\Metadata\ApiProperty;
use ApiPlatform\Metadata\ApiResource;
use Illuminate\Database\Eloquent\Model;
use Symfony\Component\Serializer\Annotation\Groups;
use Illuminate\Database\Eloquent\Relations\BelongsTo;

#[ApiResource(
    shortName: 'Son',
    normalizationContext: ['groups' => ['son:read']],
    denormalizationContext: ['groups' => ['son:write']],
)]
class Son extends Model
{
    protected $table = 'sons';
    protected $primaryKey = 'id_son';
    protected $fillable = ['name'];

    #[ApiProperty(writable: true)]
    #[Groups(['son:read','son:write','father:read','father:write'])]
    private ?string $name = null;

    #[Groups(['son:read','son:write','father:write'])]
    private ?Father $father = null;

    /**
     * Father
     *
     * @return BelongsTo
     */
    public function father(): BelongsTo
    {
        return $this->belongsTo(Father::class, 'father_id', 'id_father');
    }

}

Created using following migrations:

<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class extends Migration
{
    /**
     * Run the migrations.
     */
    public function up(): void
    {
        Schema::create('fathers', function (Blueprint $table) {
            $table->increments('id_father');
            $table->string('name');
            $table->timestamps();
        });
    }

    /**
     * Reverse the migrations.
     */
    public function down(): void
    {
        Schema::dropIfExists('fathers');
    }
};

and

<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class extends Migration
{
    /**
     * Run the migrations.
     */
    public function up(): void
    {
        Schema::create('sons', function (Blueprint $table) {
            $table->increments('id_son');
            $table->string('name');
            $table->unsignedInteger('father_id')->nullable();
            $table->timestamps();
            $table->foreign('father_id')->references('id_father')->on('fathers');

        });

    }

    /**
     * Reverse the migrations.
     */
    public function down(): void
    {
        Schema::dropIfExists('sons');
    }
};

Examples:

FATHER

  • I would like to create a father with 2 children.

Using POST on Father:

{
  "name": "new father no1", // new `Father`
  "sons": [
    {
      "name": "new son n1" // new `Son`
    },
    {
      "name": "new son no2" // new `Son`
    }
  ]
}

It is supposed to create 1 new Father and 2 new Son but i returns:

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'sons' in 'field list'

The same if instead of creating 2 new Son I just create a new father and relate it with 2 existing Son:

{
  "name": "new father no1", // new `Father`
  "sons": [
    {
      "name": "son n1" // existing `Son`
    },
    {
      "name": "son no2" // existing `Son`
    }
  ]
}

The same using IRI:

{
  "name": "new father no1", // new `Father`
  "sons": [
    "/api/sons/3", // existing `Son`
    "/api/sons/4" // existing `Son`
  ]
}

SON

  • I would like to create a new Son with a new Father.

Using POST on Son

{
  "name": "new son", // new `Son`
  "father": {
    "name": "new father" // new `Father`
  }
}

It is supposed to create 1 new Son and a new Father but a new Son with no Father is created.

Creating a new Son related to an existing Father using IRI:

{
  "name": "new son", // new `Son`
  "father": "/api/fathers/2"  // exsiting `Father`
}

It actually creates the new Son with the proper existing Father

  • I would like to update Father's name from Son.

Using PATCH on Son with id = 1:

{
  "name": "changing new of son 1", // exsting `Son`
  "father": {
    "@id": "/api/fathers/2", // related `Father`
    "name": "new fathres name"
  }
}

The father's name does not change.

I didn't add all the possibilities using POST, PATCH and PUT but I think the problem can be understood with the previous examples.

Possible Solution
On a Symfonycasts course API Platform 3 Part 1: Mythically Good RESTful APIs I've seen something called cascade: ['persist'] here and here not sure how to reproduce it on Laravel if it is needed.

Also, I am not sure if some kind of additional denormalization is needed neither because I didn't see anything related on Documentation, although I am aware Larave'ls one still need some improvements. In that case, could someone explain how to do it?

Thank you

@toitzi
Copy link
Contributor

toitzi commented Jan 8, 2025

Stumbeling upon the same issue, which unfortunatley is a bigger one to us. In anoterh issue here it is mentioned that this is possible as described here but that is not for laravel afaik, at least i have not got it working yet.

@toitzi
Copy link
Contributor

toitzi commented Jan 8, 2025

This seems a bit clunky, since in laravel you usually do not write your model attributes as properties, hard to tell what is the intended way here.

@soyuka would love to here your input on how all of that.

EDIT: Thinking about it, this might also add some other problems and rabbit holes like the security arround all of that...

EDIT2: Removed my confusing examples, already explained well enough above.

@llei4
Copy link
Author

llei4 commented Jan 9, 2025

Happy to know I am not the only one facing this issue @toitzi so it's not my bad :)

Current Situation

After digging into the code a little bit, IMO the key is the PersistProcessor:

Relations

PersistProcessor only processes BelongsTo Relations so any other relation is processed as it was an attribute of that Model, that's why when trying to save a HasMany Relation "Unknown column in 'field list'" DB's error is returned.

Persist Related Models

At the same time, the only action performed is associate an existing Model so it is not creating a new Model on POST operations (maybe PUT operations should also?) neither no change on that associated Model on PATCH operation is persisted as I guess cascade: ['persist'] on Symfony does.

Suggestions

Relations

PersistProcessor should also process other Relations:

  • HasMany:
    • "parent" should use save() on its relation in case the related Model should be created
    • "parent" should be created first and then each "son" should be related using its associate action
  • ManyToMany:
    • using its attach action
  • etc.

Persist Related Models

Operations should persist related Models:

  • POST:

    • Should relate existing Models if IRI is used
    • Should create new related Models if array is used
  • PATCH:

    • Should Modify existing related model if @id with IRI is used
    • Should create new related Models if no @id is used
  • PUT:

    • I do have doubts here. PUT is now replacing the Model data: should also replace related Model's data? What if it is a BelongsTo Relation which might affect other Models which is related to?

Do you agree @toitzi ?

What are your thoughts @soyuka?

@soyuka
Copy link
Member

soyuka commented Jan 9, 2025

Writing embeded relations has always raised many issues we don't recommend this at all. Instead you should work on the resource itself. For example:

POST /father/1/sons to create a Son belonging to /father/1 or POST on /sons with {"father": "/father/1"}.

Same goes for patching etc. Note that by not doing so, performances are horrible. Also, having an embed collection you won't be able to control how it is paginated, therefore we recommend to use URIs for your collection relations:

{"@id": /fathers/1", "sons": "/fathers/1/sons"}

This solves all issues, more on the subject at https://edge-side-api.rocks/.


Now as it may be convenient (especially for graphql I guess), I'm okay to merge patches to the PersistProcessor to support the cascade feature we have inside Doctrine (although I strongly discourage its usage). Please add some tests when contributing such a feature.

@llei4
Copy link
Author

llei4 commented Jan 9, 2025

Thanks for the answer @soyuka.

So if I understood correctly that means denormalization as it is explained on the documenctation doesn't apply on Laravel so we should manage the relations not altogether but the resources itself independently, right?

I understand the headeache with relations but that means we will need at least 1 call for each relation, so if we want to create 1 Model that has 5 relations then 6 calls will be needed (1 for the Model itself and 1 for each relation in case we need to create those Models also).

Not sure what is better in terms of performance, plus the complexity of managing errors and recalls if some of those relations fail.

Is there no other alternative?

@soyuka soyuka added bug and removed bug labels Jan 10, 2025
@soyuka
Copy link
Member

soyuka commented Jan 10, 2025

I thought that denormalization was working indeed, at least it should. I need to investigate this then, I thought only the Persistor was an issue here that should be fixable!

@soyuka soyuka added the bug label Jan 10, 2025
@llei4
Copy link
Author

llei4 commented Jan 14, 2025

Thank you so much @soyuka , let me know if there is anything I can help on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants