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

API platform 3.2 feedbacks #5793

Closed
soyuka opened this issue Sep 2, 2023 · 21 comments
Closed

API platform 3.2 feedbacks #5793

soyuka opened this issue Sep 2, 2023 · 21 comments

Comments

@soyuka
Copy link
Member

soyuka commented Sep 2, 2023

Use this issue to send feedbacks on the next-gen API Platforms version.
Thanks for the support!

@paullallier
Copy link
Contributor

When I run composer require api-platform/core v3.2.0-alpha.1 -W. I get the following:

Class ApiPlatform\Tests\Util\ArrayTraitTest located in ./vendor/api-platform/core/src/GraphQl/Tests/Util/ArrayTraitTest.php does not comply with psr-4 autoloading standard. Skipping.

I think it should be namespace ApiPlatform\GraphQl\Tests\Util; rather than namespace ApiPlatform\Tests\Util; (I don't think there's a branch I can PR that to?)

And later, more critically:

Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 1
!!  
!!  In DecoratorServicePass.php line 89:
!!                                                                                 
!!    The service "api_platform.graphql.state_provider.validate" has a dependency  
!!     on a non-existent service "api_platform.graphql.state_provider".            
!!                                                                                 
!!  
!!  
Script @auto-scripts was called via post-update-cmd

I might be missing a config file change, but I didn't see anything mentioned in the notes. I'm not intentionally using GraphQL in my project.

@soyuka
Copy link
Member Author

soyuka commented Sep 5, 2023

Thanks @paullallier I just fixed these on dev-main and will tag in a few days.

@paullallier
Copy link
Contributor

paullallier commented Sep 5, 2023

I grabbed dev-main to have another quick look. I'm seeing an issue with the Swagger docs, but that might be something specific to the (probably) wrong why I'm defining my entity relations.

Specifically, it doesn't seem happy if the parameter defaults to null (or possibly just if it's nullable)

    #[ORM\Column(length: 128, nullable: true)]
    #[Groups(['invoice_in:read_item', 'invoice_in:read_collection', 'invoice_in:write_update'])]
    protected ?string $description = null;

In dev-main (f02f379):
Screenshot 2023-09-05 at 14 05 11

In v3.1.16:
Screenshot 2023-09-05 at 14 05 36

Note, "items" is a ?ArrayCollection of InvoiceInItem Enities, but the way I've defined it is probably non-standard. It was pulling the structure inside an InvoiceInItem into the InvoiceIn schema correctly before, however.

@soyuka
Copy link
Member Author

soyuka commented Sep 8, 2023

Interesting isn't that the case in 3.1 though? you'd need to check if the generated schema is correct I don't think we changed this part of the code.

https://github.com/api-platform/core/releases/tag/v3.2.0-alpha.2 released, note that we have a few issues to fix:

  • the api_platform.state_provider isn't backward compatible yet I need to add an api_platform.state_provider.main instead.
  • I want to merge feat: deprecate json support + add openapi format #5632 because I have a bug to fix when swagger is disabled we force the html response and it's kinda bad

Apart from that I think that the next features will reach 3.3 (in 6 months).

@paullallier
Copy link
Contributor

paullallier commented Sep 8, 2023

The schema for that request body in v3.2.0-beta2 is:

  "InvoicesIn.jsonld-generic_financial_entity.write_update": {
    "type": "object",
    "description": "",
    "deprecated": false,
    "properties": {
      "purchaseOrder": {
        "type": [
          "string",
          "null"
        ]
      },
      "date": {
        "type": "string",
        "format": "date-time"
      },
      "contact": {
        "type": [
          "integer",
          "null"
        ]
      },
      "site": {
        "type": [
          "integer",
          "null"
        ]
      },
      "description": {
        "type": [
          "string",
          "null"
        ]
      },
      "number": {
        "type": "string"
      },
      "items": {
        "type": "array",
        "items": {
          "type": "string"
        }
      }
    }
  }

and in v3.1.17 I get:

  "InvoicesIn.jsonld-generic_financial_entity.write_update": {
    "type": "object",
    "description": "",
    "deprecated": false,
    "properties": {
      "purchaseOrder": {
        "type": "string",
        "nullable": true
      },
      "date": {
        "type": "string",
        "format": "date-time"
      },
      "contact": {
        "type": "integer",
        "nullable": true
      },
      "site": {
        "type": "integer",
        "nullable": true
      },
      "description": {
        "type": "string",
        "nullable": true
      },
      "number": {
        "type": "string"
      },
      "items": {
        "type": "array",
        "items": {
          "$ref": "#/components/schemas/InvoiceInItem.jsonld-generic_financial_entity.write_update"
        }
      }
    }
  }

The null issue might be related to this, if we have the affected version of SwaggerUI in 3.2 and not in 3.1? swagger-api/swagger-ui#9056

But I'm not sure why I'm losing the definition of the items array in 3.2, or why the nullable string types have changes from
{"type": "string", "nullable": true} to "{type": ["string", "null"]}other than potentially because my code is non-idiomatic.

@paullallier
Copy link
Contributor

paullallier commented Sep 8, 2023

I wanted to check if it was something in my code, so I created a new entity with maker bundle.

<?php

namespace App\Entity;

use ApiPlatform\Metadata\ApiResource;
use Doctrine\ORM\Mapping as ORM;

#[ORM\Entity]
#[ApiResource]
class TestEntity
{
    #[ORM\Id]
    #[ORM\GeneratedValue]
    #[ORM\Column]
    private ?int $id = null;

    #[ORM\Column(length: 255)]
    private ?string $notNullableString = null;

    #[ORM\Column(length: 255, nullable: true)]
    private ?string $nullableString = null;

    #[ORM\Column]
    private ?int $notNullableInt = null;

    #[ORM\Column(nullable: true)]
    private ?int $nullableInt = null;

    public function getId(): ?int
    {
        return $this->id;
    }

    public function getNotNullableString(): ?string
    {
        return $this->notNullableString;
    }

    public function setNotNullableString(string $notNullableString): static
    {
        $this->notNullableString = $notNullableString;

        return $this;
    }

    public function getNullableString(): ?string
    {
        return $this->nullableString;
    }

    public function setNullableString(?string $nullableString): static
    {
        $this->nullableString = $nullableString;

        return $this;
    }

    public function getNotNullableInt(): ?int
    {
        return $this->notNullableInt;
    }

    public function setNotNullableInt(int $NotNullableInt): static
    {
        $this->notNullableInt = $NotNullableInt;

        return $this;
    }

    public function getNullableInt(): ?int
    {
        return $this->nullableInt;
    }

    public function setNullableInt(?int $nullableInt): static
    {
        $this->nullableInt = $nullableInt;

        return $this;
    }
}

That one shows the same behaviour - looks correct in v3.1.17 and like this in 3.2.0-beta.2:
Screenshot 2023-09-08 at 22 08 20

@paullallier
Copy link
Contributor

On the plus side, if I skip all my assertMatchesResourceItemJsonSchema checks, all my test cases pass with v3.2.0-alpha.2. My code is heavy on custom StateProcessor/Provider logic and doesn't use Doctrine ORM for DB access.

@soyuka
Copy link
Member Author

soyuka commented Sep 11, 2023

/edit: actually I think that this is the prefered way with OpenAPI, maybe we should update Swagger UI ? The assertion needs a fix I guess?

@vincentchalamon
Copy link
Contributor

We're upgrading Swagger UI to its latest version, it fixes the Unknown Type: ..., null issue (cf. #5807).

About your other issues (#5793 (comment) and #5793 (comment)), can you give us a reproducer please?

@paullallier
Copy link
Contributor

It looks like the schema assertion fails because the relation isn't showing the fields inside each list item (possibly it's saying that the endpoint will just return the IRI, but my project is definitely returning more than that - tests rely on it). I've got an entity which includes a nullable field but no relations and that one passes the schema assertion.

If I create this entities:

<?php

namespace App\Entity;

use ApiPlatform\Metadata\ApiResource;
use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Serializer\Annotation\Groups;

#[ORM\Entity]
#[ApiResource]
class TestEntity
{
    #[ORM\Id]
    #[ORM\GeneratedValue]
    #[ORM\Column]
    #[Groups(['read', 'write'])]
    private ?int $id = null;
    
    #[ORM\Column(length: 255, nullable: true)]
    #[Groups(['read', 'write'])]
    private ?string $nullableString = null;

    #[ORM\Column(nullable: true)]
    #[Groups(['read', 'write'])]
    private ?int $nullableInt = null;

    #[ORM\ManyToOne(inversedBy: 'tests')]
    private ?BagOfTests $bagOfTests = null;

    public function getId(): ?int
    {
        return $this->id;
    }

    public function getNullableString(): ?string
    {
        return $this->nullableString;
    }

    public function setNullableString(?string $nullableString): static
    {
        $this->nullableString = $nullableString;

        return $this;
    }

    public function getNullableInt(): ?int
    {
        return $this->nullableInt;
    }

    public function setNullableInt(?int $nullableInt): static
    {
        $this->nullableInt = $nullableInt;

        return $this;
    }

    public function getBagOfTests(): ?BagOfTests
    {
        return $this->bagOfTests;
    }

    public function setBagOfTests(?BagOfTests $bagOfTests): static
    {
        $this->bagOfTests = $bagOfTests;

        return $this;
    }
}
<?php

namespace App\Entity;

use ApiPlatform\Metadata\ApiResource;
use App\Repository\BagOfTestsRepository;
use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Serializer\Annotation\Groups;

#[ORM\Entity]
#[ApiResource(
    normalizationContext: ['groups' => ['read']],
    denormalizationContext: ['groups' => ['write']],
)]
class BagOfTests
{
    #[ORM\Id]
    #[ORM\GeneratedValue]
    #[ORM\Column]
    #[Groups(['read', 'write'])]
    private ?int $id = null;

    #[ORM\Column(length: 255)]
    #[Groups(['read', 'write'])]
    private ?string $description = null;

    #[ORM\OneToMany(mappedBy: 'bagOfTests', targetEntity: TestEntity::class)]
    #[Groups(['read', 'write'])]
    private Collection $tests;

    public function __construct()
    {
        $this->tests = new ArrayCollection();
    }

    public function getId(): ?int
    {
        return $this->id;
    }

    public function getDescription(): ?string
    {
        return $this->description;
    }

    public function setDescription(string $description): static
    {
        $this->description = $description;

        return $this;
    }

    /**
     * @return Collection<int, TestEntity>
     */
    public function getTests(): Collection
    {
        return $this->tests;
    }

    public function addTest(TestEntity $test): static
    {
        if (!$this->tests->contains($test)) {
            $this->tests->add($test);
            $test->setBagOfTests($this);
        }

        return $this;
    }

    public function removeTest(TestEntity $test): static
    {
        if ($this->tests->removeElement($test)) {
            // set the owning side to null (unless already changed)
            if ($test->getBagOfTests() === $this) {
                $test->setBagOfTests(null);
            }
        }

        return $this;
    }
}

I get this in v3.1.17:
Screenshot 2023-09-11 at 10 09 26
But this in 3.2.0-alpha.2
Screenshot 2023-09-11 at 10 02 23

I haven't tried populating them and running the assertion, but the missing data isn't going to help the assertion pass. And I don't think it's limited to OneToMany relations, but I suspect there are all related problems.

@soyuka
Copy link
Member Author

soyuka commented Sep 12, 2023

This is fixed, note that schemas have the same output now but swagger ui displays them differently.

@paullallier
Copy link
Contributor

paullallier commented Sep 15, 2023

Thanks @soyuka, @vincentchalamon. It's a lot better, but there's still something odd in the schema, unfortunately. Possibly I'm getting json schema labelled as jsonld in a few cases? My schema match assertions are failing when checking the items inside a subresource collection - detecting the type as "unknown-type" rather than "object"

Screenshot 2023-09-15 at 09 44 36

The other issues I mentioned here also remain - in case they were overlooked when fixing the schema. #5810 (comment)

EDIT: Oh and noting from your comment that this change of display is probably a feature of the updated SwaggerUI, so not in scope. And that the bits highlighted in yellow, above, might be the correct way to define this for Swagger too.
Screenshot 2023-09-15 at 10 35 06

@soyuka
Copy link
Member Author

soyuka commented Sep 18, 2023

@paullallier you tried dev-main? What's the failing use case? subresource collection like /companies/1/employees?

@paullallier
Copy link
Contributor

@soyuka Yes, that was with dev-main from a few days ago. But there's an example repo here:
https://github.com/paullallier/api-platform-schema-bug

Which shows the schema issue:
Screenshot 2023-09-18 at 20 37 17

And the problem with the non-docs page:
Screenshot 2023-09-18 at 20 37 28

And the problem with the reference back to BagOfTests from TestEntity (if you look at the request and response body on the POST /api/test_entities endpoint, for example).

All look OK if I require v3.1.18 instead.

I don't have any subresource endpoints in there (and I don't think I use them in the intended way on my project anyway), so I can't speak for those.

It's definitely possible that I've incorrectly defined something though. Though there isn't much in the repo to mis-define.

@paullallier
Copy link
Contributor

I haven't actually tried persisting and retrieving data from a database with that - but as far as I know (since my tests on my project work) it's only a schema / web page issue. Since I don't use Doctrine ORM though, I can't be certain.

@divine
Copy link
Contributor

divine commented Sep 18, 2023

Hello,

Looks like there is a bug... after upgrading to the dev-main I can't simply debug 404 errors.

Let's say I have a /books route and I'm trying to GET /books/1 which doesn't exist.

What should happen?

I should receive a 404 error message and can click on the profiler to see what database queries were done, right?

This commit works: 2efdc7d

After that commit 404 errors are simply not caught by the profiler (it's shown in the list but when clicking on the link Token not found error).

Probably it was broken in #5657 (wasn't able to check since it's throwing a Graphql validation error which was fixed in 7ecfdff)

Thanks!

@vincentchalamon
Copy link
Contributor

I can confirm this bug was introduced in afb739b. Investigation in progress

@paullallier
Copy link
Contributor

@soyuka It looks like the http://localhost:8000/api page switches to json rather than html at this commit.
2141b01

I think there's a change of config variable names at that point - perhaps the default is bad? Not sure.

And the schema display for TestEntity.json-ld.write - I'm not actually sure I should expect this to have @context, etc fields? Looking at my current project with v3.1.16, some of these ...json-ld.write entries have @context and some do not - and I don't instantly see a pattern.

@vincentchalamon
Copy link
Contributor

@divine fix in progress here: #5837

@divine
Copy link
Contributor

divine commented Sep 20, 2023

@divine fix in progress here: #5837

I can confirm the problem was fixed, thanks.

@soyuka soyuka closed this as completed Sep 21, 2023
@amoulin974
Copy link

For any entities that contain many-to-many relationships, pages created using the next-gen will not work. I have an error :
TypeError: entity.fieldRelation.map is not a function (ex: customer.entity_id.map is not a function)

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

No branches or pull requests

5 participants