nelmio / NelmioApiDocBundle

Generates documentation for your REST API from annotations
MIT License
2.22k stars 833 forks source link

Groups is not used for Model naming in array property of object #1787

Open arneee opened 3 years ago

arneee commented 3 years ago

Hi,

I have the following classes:

class Bu {
    /**
     * @ORM\Column(name="name", type="string", length=255, nullable=false)
     * @JMS\Groups("Default")
     */
    private string $name;
}
class AppDataDTO {
    /**
     * @var Bu[]
     * @JMS\Type("array<App\Entity\Bu>")
     */
    private array $bus = [];
}

Then I have two controllers:

     /*
     * @OA\Get(operationId="getData",
     *  tags={"app"},
     *  @OA\Response(
     *     response=200,
     *     description="Base data for the application",
     *     @ApiDoc\Model(type=AppDataDTO::class, groups={"Default"})
     *  )
     * )
     */
    /*
     * @OA\Get(
     *     operationId="getBu",
     *     tags={"bu"},
     *     @OA\Parameter(
     *         name="buId",
     *         in="path",
     *         description="The BU to get",
     *         @OA\Schema(type="integer")
     *     ),
     *  @OA\Response(
     *     response=200,
     *     description="The BU",
     *     @ApiDoc\Model(type=Bu::class, groups={"Default"})
     *  )
     * )
     */

Strangely, this results in two Models, Bu and Bu2, both with the same schema and fields ("name" is included in both).

To "debug", I've added aliases:

    models:
        names:
            - { alias: BuNone, type: App\Entity\Bu }
            - { alias: BuDefault, type: App\Entity\Bu, groups: [Default] }

The one from the getData API is being named BuNone, the one from get getBu API is named "BuDefault".

So it seems like the "Default" group is used for the schema, but not the model name in the getData API.

After looking in the ModelRegistry with the debugger, I indeed see the one Bu Model without any "groups".

Any idea how to solve this? Are groups generally not "inherited" from parent model / groups?

arneee commented 3 years ago

Hi,

even if I change the AppDataDTO to specifically include the Groups, I still end up with two differently named models with the same properties:

class AppDataDTO {

    /**
     * @var Bu[]
     * @JMS\Type("array<App\Entity\Bu>")
     * @OA\Property(property="bus", @OA\Items(ref=@ApiDoc\Model(type=Bu::class, groups={"Default"})))
     */
    private array $bus = [];

It seems like the JMS Describer overwrites the existing Items (with the ref).

As a dirty hack i managed to get it working (not the example from the first post, but the workaround in this one) changing the JMSModelDescriber on line 245:

if(empty($property->items->ref)) {
    $property->items = Util::createChild($property, OA\Items::class);
    $this->describeItem($nestedType, $property->items, $context);
}
GuilhemN commented 3 years ago

Hi!

I'm not the one who wrote the JMSDescriber (it's @goetas), but this describer removes the Default group whenever possible to make it implicit (no groups defined in @Model is the same as ['Default']), so here what you get is that Bu is defined with the Default group, while Bu2 is defined with no groups but the result should be the same as no groups is equivalent to the Default group.

Isn't it enough to use @Model without the groups field when using only the Default group?

arneee commented 3 years ago

Hi,

Thanks for your reply! Actually using "Default" and no group is different in serialization. No group will include all fields regardless of their group, "Default" will only include the fields which don't have any group. So unfortunately I can't remove the Default group otherwise the fields I don't want in my public model would show up.

https://jmsyst.com/libs/serializer/master/cookbook/exclusion_strategies

Any property without an explicit @Groups annotation will be included in a Default group, which can be used when specifying groups in the serialization context.

I will try to explicitly mark all properties I need as "Public" and check if the problem also occurs in this case, or if it is special with the "Default" group.

arneee commented 3 years ago

Interesting, I can confirm it is working fine by using an explicit group everywhere (like "Standard" or "Public"). I don't exactly understand why, but it works. I didn't even need the OA\Property in the AppDataDTO.

So it only seems to be an issue when "Default" is used to get the non-group properties in combination with the JMS\Type annotation.

GuilhemN commented 3 years ago

This is most likely related to https://github.com/nelmio/NelmioApiDocBundle/blob/master/ModelDescriber/JMSModelDescriber.php#L195-L197, but I believe it is really there on purpose to consider as similar ['Default'] and null groups, that's bothering if that's not working so well :thinking:

arneee commented 3 years ago

Removing that seems to work :) But I'm not sure what kind of side effects that would have.

goetas commented 3 years ago

Actually using "Default" and no group is different in serialization. No group will include all fields regardless of their group, "Default" will only include the fields which don't have any group.

That is not exactly true.

Default or no group are handled in the same way when there is a group set in the context. While is there is no group set in the context, the all groups are just ignored.

DoobleD commented 3 years ago

I'm having the exact same issue, and removing https://github.com/nelmio/NelmioApiDocBundle/blob/master/ModelDescriber/JMSModelDescriber.php#L195-L197 works for me as well, though like @arneee I don't know the repercussions of that change.

arneee commented 3 years ago

I ended up always explicitly defining a group on each property and specify it everywhere instead of using the "magic" default group.

DoobleD commented 3 years ago

Thanks for the update @arneee. I can get it to work too using explicit non Default groups everywhere. That kind of defeats the whole purpose of the Default group though, if we can't use it.

With the removal of L195-197 I can just put an internal group on properties of the internal API, and the rest is simply considered Default. That way when I require Default I only get the properties without a group (i.e. all non internal properties).

I'd prefer the latter solution if the fix was validated and pushed into master.

chrisguitarguy commented 2 years ago

Related:

The model name generation thing is definitely a pain point, it seems.