api-platform / core

The server component of API Platform: hypermedia and GraphQL APIs in minutes
https://api-platform.com
MIT License
2.43k stars 866 forks source link

[Proposition] Add support of workflow with @ApiWorkflow #1767

Closed pierre-H closed 5 years ago

pierre-H commented 6 years ago

Hello !

First, thanks again for your great work ! Api Platform saves me hours of coding and now every web project in my company is based on Api Platform ! Thank you !

I need to use Symfony Workflow and it would be great to add an automatic support of it in Api Platform : I mean every time I post an entity, Api Platform would check if the entity can change state with $workflow->can($post, 'to_review'); .

This can be done with an annotation :

/**
 * @ApiWorkflow(name="workflowName")
 * Note : (the workflow name is needed only if there are multiple workflows on the same entity
 */
private $state

If the workflow::can() returns false, Api Platform would throw an exception.

What do you think ?

dunglas commented 6 years ago

Thanks for the nice feedback :) Can't we extend the existing access_control attribute to use add a workflow variable in the expression?

pierre-H commented 6 years ago

That's not a bad idea !

With the access_control attribute, workflow events could not be triggered since we neede to use $workflow->apply() (another feature that I didn't write in my original post). It would be great that only puting an update to Api Platform, the new state is checked and all workflow events are triggered ...

What do you think ?

pierre-H commented 6 years ago

Technically, we could add the workflow variable in the expression but I don't what to write if I have multiple workflows on the same entity. Also, using access_control isn't user friendly as an ApiWorkflow annotation and since the check has to be only on POST and PUT operations, if we add it on the general ApiResource attribute, the check will be every time we do a GET so it would be better to use operation.access_control ApiResource attribute but that's not user friendly.

pierre-H commented 6 years ago

@dunglas tell me if it's worth to propose a PR I yes, I will try to do it

dunglas commented 6 years ago

It is worth it, but use an attribute instead of a new annotation, it has several benefits including:

And I'll propose a PR soon to ease the use of attributes with annotations (all unknown to level annotation's property will be considered as an attribute).

pierre-H commented 6 years ago

Ok,

I've got then few questions : If we use attribute, we should be able to configure a message that is specific to the workflow error but if we use the Access Control Message, then it will be hard to distinguish between the security error and the workflow error. How could we fix that ? Shall we create a workflowattribute with a workflow_message attribute ?

The $workflow->can() will be called before the denormalization of the entity but when should the $workflow->apply() be called (for workflow events) ?

dunglas commented 6 years ago

Shall we create a workflowattribute with a workflow_message attribute ?

Looks good to me.

Are you sure that both shouldn't be called after? The workflow instance will need the deserialized object to check if it can execute the transition isn'it?

pierre-H commented 6 years ago

For me the workflow needs the original object and the new value.

Example :

Before the http call we have this post:

{
    "message": "This is the message",
    "state": "unpublished"
}

I want to post this :

{
    "message": "This is the modified message",
    "state": "published"
}

The workflow needs to check if from unpublished it can go to published. So we need the 'old' object and the new value. So the transition should be done, you're right, after the denormalization of the http content so we have the new value of the state but before the update of the object so we can check if it's ok.

soyuka commented 6 years ago

How I deal with workflows in ApiPlatform

  1. DependencyInjection\Compiler\WorkflowPass:
    • Gets every class that supports a workflow and use them as argument for WorkflowOperationResourceMetadataFactory
  2. Metadata\Resource\Factory\WorkflowOperationResourceMetadataFactory:
    • Adds a PATCH method on $url/state (where $url is the original ITEM url - example /product/1)
    • Adds a GET method on $url/state
  3. EventListener\WorkflowEnabledTransitionsListener:
    • Response on the GET $url/state simply answers what are the enabled transitions for a given resource ($workflow->getEnabledTRansitions($class))
  4. EventListener\WorkflowStateListener:
    • Manages the PATCH $url/state
    • Parses the data given which must be {state: 'newstate'}
    • throws if $workflow->can($class, $state) is falsy
  5. OperationPathResolver:
    • Decorates the path with the state suffix from above when needed

I can put this in a bundle/module that we could use on top of ApiPlatform if needed. I'm not sure that adding the workflow functionality to the core is needed/useful. These classes are really generic and allows you to handle the class state very easily without interfering with the access control stuff etc. You can add workflow listeners to your project and everything will just work.

Simperfit commented 6 years ago

@soyuka I think it's OK if you want to, to create a bundle for this. After that with the code we could decide if this could be in API-Platform or not. We already included things like graphql, a workflow bridge could be useful.

dunglas commented 6 years ago

Ok to have bridges with official Symfony components in core (but not to less maintained third party packages, such as FOS bundles).

soyuka commented 6 years ago

@soyuka I think it's OK if you want to, to create a bundle for this. After that with the code we could decide if this could be in API-Platform or not. We already included things like graphql, a workflow bridge could be useful.

It's so easy that I'm not sure we even need a bridge or whatever. Just add the bundle to the kernel and you're good. Also I'm not sure adding workflow control to ApiPlatform adds a real value.

Simperfit commented 6 years ago

This should go in a bundle in the first place, we can close this.

soyuka commented 6 years ago

My code if someone is interested: https://gist.github.com/soyuka/7c75933a6ae3d64940bb1d1f0d9fa9da

@Simperfit I'm reopening this one for few more months, maybe more people would see an interest in having this in the core.

soyuka commented 5 years ago

Closing as we closed #2040 this will not be included in the core as we couldn't find a RESTful way of adding it.

Steveb-p commented 4 years ago

Sorry for resurecting an old thread, but I don't want to open a new one. I'd like to reconsider supporting workflows in Api Platform, albeit with a different approach than presented above - or at the very least I'd be grateful for any comments regarding what I intend to do in my own application before I fall into a rabbit hole :smile:

I too was wondering how to add workflows to Api Platform resources. I've seen both @soyuka 's and @wesnick 's approaches and ultimately I think that the reason they do not work well with REST concepts is that they try to treat transitions as parts / actions of the entity they apply to. Conceptually REST works best if one does not try to go beyond the usual CRUD operations.

Therefore, I would treat transitions as first class resources in Api, similarly to how Messenger can be used to dispatch "reset password" command to a user account. I would expect them to receive iri and transition name as part of request.

class TransitionRequest
{
    public $iri;
    public $transition;
}

Then during transformation use IriConverter. Use event listener to see if entity can actually transition (by checking workflow as per usual). If yes, then do so, activating the whole workflow chain. Finally return some kind of TransitionResponse wraping resource after transition or simply respond with 202.

As for attaching transitions to resources, they could be kept under "@transitions" key for ld+json, similarly to IRI and type. Since transitions would always be under the same endpoint, the only information required would be transition name.

Is it viable to try to create a bundle out of this or integrate it into Api Platform? Is there something I'm missing?

Thanks in advance.

soyuka commented 4 years ago

Is it viable to try to create a bundle out of this or integrate it into Api Platform?

Feel free to do so, we won't integrate this in the core as it's really project-specific.

Cruiser13 commented 3 years ago

@Steveb-p your solution sounds good to me. Did you make any progress towards some bundle or integration?

Steveb-p commented 3 years ago

@Steveb-p your solution sounds good to me. Did you make any progress towards some bundle or integration?

@Cruiser13 as soyuka said, the implementation of exposing workflow information to API depends highly on project-specific requirements. I've done a normalizer that works with entities marked with a WorkflowEnabled interface. In theory you could simply check the registry for each, but since those checks need to be done each time supportsNormalization is called, I opted against it at the time.

<?php

declare(strict_types=1);

namespace App\Serializer\Normalizer;

use ApiPlatform\Core\JsonLd\Serializer\ItemNormalizer;
use Symfony\Component\Serializer\Normalizer\ContextAwareNormalizerInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerAwareInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerAwareTrait;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;
use Symfony\Component\Workflow\Registry;
use Symfony\Component\Workflow\Transition;

final class WorkflowEnabledNormalizer implements NormalizerInterface, NormalizerAwareInterface, ContextAwareNormalizerInterface
{
    use NormalizerAwareTrait;

    private const WORKFLOW_CHECKED_ALREADY = 'WORKFLOW_CHECKED_ALREADY';

    private Registry $workflowRegistry;

    public function __construct(Registry $workflowRegistry)
    {
        $this->workflowRegistry = $workflowRegistry;
    }

    public function supportsNormalization($data, string $format = null, array $context = [])
    {
        if (!$data instanceof WorkflowEnabled) {
            return false;
        }

        $resourceClass = $context['resource_class'] ?? '';
        if ($resourceClass !== get_class($data)) {
            return false;
        }

        return ($context[self::WORKFLOW_CHECKED_ALREADY] ?? false) !== spl_object_hash($data);
    }

    public function normalize($object, string $format = null, array $context = [])
    {
        $normalized = $this->normalizer->normalize($object, $format, $context + [self::WORKFLOW_CHECKED_ALREADY => spl_object_hash($object)]);

        if (is_array($normalized)) {
            $workflow = $this->workflowRegistry->get($object);
            $transitions = $workflow->getEnabledTransitions($object);
            $transitions = array_map(static function (Transition $transition): string {
                return $transition->getName();
            }, $transitions);

            switch ($format) {
                case ItemNormalizer::FORMAT:
                    $normalized['@transitions'] = $transitions;
                    break;
                default:
                    $normalized['__transitions'] = $transitions;
                    break;
            }
        }

        return $normalized;
    }
}

Feel free to re-use it if you'd like.

Do note (since it popped up in my application causing a major issue) that only root resource is checked.

nayodahl commented 2 years ago

Thanks a lot for your solution @Steveb-p . I'm wondering though, why you added this test in the support method if ($resourceClass !== get_class($data)) { return false; }
I can not think of a case where they could be different... Any idea ?

Steveb-p commented 2 years ago

@nayodahl This prevented trying to check workflow information for embedded objects for me, so only root objects were normalized (well, at least those of the same class as the root resource). Without it if there were many objects in relationship to the root resource, it resulted in timeouts & memory issues (especially if you have a robust workflows that depend on even more objects).

So I'd suggest using either a similar solution or some sort of context, so workflow normalizers like this one do not handle objects deeper in the tree.