Elao / PhpEnums

:nut_and_bolt: Extended PHP 8.1+ enums features & specific integrations with frameworks and libraries
MIT License
324 stars 27 forks source link

Serializationgroup Support #106

Closed GrmpfNarf closed 2 years ago

GrmpfNarf commented 3 years ago

Hello everyone, is there any need for the support of Symfony\Component\Serializer\Annotation\Groups for an enum ? With this it is possible to reduce or expand the enum based on the (de)serializationgroup.

I have already implemented this feature and will be able to provide a PR if there is a need for this feature.

ogizanagi commented 3 years ago

Hi @GrmpfNarf

reduce or expand the enum based on the (de)serializationgroup.

What do you mean exactly?

GrmpfNarf commented 3 years ago

Hi, in our case it is necessary to reduce the possible values of an enum in the Open Api documentation (and of course in the normalizer) by the given serializationgroups.

An example:

<?php

namespace App\Enum;

use Elao\Enum\AutoDiscoveredValuesTrait;
use Elao\Enum\Enum;

class ActionEnum extends Enum
{
    use AutoDiscoveredValuesTrait;

    public const ADD = 'ADD';

    public const MODIFY = 'MODIFY';

    public const DELETE = 'DELETE';
}

In one Api Platform endpoint all enum values are possible and in another one just a subset. But the entity which is used should stay the same, and creating a DTO just to change the allowed enum values seems a litte bit overdressed.

So I want to use serializationgroups to reduce the possible enum values in this endpoint:

<?php

namespace App\Enum;

use Elao\Enum\AutoDiscoveredValuesTrait;
use Elao\Enum\Enum;
use Symfony\Component\Serializer\Annotation\Groups;

class ActionEnum extends Enum
{
    use AutoDiscoveredValuesTrait;

    /**
     * @Groups({"full", "partially"})
     */
    public const ADD = 'ADD';

    /**
     * @Groups({"full", "partially"})
     */
    public const MODIFY = 'MODIFY';

    /**
     * @Groups({"full"})
     */
    public const DELETE = 'DELETE';
}

Now the API platform has two models one Model-full and one Model-partially

When I look at the schema in the Open Api documentation die Model-full looks like this:

someProperty:
          type: string
          enum:
            - ADD
            - MODIFY
            - DELETE
          example: ADD

The model of Model-partially looks like this:

someProperty:
          type: string
          enum:
            - ADD
            - MODIFY
          example: ADD

And in the denormalization process an UnexpectedValueException is thrown when I try to set DELETE on a endpoint which don't have the group full.

ogizanagi commented 3 years ago

I see. Thank you for detailed explanation of your use-case 👍

Ideally, I'd like to provide something like this, but using context. The enum:

class ActionEnum extends Enum
{
    use AutoDiscoveredValuesTrait;

    public const ADD = 'ADD';
    public const MODIFY = 'MODIFY';
    public const DELETE = 'DELETE';
}

stays without annotations/mapping.

But the context would allow, on a per-case-basis, to provide the allowed values:

$this->serializer->unserialize($json, 'json', [
    EnumNormalizer::ALLOWED_VALUES => [ActionEnum::ADD, ActionEnum::MODIFY],
])

This can be implemented right now, easily.

To complete this feature and cover any use-case easily, I'd like to be able to set this context on a per-property basis:


class MyObjectBeingUnserialized
{
    // [...]

    /**
     * @var ActionEnum
     *
     * @Serializer\Context({
     *      EnumNormalizer::ALLOWED_VALUES = [ActionEnum::ADD, ActionEnum::MODIFY]
     * })
     */
    public $property;

    // [...]
}

But @Serializer\Context (or it's yaml/xml mapping equivalent) does not exist yet (it's a feature I'd like to implement since a while).

I think it's more appropriate than serialization groups for some reasons:

WDYT?

GrmpfNarf commented 3 years ago

Hi, I don't see the difference by just using 2 different ENUMs. It also seems not to help with my problem or will the annotation be able to support different serialization contexts ?

groups are originally meant to control how is serialized/unserialized, i.e which properties are available. But not which values are acceptable. For instance, a legit use-case would be to control whether a readable enum should be encoded with it's label as well, or just the raw value alone.

I agree with that but ENUM values are constant so I think it should be allowed to custom them by Groups

multiplying groups can become a nightmare in an app.

They are already a little nightmare when you have a complex DTO/Entity structure but I agree that using an annotation is more readable instead if looking in the Enum to see which values are allwoed.

naming is hard, and you might not need to have "named" sets of values for this. So specifying explicitly the allowed values on a per-case basis is more powerful. But in case you want to name your subsets, just use a php constant containing the array of allowed values for this set, and reference it in the @Context.

Which naming to you mean ?

ogizanagi commented 3 years ago

Which naming to you mean ?

Naming groups created specifically for such use-cases.

I don't see the difference by just using 2 different ENUMs. It also seems not to help with my problem or will the annotation be able to support different serialization contexts ?

The annotation is set on any entity/DTO property on a per-case basis. Without this annotation, the property would accept any value. With this annotation, the property (and only this property) would only accept a subset.

The idea of the @Serializer\Context annotation precisely is to be able to provide a different serialization context on any serialized class/property.

ogizanagi commented 3 years ago

However, regarding the use-case of restricting values specifically, may I ask why did you opt for a solution during denormalization, rather than using validation?

I.e:

use Elao\Enum\Bridge\Symfony\Validator\Constraint\Enum as AssertEnum;

class MyObjectBeingUnserialized
{
    // [...]

    /**
     * @var ActionEnum
     *
     * @AssertEnum(ActionEnum::class, choices=[ActionEnum::ADD, ActionEnum::MODIFY])
     */
    public $property;

    // [...]
} 
GrmpfNarf commented 3 years ago

Hi, I missed the idea behind the annotation but after I've consulted a colleague of mine I understand now how it should work.

We don't need the context on the property for Apiplatform it's sufficent on class level for now.

We already have build the solution with a small improvement in the context notation.

<?php
...
/**
 * @ApiResource(
 *     itemOperations={
 *         "get",
 *         "get_partially"={
 *             "method"="GET",
 *             "path"="/partially/{id}",
 *             "normalization_context"={
 *               EnumNormalizer::ALLOWED_VALUES = {
 *                  ActionEnum::class = {
 *                      ActionEnum::ADD, ActionEnum::MODIFY
 *                  },
 *               }
 *             }
 *         }
 *     },
 *     collectionOperations={
 *          "get",
 *          "post_partially"={
 *              "method"="POST",
 *              "path"="/partially",
 *              "normalization_context"={
 *                  "groups"={"provisionOrder"},
 *                  EnumNormalizer::ALLOWED_VALUES = {
 *                      ActionEnum::class = {
 *                          ActionEnum::ADD, ActionEnum::MODIFY
 *                      },
 *                  }
 *              },
 *              "denormalization_context"={
 *                  EnumNormalizer::ALLOWED_VALUES = {
 *                      ActionEnum::class = {
 *                          ActionEnum::ADD, ActionEnum::MODIFY
 *                      },
 *                  }
 *              },
 *          },
 *          "get_partially"={
 *              "method"="GET",
 *              "path"="/partially",
 *              "normalization_context"={
 *                  EnumNormalizer::ALLOWED_VALUES = {
 *                      ActionEnum::class = {
 *                          ActionEnum::ADD, ActionEnum::MODIFY
 *                      },
 *                  }
 *              }
 *          }
 *     }
 * )
 */
class SomeClass
{
    ...

    /**
     * @var ActionEnum
     */
    private $action;

    ....
}

And ActionEnum stays in default

class ActionEnum extends Enum
{
    use AutoDiscoveredValuesTrait;

    public const ADD = 'ADD';
    public const MODIFY = 'MODIFY';
    public const DELETE = 'DELETE';
}

And in Normalizer we just added a method which checks that the value is allowed:

public static function isAllowed(string $class, $value, array $context)
    {
        if (array_key_exists(EnumNormalizer::ALLOWED_VALUES, $context)  && array_key_exists($class, $context[EnumNormalizer::ALLOWED_VALUES])  && null !== $allowedValues = $context[EnumNormalizer::ALLOWED_VALUES][$class]) {
            return in_array($value, $allowedValues, true);
        }

        return true;
    }
ogizanagi commented 3 years ago

I see, that's interesting. As ApiPlatform already has a built-in way to provide a context per resource, that's a smart way to tackle your use-case 👌 I'd expect a future context config built-in the serializer to tackle more situations like this (https://github.com/symfony/symfony/issues/39039)

Can I point out again https://github.com/Elao/PhpEnums/issues/106#issuecomment-724079996 about validation for your use-case, though? Wouldn't it be more appropriate as it would allow to build an API response with the full list of violations?

GrmpfNarf commented 3 years ago

Can I point out again #106 (comment) about validation for your use-case, though? Wouldn't it be more appropriate as it would allow to build an API response with the full list of violations?

Validation is also a way but it has 2 problems in my opinion:

  1. I have to write it in every property when is Enum is used multiple times
  2. It does not affect the documentation generation (which is the bigger problem in our case)