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

APIResource mess with denormalization for underscore property #5641

Closed
Grafikart opened this issue Jun 28, 2023 · 6 comments
Closed

APIResource mess with denormalization for underscore property #5641

Grafikart opened this issue Jun 28, 2023 · 6 comments
Labels

Comments

@Grafikart
Copy link
Contributor

API Platform version(s) affected: 3.1

Description
I have a model with the following field

<?php

namespace App\Entity;

use ApiPlatform\Metadata\ApiProperty;
use App\Repository\UserRepository;
use App\State\DebugProcessor;
use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Serializer\Annotation\Groups;

#[ORM\Entity(repositoryClass: UserRepository::class)]
#[ORM\Table(name: '`user`')]
class User
{
    #[ORM\Id]
    #[ORM\GeneratedValue]
    #[ORM\Column]
    private ?int $id = null;

    #[ORM\Column(length: 255)]
    #[Groups("write")]
    private ?string $username = null;

    #[ORM\Column(length: 255)]
    #[Groups("write")]
    private ?string $first_name = null;

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

    public function getUsername(): ?string
    {
        return $this->username;
    }

    public function setUsername(string $username): static
    {
        $this->username = $username;

        return $this;
    }

    public function getFirstName(): ?string
    {
        return $this->first_name;
    }

    public function setFirstName(string $first_name): static
    {
        $this->first_name = $first_name;

        return $this;
    }
}

If I try to denormalize it I get a model filled with all the properties.

        $data = $denormalizer->denormalize([
            'username' => 'John',
            'first_name' => 'Doe',
        ], User::class, 'json', [
            'groups' => ['read']
        ]);
        dd($data);

Then, If I add APIResource attribute on my model, the denormalization is broken for fields with _ (here first_name)

#[ApiResource]
class User {
 // ...

How to reproduce
Add a proprerty with underscore to an entity and use the denormalizer to set the property. It will stay to null

Possible Solution
I'm currently debugging to understand what API Platform add.

@Grafikart
Copy link
Contributor Author

Grafikart commented Jun 29, 2023

After further investigation in AbstractObjectNormalizer $allowedAttributes is ["username", "firstName"] when APIResource is enabled instead of ["username", "first_name"], my guess is a problem with getAllowedAttributes() in AbstractItemNormalizer

The method $this->isAllowedAttribute($classOrObject, $propertyName, null, $context) return false and it expects a #[ApiProperty(writable: true)] attribute on the property. But I don't see ApiProperty documented a lot on the documentation so I wonder if adding the APIProperty attribute is still the way to go ?

@soyuka
Copy link
Member

soyuka commented Jun 30, 2023

There should be some kind of NameConverter that isn't doing things right.

@soyuka soyuka added the bug label Jun 30, 2023
@Grafikart
Copy link
Contributor Author

If it helps I created a blank project (--webapp) with symfony and added api platform with composer require api to be sure I don't have parasit code. Since the #[ApiProperty] exists there is not urgency.

@jbtronics
Copy link

This problem is related to issue #1554
API platform basically considers all snake_case style properties as readonly, and therefore ignores the data on denormalization.
This is actually caused by an issue (or an inconsistency) in the PropertyInfo symfony component API Platform uses to determine if a property is writeable.

For some reason this component considers an object with a camelCase style getter as readable (therefore reading works fine in API platform), but it completely ignores the camelCase style setters and therefore reports the property as readonly. However the setter method is accessible by the serializer and PropertyAccess component (as can be seen, when you override that info with the APIProperty attribute), and therfore the reported information is flawed.
If your setter would be named setFirst_name() in your example, the PropertyInfo correctly reports the property as writable and API Platform would behave fine.

I have submitted a pull request to the PropertyInfo component which should fix this issues.

@jbtronics
Copy link

As a workaround until the PR is merged, you can define an additional PropertyAccessExtractorInterface service for the PropertyInfo component, which can correctly handle the ability to write snake_case properties with camelCase setter.

See this gist for an example: https://gist.github.com/jbtronics/0e0f36cfaf49d13d13fa0b8023532b9b

nicolas-grekas added a commit to symfony/symfony that referenced this issue Sep 29, 2023
… isReadable() when checking snake_case properties (jbtronics)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[PropertyInfo] Make isWriteable() more consistent with isReadable() when checking snake_case properties

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Related with issues #29933 and #16889
| License       | MIT
| Doc PR        |

Currently PropertyInfo is a bit inconsistent in the behavior of isWriteable() and isReadable() when using it with properties in snake_case format, which are only accessible via a getter and setter. To be readable the getter function has to be in camel_case (e.g. `getSnakeCase()`), while the setter function has to remain in the snake case format (e.g. `setSnake_case()`).

In this example class:
```php
class Dummy {
  private string $snake_case;

  public function getSnakeCase(): string
  { }

  public function setSnakeCase(string $snake_case): void
  { }

}
````

the property `snake_case` is considered readable like you would expect, but not writeable, even though the property has a useable setter and the value can be actually be modified fine by something like the PropertyAccess component.
To make the property actually considered writeable, the setter would need to be named `setSnake_case`, which is pretty inconsistent with the behavior of isReadable and makes it very hard to use this component on snake_case properties.

This inconsistencies are caused by the fact, that `isReadable` in ReflectionExtractor uses the `getReadInfo()` function which does a camelization of the property name internally, while the `isWriteable()` function uses the `getMutatorMethod()` function which just perform a capitalization of the first letter.

This PR fixes this inconsistencies between isReadable() and isWriteable() by allowing to use a camelCase style setter to be considered writeable, as this is much more common then to use the mix of snake and camelCase currently required.

The getWriteInfo() function is not useable here, because it needs both a add and remove Function on collection setters to give proper infos, while the current `isWriteable()` implementation considers a class with just a add or a remove function as writeable. Therefore the property name just gets camelized before being feed into the `getMutatorMethod()`, which gives the desired result.

To maximize backwards compatibility, if no camelCase style setter is found, it is still checked for a snake_case setter, so that classes having these, are still considered writeable after the change.

The current behavior is causing some inconsistencies in higher level libraries, which rely on this component. In API Platform for example properties in snake_case are considered read only even though they have a (camel case) setter, because of this. See issue: api-platform/core#5641 and api-platform/core#1554

Commits
-------

7c9e6bc [PropertyInfo] Make isWriteable() more consistent with isReadable() when checking snake_case properties
symfony-splitter pushed a commit to symfony/property-info that referenced this issue Sep 29, 2023
… isReadable() when checking snake_case properties (jbtronics)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[PropertyInfo] Make isWriteable() more consistent with isReadable() when checking snake_case properties

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Related with issues #29933 and #16889
| License       | MIT
| Doc PR        |

Currently PropertyInfo is a bit inconsistent in the behavior of isWriteable() and isReadable() when using it with properties in snake_case format, which are only accessible via a getter and setter. To be readable the getter function has to be in camel_case (e.g. `getSnakeCase()`), while the setter function has to remain in the snake case format (e.g. `setSnake_case()`).

In this example class:
```php
class Dummy {
  private string $snake_case;

  public function getSnakeCase(): string
  { }

  public function setSnakeCase(string $snake_case): void
  { }

}
````

the property `snake_case` is considered readable like you would expect, but not writeable, even though the property has a useable setter and the value can be actually be modified fine by something like the PropertyAccess component.
To make the property actually considered writeable, the setter would need to be named `setSnake_case`, which is pretty inconsistent with the behavior of isReadable and makes it very hard to use this component on snake_case properties.

This inconsistencies are caused by the fact, that `isReadable` in ReflectionExtractor uses the `getReadInfo()` function which does a camelization of the property name internally, while the `isWriteable()` function uses the `getMutatorMethod()` function which just perform a capitalization of the first letter.

This PR fixes this inconsistencies between isReadable() and isWriteable() by allowing to use a camelCase style setter to be considered writeable, as this is much more common then to use the mix of snake and camelCase currently required.

The getWriteInfo() function is not useable here, because it needs both a add and remove Function on collection setters to give proper infos, while the current `isWriteable()` implementation considers a class with just a add or a remove function as writeable. Therefore the property name just gets camelized before being feed into the `getMutatorMethod()`, which gives the desired result.

To maximize backwards compatibility, if no camelCase style setter is found, it is still checked for a snake_case setter, so that classes having these, are still considered writeable after the change.

The current behavior is causing some inconsistencies in higher level libraries, which rely on this component. In API Platform for example properties in snake_case are considered read only even though they have a (camel case) setter, because of this. See issue: api-platform/core#5641 and api-platform/core#1554

Commits
-------

7c9e6bc36e [PropertyInfo] Make isWriteable() more consistent with isReadable() when checking snake_case properties
@soyuka
Copy link
Member

soyuka commented Oct 17, 2023

thanks @jbtronics let us now if this doesn't solve your issue

@soyuka soyuka closed this as completed Oct 17, 2023
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