Request linked resources directly from returned responses.

[eporama]

Is your feature request related to a problem? Please describe.
Some endpoints return a Notification UUID to allow for tracking the completion of an attempt.
For example, when you create a database backup or ask to purge Varnish cache, the response is that the process has started and a Notification to track.

However, the json body response only has a "_links.notification.href" with a full URL to the notification. To use the "Notifications" object directly, you need to have a notification UUID. In the body response, this would involve parsing the href URL to extract the ID.

There is also an HTTP header, X-CloudApi-Notification-Id, that is returned with just the ID which would be helpful.

Describe the solution you'd like
It seems that having the ability to a) retrieve the UUID directly and b) have a semi-automated looping mechanism that can track the notification to "completed" or an error state.

[typhonius]

Great news to hear notification UUID is passed back in a header. I always thought looking up the most recent notification (as I do in Acquia cli) was a bit frail.

I think a good option here for this library would be to retrieve the UUID and pass it back as a property of the OperationResponse.

By the way, is this header reflected in the docs somewhere so I can refer as I build? No dramas if not but would be good if there’s a recommendation on implementation/stability/coverage.

[typhonius]

I had a first pass at this in #104. I've passed through the response headers by adding them to the body as _headers because none of the endpoint classes have access to the client or response so can't access it directly. I did consider passing $this->client to each of the endpoints but that didn't smell right either.

One thing I have also considered doing is parsing the _links property within the OperationResponse to get the UUID out that way. I do similar in the code below but again, it doesn't seem clean to me: https://github.com/typhonius/acquia_cli/blob/master/src/Commands/AcquiaCommand.php#L161-L165

Before I look further into this, I'd love to see whether the Acquia API team would consider changing the API or augmenting in such a way that any operation would also pass back the notification UUID in the response as that seems to me to be a cleaner way to handle this.

e.g. for the creation of a database.

Current

{
  "message": "The database is being created.",
  "_links": {
    "self": {
      "href": "https://cloud.acquia.com/api/applications/a027502b-ad6c-a48e-a7e8-aa0def7d25e1/databases"
    },
    "notification": {
      "href": "https://cloud.acquia.com/api/notifications/6992a41d-a953-4ded-ae99-41d2f4d62f69"
    },
    "parent": {
      "href": "https://cloud.acquia.com/api/applications/a027502b-ad6c-a48e-a7e8-aa0def7d25e1"
    }
  }
}

Proposed

{
  "message": "The database is being created.",
  "notificationUuid": "6992a41d-a953-4ded-ae99-41d2f4d62f69",
  "_links": {
    "self": {
      "href": "https://cloud.acquia.com/api/applications/a027502b-ad6c-a48e-a7e8-aa0def7d25e1/databases"
    },
    "notification": {
      "href": "https://cloud.acquia.com/api/notifications/6992a41d-a953-4ded-ae99-41d2f4d62f69"
    },
    "parent": {
      "href": "https://cloud.acquia.com/api/applications/a027502b-ad6c-a48e-a7e8-aa0def7d25e1"
    }
  }
}

[typhonius]

The more I look at this, the more inclined I am to either parse the _links URL in the OperationResponse class or wait for the notification UUID to be passed back in the response body itself.

Unless there's a more clean way to pass this header through (which may require quite a large restructure of the codebase), this might just be something that we delegate to dependent libraries (as Acquia Cli does).

My main reasoning behind this is to keep this library as simple as possible without making on-the-fly adjustments to the responses from the API (which I've realised my patch does). I could potentially make a public method to retrieve response headers, however to me that feels like the wrong way to go forward with this library.

I'll leave this issue and PR open for now as there might be some useful input that I've not considered, but currently I can't think of a clean solution - especially when there is a workaround, albeit a slightly dirty one.

[itafroma]

Hi @typhonius !

I think it could prove helpful to add linked resource retrieval: Cloud API uses HAL+JSON as its response format to facilitate traversing the API via related resources (HATEOAS).

Something like this might work to retrieve the notification in this case, for example:

$response     = $client->deployCode();
$notification = $client->getLinkedResource($response->getLink('notification'));

Or in other contexts, let's say you wanted to get the application that contains an environment:

$environment = $client->getEnvironmentById('env-id');
$application = $client->getLinkedResource($environment->getLink('application'));

That said, I've noted internally that we should consider just embedding the notification in the response, to avoid having to make the additional request to retrieve it. While this would save one request, we expect that the notification resource to be polled multiple times as the operation completes, so I'm not sure how much value it'd be to save that initial request.

Let me know what you think!

[typhonius]

Thanks for that extra context @itafroma. You’ve given me a good idea about how we can make this happen.

Because all tasks that would effect a change causing a notification get turned into an OperationResponse I can add some parsing logic there to change the linked URL into a notification UUID. I’ll make the relatively safe assumption that the way linked resources work won’t change (at least in v2 of the API) as effectively the URL will need to be munged to retrieve the UUID from the end.

I’ve also decided to explore creating a new GenericResponse class which all the other non-collection classes can extend. I’ll have a play with this to see about making it easy for users to get linked resources. Below is what I'm trying at the bottom but that will likely change as I play with it more to keep things really tidy.

abstract class GenericResponse
{

    public function getLink(string $name)
    {
        if (!$this->links) {
            throw new NoLinkedResourceException('No linked resources for' . get_called_class());
        } elseif (!property_exists($name, $this->links)) {
            throw new LinkedResourceNotFoundException('No property exists for ' . $name);
        }
        return $this->links->$name;
    }
}

class OperationResponse extends GenericResponse
{

    /**
     * @var string $message
     */
    public $message;

    /**
     * @var object|null $links
     */
    public $links;

    /**
     * @param object $operation
     */
    public function __construct($operation)
    {
        $this->message = $operation->message;
        if (isset($operation->_links)) {
            $this->links = $operation->_links;
        }
    }

    public function getNotification()
    {
        $notification = parse_url($this->getLink('notification'), PHP_URL_PATH);  
        $segments = explode('/', rtrim($notification));

        return end($segments);
    }
}

I’m not sure yet how I’ll go about actually making the call to retrieve the linked resource but that’s just an implementation and organisation puzzle.

[typhonius]

I've created a first pass in #111 that definitely won't pass tests. I've branched from #108 so the PR will look extra-large until that is merged in. Functionality-wise we're looking at something like the below:

$connector = new Connector($config);
$client = Client::factory($connector);
$application  = new Applications($client);

$applicationUuid = '62f5f533-a138-04be-7421-ae6aa3281a6d';
$app = $application->get($applicationUuid);
$databases = $application->getLinkedResource($app->getLink('databases'));
$environments = $application->getLinkedResource($app->getLink('environments'));

[typhonius]

And for notifications (the original point of this issue), we'd be looking at something like:

$connector = new Connector($config);
$client = Client::factory($connector);
$application  = new Applications($client);

$applicationUuid = '62f5f533-a138-04be-7421-ae6aa3281a6d';
$response = $db->create($acquiaCliTestApplication, 'foobar1234');

$notification = $db->getLinkedResource($response->getLink('notification'));
$notificationUuid = $notification->uuid;

[typhonius]

Looks like this passes tests now. As this is a fairly meaty addition - @eporama could I please ask you to have a play at your end with #111 to make sure that it works as expected?

[itafroma]

Because all tasks that would effect a change causing a notification get turned into an OperationResponse I can add some parsing logic there to change the linked URL into a notification UUID. I’ll make the relatively safe assumption that the way linked resources work won’t change (at least in v2 of the API) as effectively the URL will need to be munged to retrieve the UUID from the end.

Very cool! Looks like you implemented what we expected (and support) in #111, but to clarify regarding how links work:

• We consider removing links to be a BC break. e.g. if a response has a notification link today, there will be a deprecation event before we ever remove it
• Our goal is to use the same type of response for each link name. e.g. notification links will always return a response that can be parsed as a NotificationResponse. We haven't done a full audit of this, so here may be cases where this isn't the case that we would consider to be unintended/a bug
• The value of href should be considered opaque and can change at any time. e.g. it's not safe to assume you can parse the notification ID just from the value of href: you should request the linked resource and check the UUID that way.

And just a word of warning, we want all operations to have a notification link, but there are still a few gaps, mainly around teams and permissions (whose operations all resolve instantaneously).

[typhonius]

Thanks @itafroma I think what's been written so far aligns with your 1st and 2nd bullets nicely. I also think your final comment is taken care of because I ended up not tying notifications to OperationResponse and instead extending GenericResponse which will send the request and create a NotificationResponse if a notification link exists and won't if it doesn't!

For your second point, can I safely assume that there will only one (currently named href) property returned per linked resource e.g. databases->href->$link, environments->href->$link etc. If so, I can do something like the below that will allow the library to get the link even if the word 'href' is substituted for something else. If the link may have multiple properties in future e.g. databases->href->$link and databases->foo->$link then we'll need to think up a new way of enumerating links.

If there's an easier way of parsing this (while taking into account href may change to a different word) then I'd love to put that in instead. The solution below works but feels a bit unclean.

    public function getLink(string $name)
    {
        if (!property_exists($this, 'links')) {
            throw new NoLinkedResourceException('No linked resources for ' . get_called_class());
        } elseif (!property_exists($this->links, $name)) {
            throw new LinkedResourceNotFoundException('No property exists for ' . $name . '. Available links are ' . implode(' ', array_keys(get_object_vars($this->links))));
        }

        $linkProperty = (array) $this->links->$name;
        $linkName = key($linkProperty);
        return ['type' => $name, 'path' => $this->links->$name->$linkName];
    }

[typhonius]

This is a bit cleaner (amazing what you can find on StackOverflow) but I fear it suffers from being a bit opaque to someone coming in fresh (or me in 2 months after I've forgotten why I did it) so may either need further cleaning up or heavy documentation.

        public function getLink(string $name)
    {
        if (!property_exists($this, 'links')) {
            throw new NoLinkedResourceException('No linked resources for ' . get_called_class());
        } elseif (!property_exists($this->links, $name)) {
            throw new LinkedResourceNotFoundException('No property exists for ' . $name . '. Available links are ' . implode(' ', array_keys(get_object_vars($this->links))));
        }

        /**
         * Because the name of the property within the $links->property->$name object may change, we must avoid accessing it directly.
         * The below converts the object into an array, obtains the values directly (bypassing the need to know the key),
         * and retrieves the first (and only) item from the resultant array which will be our linked resource path.
         */
        $path = current(array_values((array) $this->links->$name));
        return ['type' => $name, 'path' => $path];
    }

[itafroma]

We adhere to the JSON HAL draft spec, so you can assume href will always contain the correct URL for a link, and all _links elements will have an href property.

The only weirdness you might run into is in parsing certain types of templated links, like the ones you see for pagination and filtering in collection responses. Those are designated with a "templated": true property:

  "_links": {
    "self": {
      "href": "https://cloud.acquia.com/api/applications?limit=10"
    },
    "sort": {
      "href": "https://cloud.acquia.com/api/applications{?sort}",
      "templated": true
    },
    "filter": {
      "href": "https://cloud.acquia.com/api/applications{?filter}",
      "templated": true
    },
    "limit": {
      "href": "https://cloud.acquia.com/api/applications{?limit}",
      "templated": true
    },
    "parent": {
      "href": "https://cloud.acquia.com/api/"
    }
  },

It may be worth supporting those as an optional parameter:

// Retrieves https://cloud.acquia.com/api/applications?limit=10
$limited_applications = $db->getLinkedResource($response->getLink('limit'), [
    'limit' => "limit=10",
]);

// Retrieves https://cloud.acquia.com/api/applications
$limited_applications = $db->getLinkedResource($response->getLink('limit'));

Though it gets tricky because this would be a case where the link name doesn't tell you the correct response. Now that I think of it, there's a number of link names that are contextual in what they return:

• self: same as original response
• parent: 100% contextual, no way to know (I'll bring this back to the team to see if we can come up with something here for discovery)
• sort: same type of collection as original response
• limit: same type of collection as original response
• filter: same type of collection as original response
• next: same type of collection as original response
• previous: same type of collection as original response

[eporama]

Okays, so this makes sense to get the notification object.

$notification = $db_backups->getLinkedResource($backup->getLink('notification'));

One question (may be a separate issue) is how to refresh that notification so that you can loop until "completed"… We probably don't need it on a lot of objects, but notification is meant to change frequently, so having a method on the object to refresh or even to watch (doing the looping and discovery for you) would be the next step.

[itafroma]

The self link is there to allow for API clients to "refresh" their view of a resource. In other words, something like this would be the expected way to poll the status of a notification:

$notification = $notification->getLinkedResource($notification->getLink('self'));

[typhonius]

One question (may be a separate issue) is how to refresh that notification so that you can loop until "completed"… We probably don't need it on a lot of objects, but notification is meant to change frequently, so having a method on the object to refresh or even to watch (doing the looping and discovery for you) would be the next step.

I think for the purposes of this library, implementing a watch/refresh function is probably best left to other packages extending it. The reason for this is that there will be a number of different configuration parameters that users may wish to alter for themselves e.g. how often to check a notification, how long to wait until a task is considered timed out, or if we even bother waiting for a response.

I feel like it would go beyond the scope of an SDK to determine how the user wishes to interact with the API beyond making requests and receiving responses. I'd be willing to reconsider if there's a reasonably simple, non-prescriptive way of doing it, but it feels more natural to keep it in a dependant library.

I've done similar in the logstream package for specifics around connecting to that service (this library just gets the logstream connection details) and in the cli package (which handles waiting for notifications). Code for that is below if you want to pinch it for acquia/cli or another package.

https://github.com/typhonius/acquia_cli/blob/master/src/Commands/AcquiaCommand.php#L152-L221

We adhere to the JSON HAL draft spec, so you can assume href will always contain the correct URL for a link, and all _links elements will have an href property.

It seems that the JSON HAL spec requires the href parameter so I'll put that back in. I've reread your message from earlier and realised that I misunderstood what you meant by the value being opaque - you didn't mean that the word 'href' could change but that the value itself could change. Since we're calling request() on the value stored in href, I think we're safe as I'm no longer trying to parse UUIDs from it. I'll also change back the code I had which specifically uses href.

The only weirdness you might run into is in parsing certain types of templated links, like the ones you see for pagination and filtering in collection responses. Those are designated with a "templated": true property:

I think your sample code below makes a lot of sense and will be relatively easy to implement, although I'll probably schedule that in a follow-up for this. We have access to $client when we're getting linked resources so it'll be a case of doing a $client->addOption('limit', 10) or similar from within the getLinkedResource function.

$limited_applications = $db->getLinkedResource($response->getLink('limit'), [
    'limit' => "limit=10",
]);

The self link is there to allow for API clients to "refresh" their view of a resource. In other words, something like this would be the expected way to poll the status of a notification:

I'll need to have a bit of a longer think about how I make this work as:

• GenericResponse (and all child response classes) don't have access to the $client so can't make subsequent calls
• CloudApiBase (and all child endpoint classes) don't have access to the returned response so can't tell what type of response we're working with

My thought for self was that I could get_class the response and use that to instantiate a new response of the same type. It's entirely possible that I'll need to rethink the architecture of this because at the moment getting linked resources is only available for single response objects rather than collections e.g. $application->get() but not $application->getAll(). The reason for this is that collections already extend ArrayObject because we use the functionality of that class to convert a number of returned results into a neat object of objects e.g. ApplicationsResponse containing many ApplicationResponse.

Ultimately however it would be beneficial to get linked resources from either though so I'll do some thinking of how I can alter things in a non-breaking way - perhaps this is where a trait would be useful...

[typhonius]

Ok, pretty mega restructure but I believe without any changes to end user code:

• Created new CollectionResponse class which all collections will extend e.g. ApplicationsResponse, EnvironmentsResponse etc. CollectionResponse extends ArrayObject as collection classes did previously
• Simplified all collection class constructors and moved the business logic into CollectionResponse
• Created new LinkedResourceTrait trait which both GenericResponse and CollectionResponse use to get links
• Passed more back from the trait so we have the response class which allows us to use self links
• Added back specific call for href links because the spec shows they're mandatory

[typhonius]

Commenting for myself later that it might be worth putting getLinkedResource on Client or in another trait.