added support for JMS Serializer GroupsExclusionStrategy #156

Merged
merged 2 commits into from May 12, 2013

Projects

None yet

9 participants

@fieg
fieg commented Mar 25, 2013

A proposal to support the GroupsExclusionStrategy of the JMS Serializer.

From the updated readme in this PR:

When using a class with JMS Serializer metadata, you can use exclude groups by using this syntax: input="Acme\YourBundle\Entity\User@update,public". In this case the groups 'update' and 'public' are used.

@willdurand
Collaborator

looks good to me

@Seldaek
Member
Seldaek commented Mar 25, 2013

To me too, except that the annotation syntax is a bit strange and not very future proof. I don't have a better idea though.

@pkruithof pkruithof commented on an outdated diff Mar 25, 2013
@@ -101,7 +101,9 @@ The following properties are available:
* `filters`: an array of filters;
* `input`: the input type associated to the method, currently this supports Form Types, and classes with JMS Serializer
- metadata, useful for POST|PUT methods, either as FQCN or as form type (if it is registered in the form factory in the container)
+ metadata, useful for POST|PUT methods, either as FQCN or as form type (if it is registered in the form factory in the container).
+ When using a class with JMS Serializer metadata, you can use exclude groups by using this syntax:
@pkruithof
pkruithof Mar 25, 2013

The term "exclude" implicates that these groups are not used. I understand the relation to the ExclusionStrategy from the Serializer, but I think that term is ambiguous also. It would be more clear if it said: "you can use specific groups by using this syntax".

@pkruithof

@Seldaek an alternative would be to use an additional annotation, something like inputgroup=blah. But I think it makes more sense to suffix the input line.

Maybe a # or :: would make more sense as a separator? I don't know what the most commonly used notation for class/property definitions is, but it seems to me that would be useful here too.

@fieg
fieg commented Mar 25, 2013

The notation is indeed arguable. Another suggestion might be:

input={
    "class"="Acme\Bundle\Entity\User", 
    "groups"="update, public"
}

This makes it also extendable in the future.

@pkruithof

👍 for that notation

@stof stof commented on an outdated diff Mar 25, 2013
Parser/JmsMetadataParser.php
@@ -80,6 +86,9 @@ protected function doParse($className, $visited = array())
throw new \InvalidArgumentException(sprintf("No metadata found for class %s", $className));
}
+ $context = new NavigatorContext(GraphNavigator::DIRECTION_SERIALIZATION, 'json'); //TODO: the exclusionStrategy has a hard dependency on this, despite it isn't even used :(
+ $exclusionStrategy = new GroupsExclusionStrategy($groups);
@stof
stof Mar 25, 2013 Contributor

What about the VersionExclusionStrategy ?

@pylebecq
Contributor

This would be a great feature. And as @stof said, VersionExclusionStrategy would be really nice too.
For the annotation, another proposal, which is a little tweak of @fieg last proposal :

// Supporting the old syntax
input="Acme\Bundle\Entity\User"
// Which would be equals to
input={
    "class"="Acme\Bundle\Entity\User",
    "groups"={"Default"}
}
// And of course could be customized by typing
input={
    "class"="Acme\Bundle\Entity\User",
    "groups"={"Update", "Public"}
}

The only difference here would be to declare groups as an array instead of a string with separators. What dou you think ?

@fieg
fieg commented Mar 27, 2013

Good suggestions @stof and @pylebecq. Will look into it.

@fieg
fieg commented Mar 27, 2013

Ok, I tackled the input/output notation.

Does anyone want to help with the VersionExclusionStrategy support? Let me know so I can add you as a contributor to the feature branch. I'm running a little short on time here...

@pylebecq pylebecq commented on an outdated diff Mar 28, 2013
Parser/JmsMetadataParser.php
@@ -80,6 +87,9 @@ protected function doParse($className, $visited = array())
throw new \InvalidArgumentException(sprintf("No metadata found for class %s", $className));
}
+ $context = new NavigatorContext(GraphNavigator::DIRECTION_SERIALIZATION, 'json'); //TODO: the exclusionStrategy has a hard dependency on this, despite it isn't even used :(
@pylebecq
pylebecq Mar 28, 2013 Contributor

NavigatorContext won't exist anymore in Serializer 0.12.
It was renamed to Context in schmittjoh/serializer@ffb2d5d and split into two classes in schmittjoh/serializer@0207222

@pylebecq
Contributor

@fieg If I have time this weekend, I will try to do something about the version exclusion strategy.

@pylebecq
Contributor

Ok, finally I took some time tonight to add the support of the VersionExclusionStrategy. It will show up here as soon as @fieg merges the PR.

My last concern is related to my previous comment about the removal of NavigationContext in Serializer 0.12.
I don't know how to handle this. Should we update the composer.json file to forbid usage of Serializer >= 0.12 or should we update the code to use the new classes of Serializer 0.12 and make this bundle only compatible with Serializer 0.12 ? Or anything else ?

@stof
Contributor
stof commented Mar 29, 2013

As 0.12 is stable as of yesterday, forbidding it is not an option IMO

@MaksSlesarenko

Is this relative to FosRestBundle SerializerGroups option #119 ?

@fieg
fieg commented Apr 2, 2013

Does anyone have some thoughts about how to proceed with this PR?

@fieg
fieg commented Apr 3, 2013

Hi, I rebased, fixed the unit tests and refactored the NavigatorContext to support the new Serializer version. Hopefully this can get merged now.

@Seldaek Seldaek commented on an outdated diff Apr 3, 2013
Extractor/ApiDocExtractor.php
+ // normalize strings
+ if (is_string($input)) {
+ $input = array('class' => $input);
+ }
+
+ // normalize groups
+ if (isset($input['groups']) && is_string($input['groups'])) {
+ $input['groups'] = array_map('trim', explode(',', $input['groups']));
+ }
+
+ // normalize version
+ if (!array_key_exists('version', $input)) {
+ $input['version'] = null;
+ }
+
+ return $input += $defaults;
@Seldaek
Seldaek Apr 3, 2013 Member

The = is kinda superfluous here.

@Seldaek Seldaek and 1 other commented on an outdated diff Apr 3, 2013
Extractor/ApiDocExtractor.php
+ );
+
+ // normalize strings
+ if (is_string($input)) {
+ $input = array('class' => $input);
+ }
+
+ // normalize groups
+ if (isset($input['groups']) && is_string($input['groups'])) {
+ $input['groups'] = array_map('trim', explode(',', $input['groups']));
+ }
+
+ // normalize version
+ if (!array_key_exists('version', $input)) {
+ $input['version'] = null;
+ }
@Seldaek
Seldaek Apr 3, 2013 Member

This block seems useless, if the key is not defined, then it'll be added from the defaults already as far as I can see.

@pylebecq
pylebecq Apr 3, 2013 Contributor

Yep, you're right. Sorry, my mistake.

@Seldaek
Member
Seldaek commented Apr 3, 2013

@fieg the README change is out of date I think, and I have one more question: it now only supports the latest Serializer version right? Is it possible to support older ones too without too much hassle? It'd be great, but if not that's fine, but then please change the composer.json to reflect this (I think the conflict should be conflicting with <0.12 now?).

Besides this stuff and the other couple comments I wrote, I think it's good to merge.

@fieg
fieg commented Apr 3, 2013

Good comments, I will try to look into it later today.

@willdurand
Collaborator

ping @fieg

@pylebecq
Contributor

I'm not completely satisfied by the implementation of the version exclusion strategy, because every time you introduce a new version you have to update all your ApiDoc annotations. Also, by doing this, there is no possibility to get the documentation for a specific version.
Don't you think a new column like the "required" column would be more appropriate ? Instead of excluding the property, we could show the version that applies to the properties. For example : >=1.0,<2.0

@fieg
fieg commented Apr 18, 2013

Thanks for the ping @willdurand.

I updated according to your comments @Seldaek and did a rebase.

@fieg
fieg commented Apr 18, 2013

Hmm, some whitespace got messed up. Will fix.

@fieg
fieg commented Apr 24, 2013

@Seldaek do you see change to review and merge this?

@Seldaek
Member
Seldaek commented Apr 24, 2013

I'd rather have @willdurand review/merge it because it's quite involved and I don't know this code in depth.

That said, I think @pylebecq had a point.. This should be carefully considered.

@fieg
fieg commented Apr 24, 2013

My suggestion is that we merge this PR as is and then @pylebecq (and others) could improve the Version-part of this feature in some other PR? It would be a shame if this would stall the GroupExclusion feature unnecessarily.

@pylebecq
Contributor

I agree with @fieg, let's not block the group exclusion strategy feature but before merging I think we should remove the work on the version exclusion strategy because the feature seems wrong to me and don't deserve to land in the code base the way it is implemented for now.

@fieg
fieg commented Apr 25, 2013

@pylebecq reworked the version support 👍 . @willdurand could you please review it?

@willdurand willdurand and 1 other commented on an outdated diff Apr 26, 2013
Extractor/ApiDocExtractor.php
@@ -266,9 +266,11 @@ protected function extractData(ApiDoc $annotation, Route $route, \ReflectionMeth
if (null !== $input = $annotation->getInput()) {
$parameters = array();
+ $normalizedInput = $this->normalizeInputOutputParameter($input);
@willdurand
willdurand Apr 26, 2013 Collaborator

normalizeParameters()?

@fieg
fieg May 7, 2013

I think normalizeParameters would be to generic; it only normalizes the type of parameter that is used with the input and output option. How about normalizeClassParameter as the type of parameter is best described as a class?

@willdurand
willdurand May 7, 2013 Collaborator

I think normalizeParameters() is fine. Do we have other parameters?

@fieg
fieg May 7, 2013

Yes, description for example. Also the method only normalizes a single parameter.

@willdurand
willdurand May 7, 2013 Collaborator

ok for normalizeClassParameter then

@willdurand willdurand and 1 other commented on an outdated diff Apr 26, 2013
Extractor/ApiDocExtractor.php
+ $defaults = array(
+ 'class' => '',
+ 'groups' => array(),
+ );
+
+ // normalize strings
+ if (is_string($input)) {
+ $input = array('class' => $input);
+ }
+
+ // normalize groups
+ if (isset($input['groups']) && is_string($input['groups'])) {
+ $input['groups'] = array_map('trim', explode(',', $input['groups']));
+ }
+
+ return $input + $defaults;
@willdurand
willdurand Apr 26, 2013 Collaborator

please, don't do that

@fieg
fieg May 7, 2013

Can you be more specific?

@willdurand
willdurand May 7, 2013 Collaborator

don't merge arrays using the + sign.

@blaugueux
Contributor

ping @fieg please :)

@remyLemeunier

+1

@willdurand
Collaborator

ping @fieg

@fieg fieg and 1 other commented on an outdated diff May 7, 2013
Formatter/AbstractFormatter.php
@@ -113,7 +113,13 @@ protected function processAnnotation($annotation)
}
if (isset($annotation['response'])) {
- $annotation['response'] = $this->compressNestedParameters($annotation['response']);
+ $response = $this->compressNestedParameters($annotation['response']);
+ foreach ($response as $name => &$info) {
+ $info['sinceVersion'] = array_key_exists('sinceVersion', $annotation['response'][$name]) ? $annotation['response'][$name]['sinceVersion'] : null;
@fieg
fieg May 7, 2013

@pylebecq I'm running into an issue here with nested parameters:

Notice: Undefined index: credit[amount] in vendor/nelmio/api-doc-bundle/Nelmio/ApiDocBundle/Formatter/AbstractFormatter.php line 118
@pylebecq
pylebecq May 7, 2013 Contributor

I'll fix it tonight and send you a PR as usual. Thanks.

@blaugueux
Contributor

@fieg can you rebase and squash your commits to have a better readability of your PR please

@fieg
fieg commented May 8, 2013

I rebased (and resolve some conflicts), but now run into some failing tests which I'm not certain how to fix. @pylebecq I think you might have a better insight in how to fix these tests. You can check the result of the rebase here: https://github.com/fieg/NelmioApiDocBundle/commits/jms-groups-support-rebased

@blaugueux I've never squashed commits before, should all commits be squashed in a single commit? How would this improve the readability?

@blaugueux
Contributor

@fieg This will group your 16 commits into 1. It will be more easy to rebase for you and better for us to read :) Here is a small tuto: http://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/

@fieg
fieg commented May 8, 2013

@blaugueux Would author information for the commits of @pylebecq be maintained?

@willdurand what do you think about this squashing of commits?

@pylebecq
Contributor
pylebecq commented May 8, 2013

@fieg I fixed the tests, this was no big deal, probably a mistake when resolving conflics.

I pushed in your rebased branch : https://github.com/fieg/NelmioApiDocBundle/commits/jms-groups-support-rebased .
I also squashed our commits resulting in two commits : the first commit is yours and is about the implementation of the group exclusion strategy. The other commit is mine and is about the version support. I pushed the squashed commits in an other branch : https://github.com/fieg/NelmioApiDocBundle/commits/jms-groups-support-rebased-squashed .

The only difference between the two branches is two use statements not used anymore I removed in the squashed branch :

audrey:ApiDocBundle pierreyves$ git diff origin/jms-groups-support-rebased..origin/jms-groups-support-rebased-squashed
diff --git a/Parser/JmsMetadataParser.php b/Parser/JmsMetadataParser.php
index add1e3d..2edc28e 100644
--- a/Parser/JmsMetadataParser.php
+++ b/Parser/JmsMetadataParser.php
@@ -12,8 +12,6 @@
 namespace Nelmio\ApiDocBundle\Parser;

 use JMS\Serializer\Exclusion\GroupsExclusionStrategy;
-use JMS\Serializer\GraphNavigator;
-use JMS\Serializer\NavigatorContext;
 use JMS\Serializer\SerializationContext;
 use Metadata\MetadataFactoryInterface;
 use Nelmio\ApiDocBundle\Util\DocCommentExtractor;

I did my best to squash like @blaugueux asked and to keep the author information for the both of us. You can view the two commits to see if the split seems fair (it should) and if everything is ok for everyone the last thing to do will be to push the jms-groups-support-rebased-squashed on jms-groups-support (you will need to use --force).

Don't hesitate to contact me via private messages if you have any question.

@willdurand
Collaborator

Ok so, can we have a clean PR now? A rebase is still needed, I don't really care about the squashing commits.
You should update the doc though.

@fieg
fieg commented May 11, 2013

@pylebecq you're the best :) I pushed jms-groups-support-rebased-squashed onto jms-group-support.

I believe @willdurand is pointing at the documentation about the version feature of this PR. @pylebecq if you could only fix this last thing, then we are all set and this can get merged.

@willdurand
Collaborator

This needs to be rebased btw

@pylebecq
Contributor

I updated the docs and rebased / fixed tests / squashed / killed some kittens again. :)

@willdurand willdurand merged commit ce1b40e into nelmio:master May 12, 2013
@willdurand
Collaborator

thanks!

@blaugueux
Contributor

@willdurand @Seldaek can you create a tag with this new feature?

@fieg fieg deleted the fieg:jms-group-support branch Oct 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment