Core: Eager Loading Extension partial objects causing properties to not be available

[bwegrzyn]

It seems like the eager loading extension loads partial objects from Doctrine through the use of the @Groups annotation. While this seems like it would be a good idea in theory, proves to be quite a hassle in practice.

I've started using @dunglas https://github.com/dunglas/doctrine-json-odm to store events with a bunch of data in them as opposed to using inheritance with several children. Because these json_document fields contain quite a bit of info that I don't need to expose in the API, I don't want to expose the entire field. Instead, I create custom getters and setters and expose those via the @Groups annotation.

However, because the json_document field is not exposed through the @Groups annotation, it is always NULL when the eager loading extension is enabled, and thus the custom getters that I have exposed return NULL as well.

In the example below, you can see the context field being using to the store the json_document. Individual properties of this property are exposed through the getMessage() and setMessage() methods. This does not work when eager loading is enabled as the context field is always NULL, but works correctly when eager loading is disabled.

I'd like to propose a few methods on how to handle this:

1. Remove partial objects from the eager loading extension as they prove to be more painful than helpful.
2. Maybe add a property to the @ApiProperty annotation that will cause the eager loading extension to load the property regardless of the @Group annotation.
3. Any other ideas?

/**
 * @ApiResource(
 *     attributes={
 *          "normalization_context"={"groups"={"api-read"}},
 *          "denormalization_context"={"groups"={"api-write"}}
 *     }
 * )
 *
 * @ORM\Entity
 */
class Event
{

    /**
     * @ORM\Column(type="json_document", options={"jsonb": true})
     *
     * @var EventContextInterface
     */
    protected $context;

    /**
     * @return EventContextInterface
     */
    public function getContext()
    {
        return $this->context;
    }

    /**
     * @param EventContextInterface $context
     */
    protected function setContext(EventContextInterface $context)
    {
        $this->context = $context;
    }

    /**
     * @Groups({"api-read"})
     *
     * @return string
     */
    public function getMessage()
    {
        return $this->getContext()->getMessage();
    }

    /**
     * @Groups({"api-write"})
     *
     * @param $message
     *
     * @return $this
     */
    public function setMessage($message)
    {
        $this->getContext()->setMessage($message);

        return $this;
    }
}

[soyuka]

I disagree about removing partials, it's neat to fetch only the data you need.

Maybe add a property to the @ApiProperty annotation that will cause the eager loading extension to load the property regardless of the @Group annotation.

+1, for example @ApiProperty(readableLink=true, readable=true) should work in your case. (may relate to #1066)

[bwegrzyn]

I agree that partial objects should be fixed rather than removed, I'm just not a fan of partial objects as they've caused me tons of issues in the past! :)

Setting readableLink=true doesn't do anything because EagerLoadingExtension->addSelect() ignores that property and readable=true is replaced with readable=false in SerializerPropertyMetadataFactory.

As far as I can see, there is currently no way to force the eager loading extension to load a property without also exposing it in the serialization process.

[soyuka]

We need to fix #1066 first because user-configured metadata should take precedence over other configurations.

Never mind, this would not fix this particular issue if, like you said, you want to "load a property without also exposing it in the serialization process".

I see a few possibilities here:

1. Allow to disable partial selection
2. Adding a new Property metadata attribute allowing to fetch the given property without checking it's readable status (ie not related to serialization)
3. Remove partial selection, though 1. would be better IMO

[bwegrzyn]

I was just looking at #1066 and I do agree with what is said there.

As far as your suggestions, I think I would prefer option 2 then option 1, in that order. Both can probably be done without a BC.

[soyuka]

As far as your suggestions, I think I would prefer option 2 then option 1, in that order. Both can probably be done without a BC.

Okay, I'll submit a patch for this!

[teohhanhui]

@soyuka This is actually a bug, because it results in invalid data when eager loading is enabled.

[soyuka]

I disagree, the data you serialize is the one you fetch from the database. Data is therefore valid according to what is serialized no?

[teohhanhui]

The entities cannot work as expected when partially loaded. It'd be an unpleasant surprise for the user - broken by default.

[soyuka]

I don't see how partially loaded entities don't work as long as it's used in APIPlatform's scope. Anyway, for everyone's sake and to avoid more issues on this, I'll change it. Still, I think it's a great feature and I'd like to keep it configurable.

So, what about I:

1. disable partial by default
2. have a new configuration to enable partial selection (resource attributes and config)

Also, I'll close this pull request and we won't be able to partial + force fetch some properties (like here).

[bwegrzyn]

I have agree with @teohhanhui. Fetching partial objects is great for very simple use cases, but for APIs that require something a bit more complex (such as serializing the output of a method), partial objects will break it because it won't know which attributes are actually required.

I'd like to see this feature configurable, but maybe keep the default to disable partial objects to prevent confusion.

[soyuka]

@bwegrzyn I'm on it ;)

[Simperfit]

This is fixed since #1100