Implemented dynamic exclusion using symfony expression language #673

goetas · 2016-11-04T13:30:44Z

This PR allows to use dynamic expressions to decide if exclude or not a field during the serialization process.

Under the hood it uses http://symfony.com/doc/current/components/expression_language.html

class Person
{
    /**
     * @Expose(if="service('some.cool.service').isAllowed(object)")
     */
    public $gender;
    /**
     * @Exclude(if="service('some.cool.service').isAllowed(object)")
     */
    public $gender;
}

the "gender" filed will be serialized if the expression service('some.cool.service').isAllowed(object) returns false or true depending if the annotation used is exclude or expose.

Closes #540

goetas · 2016-11-04T13:35:27Z

src/JMS/Serializer/Serializer.php

@@ -170,6 +182,9 @@ public function fromArray(array $data, $type, DeserializationContext $context =

    private function visit(VisitorInterface $visitor, Context $context, $data, $format, array $type = null)
    {
+        if (null !== $this->expressionEvaluator) {
+            $context->addExclusionStrategy(new DynamicExclusionStrategy($this->expressionEvaluator));
+        }


not really proud on how this is implemented

@schmittjoh any idea on how to do this in a better way?

goetas · 2016-11-04T13:41:30Z

@lsmith77 @schmittjoh @Ener-Getick @xabbuh @willdurand @stof

Some of your opensource projects relies on the serializer, I would be very grateful if somebody of you can have a look at it and review it a bit?

mvrhov · 2016-11-04T17:28:10Z

This and more is implemented in https://github.com/willdurand/Hateoas#exclusion

goetas · 2016-11-04T17:32:26Z

@mvrhov I took inspiration from it :)

xabbuh · 2016-11-16T19:15:42Z

@goetas I am sorry, but I am not really familiar with the JMS Serializer internals. So I am not of much help here.

schmittjoh

I think it would be good to have an exception if someone uses the new capabilities, but does not have the exclusion strategy enabled to enforce it.

Alternatively, we could also think about automatically enabling the exclusion strategy in some way if it encounters an if expression if it's not already active.

In general, I like the changes 👍

schmittjoh · 2016-11-16T19:39:04Z

src/JMS/Serializer/Metadata/Driver/AnnotationDriver.php

@@ -158,8 +159,15 @@ public function loadMetadataForClass(\ReflectionClass $class)
                        $propertyMetadata->serializedName = $annot->name;
                    } elseif ($annot instanceof Expose) {
                        $isExpose = true;
+                        if (null !== $annot->if){
+                            $propertyMetadata->excludeIf = "!" . $annot->if;


Probably need to wrap this in parentheses to support something like a && b.

schmittjoh · 2016-11-16T19:42:12Z

src/JMS/Serializer/Expression/ExpressionEvaluator.php

+
+use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
+
+class ExpressionEvaluator implements ExpressionEvaluatorInterface


Is there a need to support another evaluator? Even if there were one, we already have the strategy interface which could be implemented, no?

done, removed the classes and interfaces, now the dynamic exclusion strategy (renamed in expression language exlusion strategy) uses directly the symfony expression language

goetas · 2016-11-21T12:16:58Z

@schmittjoh can't find any good strategy to throw some exception if the expression language is used for expose/exclude annotation, so i have added this documentation part 65311c4

enabling it automatically is also not possible because the expression language requires always some configuratuion

goetas · 2016-11-21T12:21:46Z

@schmittjoh i consider this as ready for review

schmittjoh · 2016-11-21T14:12:25Z

Regarding the exception, I think you could add a flag to the class metadata (true/false) whether expressions are used, and then also have a similar flag on the Context whether the strategy is enabled. After loading metadata in the GraphNavigator, you can then compare the flags to see if everything is ok.

It's a bit of a code smell to hard code something for a single exclusion strategy in those classes, but I think it's worth it in this case since exposing data that should not be exposed can pose a serious security issue, and it's also better for DX. What do you think?

Another question, how does this behave with deserializing? I think it's currently only designed for serialization?

goetas · 2016-11-22T07:57:58Z

@schmittjoh regarding the user experience, I definitely agree with you that is way better throwing an exception in the serialization process instead of just ignoring it.

I have implemented it with 264f32a but from the code point of view im really not happy...

What do you think... should we keep it hard-coded as it is now... putting the user experience first, and the the code cleanness ?

schmittjoh

I think hard-coding is ok for this.

What about deserialization, did you think about that?

schmittjoh · 2016-11-22T08:04:11Z

src/JMS/Serializer/Context.php

+    public function hasExclusionStrategy($class)
+    {
+        if (isset($this->hasExclusionStrategy[$class])) {
+            return $class;


Should return $this->hasExclusionStrategy[$class] here.

my, bad, fixed

schmittjoh · 2016-11-22T08:05:25Z

src/JMS/Serializer/Context.php

+            if ($strategy instanceof $class) {
+                return $this->hasExclusionStrategy[$class] = true;
+            } elseif ($strategy instanceof DisjunctExclusionStrategy) {
+                $ref = new \ReflectionProperty($strategy, 'delegates');


Better add a getter to that class, maybe even a separate interface like DelegatingStrategyInterface which other classes could also implement and which exposes a getDelegates.

i prefer to add just the method... DelegatingStrategyInterface looks an overkill to me

schmittjoh · 2016-11-22T08:13:58Z

src/JMS/Serializer/Metadata/Driver/AnnotationDriver.php

@@ -158,8 +158,17 @@ public function loadMetadataForClass(\ReflectionClass $class)
                        $propertyMetadata->serializedName = $annot->name;
                    } elseif ($annot instanceof Expose) {
                        $isExpose = true;
+                        if (null !== $annot->if) {
+                            $propertyMetadata->excludeIf = "!(" . $annot->if . ")";
+                            $classMetadata->usingExpression = true;


Better move this to the addPropertyMetadata method, then we do not have to check this in each driver.

good tip, done

goetas · 2016-11-22T08:27:07Z

I think hard-coding is ok for this.

ok, then lets stay with it

What about deserialization, did you think about that?

It was not my usecase so i have just skipped it.

Probably for consistency should be implemented something. My personal opinion is that rarely in deserialization you need to skip some field, so the possible scenario are really few

goetas · 2016-11-23T08:25:56Z

@schmittjoh There is one disadvantage in my approach, when using the context factory to add the exclusion strategy... from now on Context::create() become useless and not enough... now starts to be fundamental having access to the context factories to create a new "good" context.

So probably Context::create() should be deprecated.

I have already prepared a PR for JMS Serializer Bundle and FOS Rest and will submit it soon.

goetas · 2016-11-27T09:38:10Z

@schmittjoh can you give me your feedback on aa4c951 ?

Now the creation if contextes is pretty important, and can't be done with a simple static method anymore... what do you think?

…he evaluator

…ude conditional parameter

schmittjoh · 2016-12-05T16:26:48Z

I've been thinking whether we can change the factory to something that can take a context and populate it with default values where nothing has been explicitly given. I would like to avoid additional complexity for end users if possible.

goetas · 2016-12-05T16:39:03Z

Do not have any really good idea on how to do it.
The problem i see is that exclusion strategies are part of the "context", and a dynamic exclusion strategy needs a wide set of dependencies.

One detail that should be considered is the amount of users using jms serializer via FOS rest bundle, and without FOS.

If they are using FOS rest, this will be fully transparent, since will be really easy to add the context factory via symfony DI container (fos has its own contextes and context factories).
If users are not using FOS, then configuring the expression language is not trivial at all, this means that users will have to figure out how to build contextes anyway.

On the other side, really do not like the static method for creating new contextes. Does not play well with future dependencies (this is more my personal opinion and for the Developer Experience, i'm ready to sacrifice my point of view).

schmittjoh · 2016-12-06T09:53:16Z

We should probably remove exclusion strategies from the context altogether and rather move them to the serializer. The context was intended as a lightweight data object, but with more complex exclusion strategies it is becoming a lot heavier.

With regard to static methods, we can actually deprecate them (they were only needed for older PHP versions). Now, it's just as easy to do (new SerializationContext)->somethingHere().

goetas · 2016-12-06T09:59:23Z

The only solution i see is adding an extra "if" condition in the GraphNavigator (exactly at here).

The approach should work. In that way, the "expression language" will become part of the graph navigator implementation.

Is this an acceptable solution?

goetas · 2016-12-30T16:01:35Z

@schmittjoh Implemented as you suggested.
Now:

there is an exception if the expression language is used but not configured
context creation is lightweight

Implementation:

expression language is added to serializer-builder and propagated via constructors to the graph-navigator.
check is done explicitly in the property loop

This implementation does not require changes on the fos rest bundle :)

…bles

goetas · 2017-01-19T14:31:35Z

was time to merge it :)

Aliance · 2017-01-28T19:39:57Z

composer.json

@@ -38,6 +38,7 @@
        "symfony/validator": "^2.2",
        "symfony/form": "~2.1",
        "symfony/filesystem": "^2.1",
+        "symfony/expression-language": "^2.6|^3.0",


You add some dependency to composer.json but not to composer.lock, so I pull the latest master, run composer install & then run ./vendor/bin/phpunit:

Fatal error: Class 'Symfony\Component\ExpressionLanguage\ExpressionFunction' not found in .../tests/JMS/Serializer/Tests/Serializer/BaseSerializationTest.php on line 243

I think this should be fixed.

The composer.lock file is not committed to git. So you need to run composer update instead.

My bad, I've seen this file in my IDE and think that it's commited to git.

goetas commented Nov 4, 2016

View reviewed changes

goetas added the enhancement label Nov 4, 2016

goetas added this to the v1.5 milestone Nov 4, 2016

goetas force-pushed the exclude-if branch from 52c44a3 to b906d2e Compare November 15, 2016 15:03

schmittjoh reviewed Nov 16, 2016

View reviewed changes

goetas force-pushed the exclude-if branch 2 times, most recently from 491e752 to 0c30f52 Compare November 21, 2016 11:51

schmittjoh reviewed Nov 22, 2016

View reviewed changes

goetas mentioned this pull request Nov 22, 2016

Context factories and expression language schmittjoh/JMSSerializerBundle#535

Closed

goetas added 10 commits December 5, 2016 17:23

added exclude if property option

a28003e

added expression evaluator

686a2e3

added expression based exclusion strategy

2eaae89

Added expression language exclusion strategy ad default strategy to t…

7a56a50

…he evaluator

added tests for new exlude if medatada

8d3d97f

added return param

afb8133

renamed hide variable

ed4daa4

tested serialization process

011f45e

tested builder

869dec8

removed expose-if annotation in flavor of a more readable expose/excl…

4559a8e

…ude conditional parameter

goetas added 4 commits December 5, 2016 17:25

handing cache of expression

b1aae84

the minimum version for expression language is 2.6

05ffb5f

deprecated the static constructors

2f4f860

documented the new way of creating contextes

c93b09e

goetas force-pushed the exclude-if branch from 4182218 to c93b09e Compare December 5, 2016 16:27

goetas added 3 commits December 30, 2016 16:47

explicit use of the expression language exclusion strategy

8b4ad38

updated tests for the explicit exclusion strategy

4570b24

reverted documentation update

c736a69

goetas force-pushed the exclude-if branch from 3cac4ba to c736a69 Compare December 30, 2016 15:53

Improved exception message

c367d0b

This was referenced Dec 30, 2016

Dynamic expose, aka 'fields' query param #195

Closed

Use expression for virtual properties #171

Closed

goetas added 3 commits January 3, 2017 14:38

added expression evaluator to handle easily addition of context varia…

ec8db64

…bles

integrated expression evaluator inside the serializer

072aec3

tested expression evaluator

3850ef0

goetas merged commit 3850ef0 into master Jan 19, 2017

goetas deleted the exclude-if branch January 19, 2017 15:52

This was referenced Jan 20, 2017

Exclude based on condition schmittjoh/JMSSerializerBundle#422

Closed

Enable @Expose on code schmittjoh/JMSSerializerBundle#456

Closed

Aliance reviewed Jan 28, 2017

View reviewed changes

This was referenced Mar 9, 2017

Implemented PathBasedExclusion #253

Closed

Add symfony expression in exclusions/expositions #406

Closed

Are there any way to select entity fields dinamically? schmittjoh/JMSSerializerBundle#496

Closed

goetas mentioned this pull request Apr 3, 2017

[feature] exclusion groups #539

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented dynamic exclusion using symfony expression language #673

Implemented dynamic exclusion using symfony expression language #673

goetas commented Nov 4, 2016 •

edited

Loading

goetas Nov 4, 2016

goetas Nov 16, 2016

goetas commented Nov 4, 2016

mvrhov commented Nov 4, 2016

goetas commented Nov 4, 2016

xabbuh commented Nov 16, 2016

schmittjoh left a comment •

edited

Loading

schmittjoh Nov 16, 2016

goetas Nov 21, 2016

schmittjoh Nov 16, 2016

goetas Nov 21, 2016

goetas commented Nov 21, 2016

goetas commented Nov 21, 2016

schmittjoh commented Nov 21, 2016 •

edited

Loading

goetas commented Nov 22, 2016 •

edited

Loading

schmittjoh left a comment

schmittjoh Nov 22, 2016

goetas Nov 22, 2016

schmittjoh Nov 22, 2016

goetas Nov 22, 2016

schmittjoh Nov 22, 2016

goetas Nov 22, 2016

goetas commented Nov 22, 2016

goetas commented Nov 23, 2016

goetas commented Nov 27, 2016

schmittjoh commented Dec 5, 2016

goetas commented Dec 5, 2016 •

edited

Loading

schmittjoh commented Dec 6, 2016

goetas commented Dec 6, 2016 •

edited

Loading

goetas commented Dec 30, 2016

goetas commented Jan 19, 2017

Aliance Jan 28, 2017

xabbuh Jan 29, 2017

Aliance Jan 29, 2017


		use Symfony\Component\ExpressionLanguage\ExpressionLanguage;

		class ExpressionEvaluator implements ExpressionEvaluatorInterface

Implemented dynamic exclusion using symfony expression language #673

Implemented dynamic exclusion using symfony expression language #673

Conversation

goetas commented Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas commented Nov 4, 2016

mvrhov commented Nov 4, 2016

goetas commented Nov 4, 2016

xabbuh commented Nov 16, 2016

schmittjoh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas commented Nov 21, 2016

goetas commented Nov 21, 2016

schmittjoh commented Nov 21, 2016 • edited Loading

goetas commented Nov 22, 2016 • edited Loading

schmittjoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas commented Nov 22, 2016

goetas commented Nov 23, 2016

goetas commented Nov 27, 2016

schmittjoh commented Dec 5, 2016

goetas commented Dec 5, 2016 • edited Loading

schmittjoh commented Dec 6, 2016

goetas commented Dec 6, 2016 • edited Loading

goetas commented Dec 30, 2016

goetas commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas commented Nov 4, 2016 •

edited

Loading

schmittjoh left a comment •

edited

Loading

schmittjoh commented Nov 21, 2016 •

edited

Loading

goetas commented Nov 22, 2016 •

edited

Loading

goetas commented Dec 5, 2016 •

edited

Loading

goetas commented Dec 6, 2016 •

edited

Loading