archivematica / Issues

Issues repository for the Archivematica project
GNU Affero General Public License v3.0
16 stars 1 forks source link

Problem: Implementation details are unnecessary in storage service API calls #910

Open ross-spencer opened 5 years ago

ross-spencer commented 5 years ago

Expected behaviour

Components of Archivematica identified using a UUID need only be referred to using UUID by callers, e.g. when referring to a storage space in an API call.

API endpoints reduce the chances of a call being changed over time.

Complexity is managed upstream wherever possible making it easier to make use of downstream.

Current behaviour

The following call to the API will wail:

curl -s -d '{
>       "pipeline": ["b7cd1ec1-3295-4d94-9d7f-983c73feb4e3"],
>       "relative_path": "/home/path/to/my/location",
>       "description": "location-description",
>       "space": "290d2dc1-6aa9-4faf-93a7-2f1685980dcc"
> }'     -X POST     -H "Authorization: ApiKey test:test"     -H "Content-Type: application/json"         "http://127.0.0.1:62081/api/v2/location/" | python -m json.tool

Resulting in:

{
    "error_message": "An incorrect URL was provided '290d2dc1-6aa9-4faf-93a7-2f1685980dcc' for the 'SpaceResource' resource.",
    "traceback": "Traceback (most recent call last):\n\n  File \"/usr/local/lib/python2.7/site-packages/tastypie/resources.py\", line 220, in wrapper\n    response = callback(request, *args, **kwargs)\n\n  File \"/usr/local/lib/python2.7/site-packages/tastypie/resources.py\", line 451, in dispatch_list\n    return self.dispatch('list', request, **kwargs)\n\n  File \"/usr/local/lib/python2.7/site-packages/tastypie/resources.py\", line 483, in dispatch\n    response = method(request, **kwargs)\n\n  File \"/usr/local/lib/python2.7/site-packages/tastypie/resources.py\", line 1380, in post_list\n    updated_bundle = self.obj_create(bundle, **self.remove_api_resource_names(kwargs))\n\n  File \"/src/storage_service/locations/api/resources.py\", line 426, in obj_create\n    return super(LocationResource, self).obj_create(bundle, **kwargs)\n\n  File \"/usr/local/lib/python2.7/site-packages/tastypie/resources.py\", line 2163, in obj_create\n    bundle = self.full_hydrate(bundle)\n\n  File \"/usr/local/lib/python2.7/site-packages/tastypie/resources.py\", line 929, in full_hydrate\n    value = field_object.hydrate(bundle)\n\n  File \"/usr/local/lib/python2.7/site-packages/tastypie/fields.py\", line 790, in hydrate\n    return self.build_related_resource(value, request=bundle.request)\n\n  File \"/usr/local/lib/python2.7/site-packages/tastypie/fields.py\", line 697, in build_related_resource\n    return self.resource_from_uri(fk_resource, value, **kwargs)\n\n  File \"/usr/local/lib/python2.7/site-packages/tastypie/fields.py\", line 601, in resource_from_uri\n    obj = fk_resource.get_via_uri(uri, request=request)\n\n  File \"/usr/local/lib/python2.7/site-packages/tastypie/resources.py\", line 840, in get_via_uri\n    raise NotFound(\"An incorrect URL was provided '%s' for the '%s' resource.\" % (uri, self.__class__.__name__))\n\nNotFound: An incorrect URL was provided '290d2dc1-6aa9-4faf-93a7-2f1685980dcc' for the 'SpaceResource' resource.\n"
}

To make it work the caller has to include a string exposing components of the API and version:

curl -s -d '{
      "pipeline": ["/api/v2/pipeline/b7cd1ec1-3295-4d94-9d7f-983c73feb4e3/"],
      "relative_path": "/home/path/to/my/location",
      "description": "location-description",
      "space": "/api/v2/space/290d2dc1-6aa9-4faf-93a7-2f1685980dcc/"
}'     -X POST     -H "Authorization: ApiKey test:test"     -H "Content-Type: application/json"         "http://127.0.0.1:62081/api/v2/location/"

Arguably /api/v2/pipeline/b7cd1ec1-3295-4d94-9d7f-983c73feb4e3/, the example given for pipeline, is an implementation detail. The UUID being unambiguous itself, shouldn't need to be constructed as a URI by the caller and can be constructed as required by the endpoint on receiving the message.

Steps to reproduce

Using the call above, without the /api/v2/pipeline/ or /api/v2/space/ components, you will see the error message appear as returned at present.

Your environment (version of Archivematica, OS version, etc)

Archivematica 1.10.

Additional context

Would this change to happen it might be low-impact but it might also make it ever so slightly easier to go about constructing API calls. Other endpoints might benefit from similar changes. Such a change would see both versions of the mechanism work the same way.


For Artefactual use: Please make sure these steps are taken before moving this issue from Review to Done:

sevein commented 5 years ago

You could argue that /api/v2/pipeline/b7cd1ec1-3295-4d94-9d7f-983c73feb4e3/ is a better representation of the resource because it's implicitly describing how it's accessed. It allows the client to navigate the system without prior knowledge. See HATEOAS. Tastypie implements a HATEOAS-driven API.

I don't think that there's an actual standard about it but it's pretty common. Many APIs go further and use absolute URIs, e.g. GitHub's API. See also:

ross-spencer commented 5 years ago

URIs are great for sure. I think the reason it is something that stands out for me trying to write location calls at the moment is because of the /api/v2/ part which is what I feel to be slightly incongruous - will a space be defined in a /api/v3/ namespace in future? Does that define a different type of Space? Will it be a different space entirely?

From our perspective too, a space is also http://<storage-service-url>/spaces/290d2dc1-6aa9-4faf-93a7-2f1685980dcc/ as well as a standalone UUID.

In other cases we don't specify a URL-like string in an API call. Take our pipeline argument on reingest:

curl -v -H "Authorization: ApiKey test:test" \
   -H "Content-Type: application/json" \
   -X POST -d '{"pipeline":"bdf745b9-139d-4438-abe2-0894d9f2ff31","reingest_type":"FULL"}' \
   'http://127.0.0.1:62081/api/v2/file/07189108-658c-44c2-a053-1ccda2f15990/reingest/' 

Which makes sense, as a user, if I look up a pipeline in the dashboard or the Pipeline tab of the storage service I'm not told how to construct /api/v2/pipeline/a1b2d4ef... so the documentation for the endpoint has to tell me that.

But I don't think there's a huge impact either way. And completely accept it's likely more of personal taste.

sevein commented 5 years ago

I think the reason it is something that stands out for me trying to write location calls at the moment is because of the /api/v2/ part which is what I feel to be slightly incongruous - will a space be defined in a /api/v3/ namespace in future? Does that define a different type of Space? Will it be a different space entirely?

As a client you don't have to worry about any of that as long as you keep using /api/v2/ because SS is not supposed to break the contract.

Which makes sense, as a user, if I look up a pipeline in the dashboard or the Pipeline tab of the storage service I'm not told how to construct /api/v2/pipeline/a1b2d4ef... so the documentation for the endpoint has to tell me that.

As far as Tastypie goes, you're actually told how to construct it. Beyond that, as in your example, I think we've been working with different representations inconsistently and that's IMO the problem. Have you seen what's returned by the API when you create a new Pipeline?

{
   "create_default_locations": true,
   "description": "Test pipeline",
   "remote_name": "192.168.1.42",
   "resource_uri": "/api/v2/pipeline/99354557-9e6e-4918-9fe3-a65b00ecb199/",
   "uuid": "99354557-9e6e-4918-9fe3-a65b00ecb199"
}

You can even inspect resource schemas.