Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADR implementation, JSON-LD decoupling and content negociation support #191

Merged
merged 6 commits into from Jul 29, 2015

Conversation

@dunglas
Copy link
Member

dunglas commented Jul 27, 2015

  • Now uses a variation of the ADR pattern done with Symfony (I'll make a blog post about that soon)
  • There is no more coupling to JSON-LD and Hydra: the bundle can now output any format including but not limited to raw JSON, XML or HAL (I'll add an exemple of a custom XML serializer in the tests).
  • To complete the previous feature, I've added content negotiation capabilities using @willdurand's Negotiation library. (I'll add an exemple in tests too). It means that you can request various representations of the same resource using the Accept header (of course the requested format must be supported by one of the registered normalizer).

@pmjones do you think you can find some time to review this ADR implementation and tell me if it is in scope with the pattern you designed?

@dunglas dunglas added the enhancement label Jul 27, 2015
/**
* Gets an item using the data provider. Throws a 404 error if not found.
*
* @param DataProvider $dataProvider

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Jul 27, 2015

Contributor

DataProviderInterface(and missing use statement)

return;
}
// Use the Symfony reques format if available and applicable

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Jul 27, 2015

Contributor

typo (request)

}
switch ($request->getMethod()) {
case 'POST':

This comment has been minimized.

Copy link
@ogizanagi
@ogizanagi

This comment has been minimized.

Copy link
Contributor

ogizanagi commented Jul 27, 2015

It looks great !
The ADR pattern is cool, but I'm glad you let the possibility to use Controllers classes outside (in a big application, ADR might be heavy, and has some drawbacks).

@dupuchba

This comment has been minimized.

Copy link
Contributor

dupuchba commented Jul 27, 2015

@dunglas, by implementing the ADR pattern you got rid of Controllers. I don't see the the big gain is this, can you explain plz ?

@dunglas

This comment has been minimized.

Copy link
Member Author

dunglas commented Jul 27, 2015

@dupuchba take a look to the link I mentioned in the description: https://github.com/pmjones/adr

General gains are:

  • Easier to unit test
  • Explicit dependencies
  • Better respect of the Single Responsibility Principle / smaller code units

Here, it comes with a bigger immediate gain: it makes it easy to decouple from JSON-LD and Hydra. Now you can return as format as you want with ApiBundle such as XML or Protobuf without touching the core.

And as @ogizanagi pointed, you can still use standard Symfony controllers for your userland code if you like.

@pmjones

This comment has been minimized.

Copy link

pmjones commented Jul 27, 2015

Hi @dunglas -- looking at it now!

$this->eventDispatcher->dispatch(Events::PRE_CREATE_VALIDATION, new DataEvent($resourceType, $data));
$violations = $this->validator->validate($data, null, $resourceType->getValidationGroups());

This comment has been minimized.

Copy link
@pmjones

pmjones Jul 27, 2015

While collection of the input is an Action task, validation of that input is not. The Domain should be validating/sanitizing its inputs. (I see this repeated below as well.)

A good rule-of-thumb is this: when the Action has a conditional, it's probably doing too much.

This comment has been minimized.

Copy link
@dunglas

dunglas Jul 27, 2015

Author Member

What kind of refactoring do you suggest considering that we don't have the hand on the user domain and that automatic validation using Symfony annotation is a very convenient feature.

Do you consider throwing an event in the action and executing the validation in a listener (that can be removed/changed...) is a better solution?

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Jul 27, 2015

Contributor

Do you consider throwing an event in the action and executing the validation in a listener (that can be removed/changed...) is a better solution?

That could be great IMO. It would allow to hook before AND after validation (with a single event)

This comment has been minimized.

Copy link
@pmjones

pmjones Jul 27, 2015

I don't know Symfony well enough to offer a Symfony-approved solution.

My own preference has been to have my Domain layer return "payload" objects, that carry not only the Domain entities, but also a status code or status object that says what the result "means." That means my Domain layer can return a "NotValid" payload when inputs are invalid; in turn, the Responder layer can look at that and determine what kind of Response to build.

Hope that makes sense.

This comment has been minimized.

Copy link
@dunglas

dunglas Jul 27, 2015

Author Member

Here it's very similar IMO: despite the name ($validator->validate()), the validation is only triggered in the action layer but is done in the domain layer. The Symfony validator will successively call all your customs (or builtin) validators (in config files or with annotations or entities) and returns a list of validation failure (the payload of the exception here). Then exception is caught by a responder that will convert it in Hydra error.

IMO we can say that validation is done in the domain layer here. It is only triggered in the action but all the validation logic lies in the domain (and if no validator are defined, it will do nothing).

An alternative solution can be to move the triggering itself in the domain layer (in DataProvider) but it has a big drawback: validation will be triggered in all cases (even in GET methods). That mean a performance decrease and possible unwanted side effects in case data retrieved from the persistence system violate some validation rules (this is really bad practice but it happens).

I'm not sure of what to do here. Throwing an event has another advantage: it allows to use easily an alternative validator that the Symfony one.

This comment has been minimized.

Copy link
@pmjones

pmjones Jul 27, 2015

the validation is only triggered in the action layer but is done in the domain layer.

(/me shrugs) I can't say too much about how Symfony does stuff, as I am not a Symfony expert.

Even so, it still strikes me that the Domain should be doing the validation work, not the overall framework. When the validation is tied to the framework, the Domain is tied to the framework, and so you never really achieve a full separation/decoupling.

But baby steps, baby steps -- what you have here is already better than most controllers.

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Jul 27, 2015

Contributor

IMO we can say that validation is done in the domain layer here. It is only triggered in the action but all the validation logic lies in the domain (and if no validator are defined, it will do nothing).

Agree. The validator is not tied to the framework and is kind of part of the Domain layer.

Throwing an event has another advantage: it allows to use easily an alternative validator that the Symfony one.

I like this idea 👍

This comment has been minimized.

Copy link
@dupuchba

dupuchba Jul 27, 2015

Contributor

👍 (on the down side, we loose immediate readability)

@dunglas

This comment has been minimized.

Copy link
Member Author

dunglas commented Jul 27, 2015

@pmjones thank you!
@ogizanagi thanks for the review, issues fixed.

@pmjones

This comment has been minimized.

Copy link

pmjones commented Jul 27, 2015

Hi again @dunglas -- I gave it a quick 5-minute scan, and the only obvious "wrong" thing I saw was that a couple of actions do validation. My opinion is that validatiing/sanitizing of inputs is more properly a domain concern, not an action concern.

I was not able to see where the response actually gets built, but my guess is that it's part of the event dispatching system. As long as the action, domain, and responder portions are separated from each other, your work fits the pattern. As far as I can tell, this looks good!

p.s. Eventually, you're going to realize that your actions all look the same: collect input (extract Request attributes to a ResourceType), retrieve a Domain result (get an Item from a DataProvider using the ResourceType), and pass the Domain result to a Responder (pass the Item to the EventDispatcher). At that point you'll be able to collect all the differences to properties and/or injected dependencies, and the number of action classes will become greatly reduced (maybe even down to just 1 generic action class).

@ogizanagi

This comment has been minimized.

Copy link
Contributor

ogizanagi commented Jul 27, 2015

I was not able to see where the response actually gets built, but my guess is that it's part of the event dispatching system.

@pmjones : The response is indeed built in the ResponderViewListener

@dunglas

This comment has been minimized.

Copy link
Member Author

dunglas commented Jul 27, 2015

Thank you very much for the review and advices @pmjones!

I've trouble seing how to merge all actions in a single generic one. The process for all actions of the CRUD is a bit different. We can achieve this in a single class but it will lead to a large number of if/else in the action (collection or item? read or write operation?...). In the end having separate classes for each using dependencies and traits for everything that can be factorized sounds better to me what do you think?

@pmjones

This comment has been minimized.

Copy link

pmjones commented Jul 27, 2015

I've trouble seing how to merge all actions in a single generic one.

Right -- it's not an immediate goal to seek, it's something I think you are likely to notice over time, especially as you move more activity down into the domain layer, or over into the responder layer.

@dunglas

This comment has been minimized.

Copy link
Member Author

dunglas commented Jul 27, 2015

For those interested in content negotiation, I've just just a demo / test to the PR: https://github.com/dunglas/DunglasApiBundle/pull/191/files#diff-af37d9fff8b64adda0485128ebf627c1R1. It could be interesting to move the XML responder to the core or at least in a dedicated doc entry. What do you think?

@ogizanagi

This comment has been minimized.

Copy link
Contributor

ogizanagi commented Jul 27, 2015

@dunglas : Great ! I think it should go to a dedicated doc entry, but I don't feel it's needed in the core due to the bundle purpose at first (which is using JsonLd and Hydra).
But it could be useful for very specific cases (not especially the xml responder), or allow to rely on other serializers/normalizers, do not deal with jsonld/hydra vocab and expose plain objects, ...

@dunglas

This comment has been minimized.

Copy link
Member Author

dunglas commented Jul 27, 2015

The bundle has been renamed some times ago to reflect that it can support other formats (any format). Maybe that this XML support is too limited to be included in core but I don't exclude to add Turtle, RDF-XML, HAL, SIREN or Protobuf support for instance in the core. This not a problem because both can be used together thanks to content negotiation.

@dupuchba

This comment has been minimized.

Copy link
Contributor

dupuchba commented Jul 27, 2015

@dunglas I think it can be a huge success :-)

@ogizanagi

This comment has been minimized.

Copy link
Contributor

ogizanagi commented Jul 27, 2015

Right, but one step after another ;)
As dunglas said, the given Xml responder example is too limited, and thus has no benefits being in the core as is regarding the current goals of this bundle.
I think it is a very nice aim nonetheless. "ApiPlatform" would already be a perfect name for this.

@dupuchba

This comment has been minimized.

Copy link
Contributor

dupuchba commented Jul 27, 2015

I am trying to be positive !!!

@dunglas

This comment has been minimized.

Copy link
Member Author

dunglas commented Jul 27, 2015

Two steps further:

  1. Externalized the validation triggering in an event listener (it allows to use another validator than the Symfony one and to validate data in the domain layer as suggested by @pmjones)
  2. Consequence of 1: the custom event system is not necessary anymore. Everything can now be handled using standard Symfony kernel events (code simplification and more powerful than before). Only one drawback: the name of the Symfony event used (kernel.view) sounds a bit strange for that use but I can live with it.

ping @sroze @theofidry

@theofidry

This comment has been minimized.

Copy link
Member

theofidry commented Jul 27, 2015

Great work. For the even system it's not much so no problem, a little mention in the changelog is welcomed. I'll take a deeper look tomorrow to see what's going to change.

@pmjones

This comment has been minimized.

Copy link

pmjones commented Jul 27, 2015

Nicely done. The Post and Put actions still look a little clunky to me, but whatever -- it's still an improvement. (I think you are starting to see what I meant earlier about "all your actions looking the same" -- Delete, GetCollection, and GetItem are very similar now. :-)

<service id="api.listener.view.validation" class="Dunglas\ApiBundle\EventListener\ValidationViewListener">
<argument type="service" id="validator" />

<tag name="kernel.event_listener" event="kernel.view" method="onKernelView" priority="20" />

This comment has been minimized.

Copy link
@dupuchba

dupuchba Jul 28, 2015

Contributor

Why 20 ? I am saying that because for sf standard edition that we use for api-platform, There is only one other listener for the kernel.viewwith a 0 default priority
capture d ecran 2015-07-28 a 09 44 08

This comment has been minimized.

Copy link
@dunglas

dunglas Jul 28, 2015

Author Member
  • validation: 20
  • Doctrine: 10
  • Responder: 0

This comment has been minimized.

Copy link
@dupuchba

dupuchba Jul 28, 2015

Contributor

Ok my bad, I didn't see the other one

@ogizanagi

This comment has been minimized.

Copy link
Contributor

ogizanagi commented Jul 28, 2015

I personally don't like so much not having custom events dispatched directly in concerned actions:

  1. We loose "accuracy". It's not as easy to identify the event and which action triggered it.
    A newcomer will not easily understand that the controllerResponse is the resource item(s) where a getData method is simpler. Same with $event->getRequest()->attributes->get('_resource_type') and getResource.
  2. ManagerViewListener and ValidationViewListener will always be called for userland actions (if we use the ADR pattern instead of controller classes). It might not always be wished. I think it also brings some kind of "magic" at this point.

I actually like the ManagerViewListener and ValidationViewListener idea, but if it relies on dunglas_api dedicated events. :/

@dupuchba

This comment has been minimized.

Copy link
Contributor

dupuchba commented Jul 28, 2015

@ogizanagi for point 1, a new comer can still php app/console debug:event-dispatcher which has been specifically dedicated for that.
Concerning the "magic" part of this, it's indeed a drawback of using listeners this way. We could always write a doc about "how things work" internally :-).

@ogizanagi

This comment has been minimized.

Copy link
Contributor

ogizanagi commented Jul 28, 2015

for point 1, a new comer can still php app/console debug:event-dispatcher which has been specifically dedicated for that.

Yes, but something like app/console debug:event-dispatcher api.pre_update is more accurate and obvious than app/console debug:event-dispatcher kernel.view.

However, we don't need as much events as before. A simple dunglas_api.action event might be sufficient.

@dunglas

This comment has been minimized.

Copy link
Member Author

dunglas commented Jul 28, 2015

@ogizanagi I understand your concerns about readability but the custom event system has other drawbacks:

  • more code to maintain and test for no real reason (except the naming)
  • more chances to have bugs in that subsystem
  • performance decrease (more events dispatched and handled for no real reason)
  • less powerful than native HttpKernel events (no access to the request and the response)

Validation and Doctrine listeners are called at every request but they return very early if they are not applicable (not the good HTTP method). It doesn't impact performance (at least less than dispatching a new event IMO).

For the readability / convenience issue I think that a doc with a pretty graphic will be better than a good name. We need the doc anyway even if we keep custom events.

@dupuchba

This comment has been minimized.

Copy link
Contributor

dupuchba commented Jul 28, 2015

👍 with @dunglas on this, IMO, pros are quite thin to use a custom event system. Besides, as we said, if a user wants to see how things work internally we should help him with a nice doc on internals.

@dunglas

This comment has been minimized.

Copy link
Member Author

dunglas commented Jul 28, 2015

@pmjones: I got your point now and I tried to create an unique generic action. I just pushed it. Indeed it allows to avoid some code duplication. What do you think about it?

@@ -23,7 +23,8 @@
"doctrine/inflector": "~1.0",
"doctrine/doctrine-bundle": "~1.2",
"dunglas/php-property-info": "~0.2",
"phpdocumentor/reflection": "^1.0.7"
"phpdocumentor/reflection": "^1.0.7",
"willdurand/negotiation": "~1.3"

This comment has been minimized.

Copy link
@willdurand

willdurand Jul 28, 2015

~1.4 would be more accurate now ;-)

This comment has been minimized.

Copy link
@dunglas

dunglas Jul 28, 2015

Author Member

Great! It means that I can remove some code in the compiler pass too.

@pmjones

This comment has been minimized.

Copy link

pmjones commented Jul 28, 2015

@dunglas:

I got your point now and I tried to create an unique generic action. I just pushed it. Indeed it allows to avoid some code duplication. What do you think about it?

This is a bit of a step backwards. The __invoke() method now does routing based on the request, and combines what should be separate actions. The rule-of-thumb in actions is that if you have a conditional (if or switch/case) the action is doing too much.

Remember the specific duties of the action: collect input, send input to a Domain layer, get back a Domain result, send the result to a Responder. Essentially, every action should look something like this:

$resourceType = $request->attributes->get('_resource_type');
$method = $this->domainMethod; // getCollection, getItem, etc
$result = $this->dataProvider->$method($resourceType);
return $result;

That really is all. Leave validation, exception-throwing, and everything else to the Domain layer; leave anything related to formatting to the Responder layer. Each action should be so simple, so dumb, that practically nothing interesting happens in it.

@dunglas

This comment has been minimized.

Copy link
Member Author

dunglas commented Jul 28, 2015

It makes sense I'll revert the last commit but I fail to understand what you were thinking about with the one-class action.

@dupuchba

This comment has been minimized.

Copy link
Contributor

dupuchba commented Jul 28, 2015

@dunglas IMO, @pmjones was more talking about natural refactoring while adding more action in a parent class for example. Right ?

@pmjones

This comment has been minimized.

Copy link

pmjones commented Jul 28, 2015

I fail to understand what you was thinking about with the one-class action.

(/me nods) That's OK -- it will reveal itself to you in time.

Essentially what will happen is that, because each different action has exactly the same steps, but uses only (1) a different set of inputs for the the domain, and (2) a different domain method or domain object, you will end up seeing a way to have only one class, maybe a base class that you end up extending, and inject different input-collection objects and domain objects when you instantiate it. You might see what I mean if you look at Arbiter.

But really, all of that is for later. For now, concentrate on getting a good separation of layers.

@pmjones

This comment has been minimized.

Copy link

pmjones commented Jul 28, 2015

@dupuchba Correct. :-)

@dunglas

This comment has been minimized.

Copy link
Member Author

dunglas commented Jul 28, 2015

Thanks for the feedback @pmjones. I understand better now.

@dunglas dunglas mentioned this pull request Jul 29, 2015
@dunglas dunglas added this to the 1.0.0 milestone Jul 29, 2015
dunglas added a commit that referenced this pull request Jul 29, 2015
ADR implementation, JSON-LD decoupling and content negociation support
@dunglas dunglas merged commit 79bba07 into master Jul 29, 2015
3 checks passed
3 checks passed
Scrutinizer 4 new issues, 41 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@dunglas dunglas deleted the adr branch Jul 29, 2015
@dunglas dunglas mentioned this pull request Jul 29, 2015
2 of 17 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.