Disable eager loading partial fetch by default #1069 #1100

soyuka · 2017-05-04T09:36:11Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1069
License	MIT
Doc PR	na

As discussed in #1069 sometimes you still want to fetch a property, even though it will not be serialized. This new metadata attribute adds a flag that will prevent the eager loader to check the readable state.
ping @bwegrzyn

teohhanhui · 2017-05-04T10:30:19Z

I think we should just use an attribute, since this is a "hint" for the eager loading extension. We do not want to be adding metadata fields that are not well thought out.

soyuka · 2017-05-04T12:05:59Z

hmm yeah you're right, I'm changing this thanks! To me this is not a bug though, more a feature.

bwegrzyn · 2017-05-04T15:27:15Z

Looks good to me! 👍

soyuka · 2017-05-04T15:56:58Z

Wow about the tests, as @meyerbaptiste said master is broken. At about 14h gmt + 1 it was okay 😛

teohhanhui · 2017-05-05T07:52:48Z

Partial loading is dangerous because it breaks entities. It must definitely be opt-in. Having partial loading on by default is thus a bug.

teohhanhui · 2017-05-05T07:53:44Z

I don't believe that this needs to be configurable, not in this way. We should instead make sure that partial loading is only activated on an opt-in basis.

soyuka · 2017-05-06T15:36:39Z

src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php

    {
        $this->propertyNameCollectionFactory = $propertyNameCollectionFactory;
        $this->propertyMetadataFactory = $propertyMetadataFactory;
        $this->resourceMetadataFactory = $resourceMetadataFactory;
        $this->maxJoins = $maxJoins;
        $this->forceEager = $forceEager;
+        $this->fetchPartial = $fetchPartial;


name is fine @teohhanhui 😛 ?

teohhanhui · 2017-05-11T09:34:57Z

src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php

@@ -50,6 +50,7 @@ public function getConfigTreeBuilder()
                    ->addDefaultsIfNotSet()
                    ->children()
                        ->booleanNode('enabled')->defaultTrue()->info('To enable or disable eager loading')->end()
+                        ->booleanNode('fetch_partial')->defaultFalse()->info('Fetch only partial data according to serialization groups')->end()


I think we should add:

"If enabled, Doctrine ORM entities will NOT work as expected if any of the other fields are used."

😝

soyuka · 2017-05-16T10:33:14Z

ping @api-platform/core-team this one should be good to go.

Should we merge this on 2.0 instead? It's not a breaking change but it removes a feature that was enabled by default before. Results won't change but performances may be impacted.

teohhanhui · 2017-05-17T07:26:55Z

This is a bugfix so it should go into 2.0.

dunglas · 2017-05-17T09:03:41Z

src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php

@@ -44,13 +44,14 @@
    private $serializerContextBuilder;
    private $requestStack;

-    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, ResourceMetadataFactoryInterface $resourceMetadataFactory, int $maxJoins = 30, bool $forceEager = true, RequestStack $requestStack = null, SerializerContextBuilderInterface $serializerContextBuilder = null)
+    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, ResourceMetadataFactoryInterface $resourceMetadataFactory, int $maxJoins = 30, bool $forceEager = true, bool $fetchPartial = false, RequestStack $requestStack = null, SerializerContextBuilderInterface $serializerContextBuilder = null)


Isn't this a BC break?

Oops... Another ugly hack required?

p/s: When can we actually break things again? In the Symfony model (deprecations only), the answer seems to be "never, unless you create new interfaces / classes". \o/

Kinda yes. I'll try to handle backward compatibility! I thought about setting this new argument as last one, do you think it'd be a solution?

@teohhanhui I can also put this as last argument. Then, no ugly hacks nor BC break no?

Yeah, sure. But then there's still the underlying problem of piling on layers upon layers of bad design (because we don't really have a choice).

Yeah sure, when we can break again we will refactor those don't worry :).

soyuka · 2017-05-17T13:11:52Z

LGTM, can someone do a last review? Thanks!

Disable eager loading partial fetch by default api-platform#1069

teohhanhui added enhancement bug labels May 4, 2017

soyuka force-pushed the fix/1069 branch 4 times, most recently from 44ae45c to e2aadf6 Compare May 4, 2017 12:11

soyuka changed the title ~~Add fetchable Property metadata #1069~~ Add fetchable attribute on Property metadata #1069 May 4, 2017

soyuka changed the title ~~Add fetchable attribute on Property metadata #1069~~ Enable fetchable attribute on Property metadata #1069 May 4, 2017

soyuka force-pushed the fix/1069 branch from e2aadf6 to 338c1d3 Compare May 4, 2017 12:13

soyuka changed the title ~~Enable fetchable attribute on Property metadata #1069~~ Disable eager loading partial fetch by default #1069 May 5, 2017

soyuka force-pushed the fix/1069 branch 2 times, most recently from 6ae4991 to 752455d Compare May 5, 2017 15:18

soyuka commented May 6, 2017

View reviewed changes

soyuka mentioned this pull request May 8, 2017

Normalization group not working in nested embedded entities api-platform/api-platform#277

Closed

soyuka force-pushed the fix/1069 branch 4 times, most recently from be428cc to 3f2d865 Compare May 9, 2017 12:40

soyuka added the [RFR] Ready for Review label May 9, 2017

teohhanhui reviewed May 11, 2017

View reviewed changes

soyuka force-pushed the fix/1069 branch 4 times, most recently from 298cd49 to cf4831b Compare May 16, 2017 07:45

soyuka changed the base branch from master to 2.0 May 17, 2017 07:30

soyuka force-pushed the fix/1069 branch from cf4831b to 22f6fed Compare May 17, 2017 07:40

dunglas reviewed May 17, 2017

View reviewed changes

dunglas approved these changes May 17, 2017

View reviewed changes

soyuka force-pushed the fix/1069 branch 2 times, most recently from b35c20b to ce14652 Compare May 17, 2017 12:36

Disable eager loading partial fetch by default api-platform#1069

e34427a

soyuka force-pushed the fix/1069 branch from ce14652 to e34427a Compare May 17, 2017 12:42

soyuka merged commit 84a402c into api-platform:2.0 May 18, 2017

Simperfit mentioned this pull request Jun 14, 2017

Eager Loading Extension partial objects causing properties to not be available #1069

Closed

soyuka deleted the fix/1069 branch January 29, 2018 09:38

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Merge pull request api-platform#1100 from soyuka/fix/1069

300ef7d

Disable eager loading partial fetch by default api-platform#1069

edamov mentioned this pull request Oct 22, 2019

fetchPartial documentation api-platform/docs#946

Merged

Uh oh!

Disable eager loading partial fetch by default #1069 #1100

Disable eager loading partial fetch by default #1069 #1100

Uh oh!

Conversation

soyuka commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teohhanhui commented May 4, 2017

Uh oh!

soyuka commented May 4, 2017

Uh oh!

bwegrzyn commented May 4, 2017

Uh oh!

soyuka commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teohhanhui commented May 5, 2017

Uh oh!

teohhanhui commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented May 16, 2017

Uh oh!

teohhanhui commented May 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented May 17, 2017

Uh oh!

Uh oh!

soyuka commented May 4, 2017 •

edited

Loading

soyuka commented May 4, 2017 •

edited

Loading

teohhanhui commented May 5, 2017 •

edited

Loading