Add the Groups feature #96

Merged
merged 2 commits into from Apr 12, 2012

Projects

None yet

9 participants

@chregu
chregu commented Mar 22, 2012

See #60

It adds `@Serializer\Group"

Annotate your properties with

* @Serializer\Group({"Bar","foo"})

then do

$serializer->setGroups(array("foo","zero"));

and if any matches, the property will be serialized. If none matches or there is no group tag, then it won't be serialized.

2 Things:

  • Maybe we should use Groups instead of Group
  • I didn't use @Expose(groups={"user"}) which would maybe be better, since then we could use it also in Exclude. Now it's basically only usable as Expose.

If someone can give me a hint where I have to look for the 2nd point in the code, I could maybe change that.

@michelsalib

Nice to see such a useful PR. I don't really have an opinion about your second point, but IMO you should use Groups instead of Group in order to be consistent with the Validation sf2 component.
Also, it would be very nice to have unit tests.

@chregu
chregu commented Mar 22, 2012

Unit tests are coming, but not this week. Wanted to have some final discussion on the interface first

@schmittjoh schmittjoh commented on an outdated diff Mar 22, 2012
Serializer/Exclusion/GroupExclusionStrategy.php
+ $this->group = $group;
+ }
+
+ public function shouldSkipClass(ClassMetadata $metadata)
+ {
+ return false;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function shouldSkipProperty(PropertyMetadata $property)
+ {
+ if ($this->group && $property->group) {
+ foreach($this->group as $group) {
+ if (in_array($group, $property->group)) {
@schmittjoh
schmittjoh Mar 22, 2012 owner

I'd like to store the groups in the form of array('group' => true), and then simply use isset() here for performance reasons.

@schmittjoh
Owner

I'm not sure whether it makes sense to re-use the @Expose/@Exclude annotations as you only either use @Expose, or @Exclude depending on your @ExclusionPolicy. I find it a bit strange to use an @ExclusionPolicy("NONE"), and then exclude different properties depending on the group setting (i.e. serializing everything that does not have the group). When would you do this? I find the other approach (only serializing what has the group) much more intuitive.

Some other thoughts:

  • Does it make sense to allow to set multiple groups on the GroupExclusionStrategy? One group should be enough, no?
  • I'm not entirely sure about naming this "Group" because it seems more like a purpose, or intention to me. Like I want to serialize objects for my ListView, or my DetailsView, etc. I don't have a better name for now though.
@mvrhov
mvrhov commented Mar 22, 2012

My expectation would be, that the exclusion policy is applied first and then what remains is filtered based on the currently selected group.

@chregu
chregu commented Mar 22, 2012

@schmittjoh I will do the changes like you said and then update this PR, but that won't be before Monday, I guess

@chregu chregu closed this Mar 22, 2012
@chregu chregu reopened this Mar 22, 2012
@lsmith77 lsmith77 and 1 other commented on an outdated diff Mar 24, 2012
Metadata/Driver/AnnotationDriver.php
@@ -91,7 +92,6 @@ public function loadMetadataForClass(\ReflectionClass $class)
$AccessType = $classAccessType;
$accessor = array(null, null);
foreach ($this->reader->getPropertyAnnotations($property) as $annot) {
- if ($annot instanceof Since) {
$propertyMetadata->sinceVersion = $annot->version;
@lsmith77
lsmith77 Mar 24, 2012

that looks weird

@chregu
chregu Mar 25, 2012

That's indeed an issue noone noticed before ;)
will be fixed in the next push

@lsmith77 lsmith77 commented on an outdated diff Mar 25, 2012
Serializer/Exclusion/GroupExclusionStrategy.php
+ }
+ $this->group = $group;
+ }
+
+ public function shouldSkipClass(ClassMetadata $metadata)
+ {
+ return false;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function shouldSkipProperty(PropertyMetadata $property)
+ {
+ if ($this->group && $property->group) {
+ foreach($this->group as $group) {
@lsmith77
lsmith77 Mar 25, 2012

missing space :)

@chregu
chregu commented Mar 25, 2012

Here's the update, renamed everything to Groups and setGroups and did the proposed changed.

And IMHO it should be possible to use multiple groups in the annotation and in setGroups

Tests still missing, will come

@chregu
chregu commented Mar 26, 2012

Tests added. Still missing are XML and YAML drivers

anyone knows, how
* @Serializer\Groups({"bar","foo"});

is translated to XML? (as this is an array)

@schmittjoh schmittjoh and 1 other commented on an outdated diff Mar 26, 2012
Annotation/Groups.php
+namespace JMS\SerializerBundle\Annotation;
+
+use JMS\SerializerBundle\Exception\RuntimeException;
+
+/**
+ * @Annotation
+ * @Target("PROPERTY")
+ */
+final class Groups
+{
+ public $groups;
+
+ public function __construct(array $values)
+ {
+ if (!isset($values['value']) || !is_array($values['value'])) {
+ throw new RuntimeException('$groups must be a array.');
@schmittjoh
schmittjoh Mar 26, 2012 owner

I believe this should be an InvalidArgumentException.

@chregu
chregu Mar 26, 2012

That was copied from Version, maybe it should be changed there too
changed on my side now

@schmittjoh schmittjoh commented on an outdated diff Mar 26, 2012
Serializer/Exclusion/GroupsExclusionStrategy.php
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace JMS\SerializerBundle\Serializer\Exclusion;
+
+use JMS\SerializerBundle\Metadata\ClassMetadata;
+use JMS\SerializerBundle\Metadata\PropertyMetadata;
+
+class GroupsExclusionStrategy implements ExclusionStrategyInterface
+{
+ private $groups = array();
+
+ public function __construct($groups)
@schmittjoh
schmittjoh Mar 26, 2012 owner

I'm not such a huge fan of mixed types, could we move this logic to the Serializer instead and always require an array here?

I think we should also throw an exception if no group is passed.

@schmittjoh schmittjoh commented on an outdated diff Mar 26, 2012
Serializer/Exclusion/GroupsExclusionStrategy.php
+ $this->groups[$group] = true;
+ }
+ }
+ }
+
+ public function shouldSkipClass(ClassMetadata $metadata)
+ {
+ return false;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function shouldSkipProperty(PropertyMetadata $property)
+ {
+ if ($this->groups && $property->groups) {
@schmittjoh
schmittjoh Mar 26, 2012 owner

Can you break this down into several ifs, for example:


if (!$property->groups) {
    return true;
}

foreach ($property->groups as $group) {
    // ...
}

return true;

This makes the different conditions a bit more obvious.

@schmittjoh schmittjoh commented on an outdated diff Mar 26, 2012
Serializer/Serializer.php
@@ -53,6 +54,17 @@ public function setVersion($version)
$this->exclusionStrategy = new VersionExclusionStrategy($version);
}
+
+ public function setGroups($groups)
+ {
+ if (null === $groups) {
@schmittjoh
schmittjoh Mar 26, 2012 owner

Maybe just use if (!$groups) here to also reset the strategy when an empty array is passed.

@schmittjoh schmittjoh and 1 other commented on an outdated diff Mar 26, 2012
Tests/DependencyInjection/JMSSerializerExtensionTest.php
@@ -85,6 +87,21 @@ public function testLoad()
$serializer->setVersion('1.1.1');
$this->assertEquals(json_encode(array('name' => 'bar')), $serializer->serialize($versionedObject, 'json'));
+
+ $this->assertEquals(json_encode(array('foo' => 'foo', 'foobar' => 'foobar', 'bar' => 'bar', 'none' => 'none')),$serializer->serialize($groupsObject, 'json'));
@schmittjoh
schmittjoh Mar 26, 2012 owner

The JMSSerializerExtensionTest is just doing some basic tests to ensure that all the different classes are wired correctly together by the DI container. Can you move these more fine-grained tests to the BaseSerializerTest?

@chregu
chregu Mar 26, 2012

Ok, moved them. But shouldn't the Version tests also be there?

@schmittjoh
Owner

Apart from the few CS fixes, this looks pretty good.

For XML we can use a comma-delimited string which is then turned into an array:

<property groups="foo, bar" />
$groups = preg_split('/\s*,\s*/', $attributeValue);

This is the usual practice in Symfony2.

@chregu
chregu commented Mar 26, 2012

So, hope I caught all the input. XML and YAML driver still to come

@chregu
chregu commented Mar 26, 2012

Finished! ;)

XML, PHP and YAML Drivers are updated, tests for this as well.

From my point of view, this can be merged

@schmittjoh schmittjoh commented on an outdated diff Mar 26, 2012
Serializer/Serializer.php
@@ -53,6 +54,17 @@ public function setVersion($version)
$this->exclusionStrategy = new VersionExclusionStrategy($version);
}
+
+ public function setGroups($groups)
+ {
+ if (!$groups) {
+ $this->exclusionStrategy = null;
+
+ return;
+ }
+
+ $this->exclusionStrategy = new GroupsExclusionStrategy($groups);
@schmittjoh
schmittjoh Mar 26, 2012 owner

An array cast seems to be missing here for people that pass just a string, otherwise we need to type hint the array in the parameter.

@schmittjoh schmittjoh commented on the diff Mar 26, 2012
Serializer/Exclusion/GroupsExclusionStrategy.php
+ foreach ($groups as $group) {
+ $this->groups[$group] = true;
+ }
+ }
+
+ public function shouldSkipClass(ClassMetadata $metadata)
+ {
+ return false;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function shouldSkipProperty(PropertyMetadata $property)
+ {
+
@schmittjoh
schmittjoh Mar 26, 2012 owner

Can you remove this extra line?

@schmittjoh schmittjoh commented on an outdated diff Mar 26, 2012
Serializer/Exclusion/GroupsExclusionStrategy.php
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace JMS\SerializerBundle\Serializer\Exclusion;
+
+use JMS\SerializerBundle\Metadata\ClassMetadata;
+use JMS\SerializerBundle\Metadata\PropertyMetadata;
+
+class GroupsExclusionStrategy implements ExclusionStrategyInterface
+{
+ private $groups = array();
+
+ public function __construct(array $groups)
+ {
+ foreach ($groups as $group) {
@schmittjoh
schmittjoh Mar 26, 2012 owner

How about throwing an exception if an empty array is passed, it does not make sense to me at least?

@schmittjoh schmittjoh commented on the diff Mar 26, 2012
Tests/Serializer/BaseSerializationTest.php
@@ -416,6 +417,29 @@ public function testAccessorOrder()
$this->assertEquals($this->getContent('accessor_order_child'), $this->serialize(new AccessorOrderChild()));
$this->assertEquals($this->getContent('accessor_order_parent'), $this->serialize(new AccessorOrderParent()));
}
+
+ public function testGroups()
+ {
+ $serializer = $this->getSerializer();
+
@schmittjoh
schmittjoh Mar 26, 2012 owner

Can you remove one of these empty lines?

@schmittjoh schmittjoh commented on the diff Mar 26, 2012
Tests/Serializer/BaseSerializationTest.php
+ $groupsObject = new GroupsObject();
+
+ $this->assertEquals($this->getContent('groups_all'), $serializer->serialize($groupsObject, $this->getFormat()));
+
+ $serializer->setGroups(array("foo"));
+ $this->assertEquals($this->getContent('groups_foo'), $serializer->serialize($groupsObject, $this->getFormat()));
+
+ $serializer->setGroups(array("foo", "bar"));
+ $this->assertEquals($this->getContent('groups_foobar'), $serializer->serialize($groupsObject, $this->getFormat()));
+
+ $serializer->setGroups(null);
+ $this->assertEquals($this->getContent('groups_all'), $serializer->serialize($groupsObject, $this->getFormat()));
+
+ $serializer->setGroups(array());
+ $this->assertEquals($this->getContent('groups_all'), $serializer->serialize($groupsObject, $this->getFormat()));
+
@schmittjoh
schmittjoh Mar 26, 2012 owner

Can you remove this empty line as well?

@schmittjoh schmittjoh commented on an outdated diff Mar 26, 2012
Tests/Serializer/JsonSerializationTest.php
@@ -61,6 +61,10 @@ protected function getContent($key)
$outputs['accessor_order_child'] = '{"c":"c","d":"d","a":"a","b":"b"}';
$outputs['accessor_order_parent'] = '{"a":"a","b":"b"}';
$outputs['inline'] = '{"c":"c","a":"a","b":"b","d":"d"}';
+ $outputs['groups_all'] = '{"foo":"foo","foobar":"foobar","bar":"bar","none":"none"}';
+ $outputs['groups_foo'] = '{"foo":"foo","foobar":"foobar"}';
+ $outputs['groups_foobar'] = '{"foo":"foo","foobar":"foobar","bar":"bar"}';
+
@schmittjoh
schmittjoh Mar 26, 2012 owner

This one as well :)

@schmittjoh
Owner

Nothing big on my side, just a few CS issues.

Could you also update the configuration references (annotations, xml, yaml) with the new feature?

@Seldaek
Seldaek commented Mar 27, 2012

I'm not sure how it ended up implemented, but I agree with @mvrhov that if both are present it should first exclude/include based on the old-style policy, then apply groups to what's left.

@chregu
chregu commented Mar 27, 2012

It's an ExclusionStrategy (like the Version thingie is). I guess it does exactly that, first exclude/include, then apply any ExclusionStrategy one has defined

chregu added some commits Apr 11, 2012
@chregu
chregu commented Apr 11, 2012

Added some docs and the WS fixes were done by @lsmith77

@schmittjoh schmittjoh merged commit bbaee82 into schmittjoh:master Apr 12, 2012
@schmittjoh
Owner

Thanks for your work, merged!

@jalliot
jalliot commented Apr 12, 2012

@schmittjoh IIUC we can't use several exclusion policies at the same time with the current approach, can we?
So something like that for instance (and maybe we can assume some @Exclude/@Expose have been set on the object) would not give the expected result:

<?php

$serializer->setVersion('1.0');
$serializer->setGroups(array('list'));
$serializer->serialize(new SomeVersionnedAndGroupedObject(), 'json'); 

It would be interesting to have some sort of chained exclusion policy which would handle multiple policies. What do you think?

@asm89
asm89 commented Apr 12, 2012

Really cool feature. :) Thanks @chregu!

@schmittjoh
Owner

@jalliot, yes, we can re-add such a strategy. I removed it at some point because we only had one exclusion strategy, so it was not necessary then.

The old implementation is here:
https://github.com/schmittjoh/JMSSerializerBundle/blob/93dc77ddada839d4c7e5fb3747c6ac47d12584f9/Serializer/Exclusion/DisjunctExclusionStrategy.php

We should probably also add some caching for better performance.

@ruimarinho

@chregu, great work, thanks for such a useful feature!

@schmittjoh does that explain the behavior I've experienced by having a relation set by a property ($friends) with @Groups({"foo", "bar"}) on the main class (User) return an empty array, even when the relation itself (Friend) has @ExclusionPolicy('all') with @Expose on several properties?

The only way I had around this mixed strategies environment was using the same groups on the relation properties I wanted to expose. Is this expected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment