-
Notifications
You must be signed in to change notification settings - Fork 71
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
Example ResourceService #131
Conversation
…edoraApi class from Chullo.
Depends on Chullo fixes at https://github.com/DiegoPino/chullo/tree/api
for get route: assuming it's running in 8181 (example php -S localhost:8181 -t src src/index.php ) then http://localhost:8181/islandora/resource/7ef68f6f-72ab-4708-b0f4-8ec1f07 cd580 will get the resource for a resource that has nfo:uuid -> 7ef68f6f-72ab-4708-b0f4-8ec1f07cd580 in the triple store and http://localhost:8181/islandora/resource/7ef68f6f-72ab-4708-b0f4-8ec1f07 cd580/OBJ will get the child resource for that one (in this case binary) Accepts also a ?tx=transactionid * if the transactionid does not exist if fails with 404 * if uuid is not uuid then it fails with 404 * if resource path is not in triple store if fails with 404
Main route for getting a resource accepts also a metadata argument localhost:8181/islandora/resource/7ef68f6f-72ab-4708-b0f4-8ec1f07cd580/O BJ/?metadata=true In case of binary it will return triples instead of the binary resource In case of an container i will get you the RDF (basically ignoring this option)
@Islandora-CLAW/7-x-2-x-sprinters, updated, includes resource creation via POST |
Also moved some route parameters to query arguments (checksum) in all routes: tx ,checksum and metadata (depending on what is used in each route)
Did also a lot of coding cleanup. Not sure if the Symphony coding standard as base is good enough? Moved the ->abort for non existing resource path in triple store into the middleware.
return $twig; | ||
})); | ||
|
||
$app['fedora'] = $app->share(function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to call share() here. Just passing in the closure is enough. In earlier versions you had to do that because default was to create a new service every time it was pulled from the DI container. Now the default is to share a single instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dani, did not find this in the documentation or release logs. Do you have the link for my own further learning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check out the usage section. If you want a new one per request from the container, you have to use $app->factory().
But now I'm wondering what version of pimple silex uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, pimple 1.x in silex. We have to stick with ->share
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to update the transaction service then!
Should we remove the following files from the Pull? services/ResourceService/composer.lock Also should we change the URL property of the VCS of the composer.json files? |
Keep the composer.json files. Ditch the lock file. We'll update where chullo gets pulled from in the composer.json files when the chullo PR gets merged. In general, it should always point to a version on packagist before getting merged. |
|
||
require_once __DIR__.'/../vendor/autoload.php'; | ||
|
||
use GuzzleHttp\Exception\ClientException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove Guzzle here right?
Using app->share in TransactionService.
* Added simplistic yaml config reader * Added middleware to handle empty $id in case of referencing root resource * Added error handler/catcher for unavailable Fedora and tripplestore services * Moved regex for uuid to config variable, not allows UUID V4 or empty * added Delete resource with ?force=true option in route to also remove tombstone in one step, can be used also on an existing tombstone Todo: * make a UUID/rdf generating service to generate intermediate resources in case of PUT and POST(if needed, still not convinced if really needed at this side) * check for mime types in case of updating existing resources, abort before hitting fedora if trying to "convert" and rdf->to binary or the other way resource. * Make messages more human friendly * Documentation
@Islandora-CLAW/7-x-2-x-committers, some updates on this if you mind looking/commenting/discussing. If followed @daniel-dgi advice on most of the new stuff 😁 , except for removing the $child parameter because it's pretty handy like when requesting fcr:metadata, tombstones, or fixity for existing resources + our own OBJ/TN/etc naming convention. Mostly some cleanup, added delete method, made the uuid check regex a variable in yaml, intercepting some exceptions, different settings for debug and production. Still lots todo! |
"repositories": [ | ||
{ | ||
"type": "vcs", | ||
"url": "/Users/dpino/Desktop/Development/ISLANDORAWORK/CLAW_MICRO/chullo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be the main github repo and just make this PR rely on the Chullo PR?
else { | ||
$configFile = __DIR__.'/../config/settings.yml'; | ||
} | ||
$settings = Yaml::parse(file_get_contents($configFile)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So simple. Love it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😊
$checksum is optional, passed as get variable to the route ?checksum="urn:sha:somekey"
Moved all common middleware to global controllers def. Cleaner code and easier to maintain.
* takes 'rx' and/or 'checksum' as optional query arguments | ||
* @see https://wiki.duraspace.org/display/FEDORA40/RESTful+HTTP+API#RESTfulHTTPAPI-YellowPUTCreatearesourcewithaspecifiedpath,orreplacethetriplesassociatedwitharesourcewiththetriplesprovidedintherequestbody. | ||
*/ | ||
$app->put("/islandora/resource/{id}/{child}",function (Application $app, Request $request, $id, $child) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there also be a PUT route for when you only have an $id? Same for PATCH below and DELETE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, child is optional, set to "" by default at https://github.com/Islandora-CLAW/CLAW/pull/131/files#diff-079c3800368dc100b90e9a9254e6fcc6R176 , so if not passed/used in the call it's all good, the route will match. Only post is {$id} only, and i would love to add $child there too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, error was due to me sending my own Content-type header with curl. Future work might be to just strip the duplicate headers for PATCH as it has to be application/sparql-update.
Future work has been completed, just that composer hates me. 👍 merging. |
😄 |
see #129
Update
https://github.com/DiegoPino/islandora/tree/example-service
Get resource is working fine.
Depends on some minor fixes on Chullo at
https://github.com/DiegoPino/chullo/tree/api
or
Islandora/chullo#30
Basic (Manual testing)
config/settings.dev.yml
to match your triplestore and fedora4 URLs$ composer update
$ php -S localhost:8282 -t src src/index.php
this will use the php embed web server to testwith a call like this
$ curl http://localhost:8282/islandora/resource/7ef68f6f-72ab-4708-b0f4-8ec1f07cd580 -v
in case you have a child resource (lets say an image binary at /OBJ) you can issue this call
$ curl http://localhost:8282/islandora/resource/7ef68f6f-72ab-4708-b0f4-8ec1f07cd580/OBJ -v
Also accepts this query arguments:
tx=transactionid
andmetadata=true
Will fail if
tx
given and not a valid transactionid (404)Will get fcr:metadata for any resource (e.g binary) if
metadata=true
Note: if you don't have a CLAW vagrant environment working you can use(only testing)
fcrepo-webapp-4.4.0-jetty-console.jar
)TODO
*
Rest of the resource routes (POST/PATCH/etc). Basically i have to resolve an issue with Fedora rewriting the triples based on thehost
header, which in our case, since we are a middle service, depends on where we are running our silex service. This means that, e.g, our returnedlocation
in case of a POST (creating a resource) will have the host and port of silex in the resource path which is bad. Same for any other operation. We are fetching the resource path from the triple store matching nfo:uuid. That one will always be a static one(inserted on resource creation) so trouble can occur when matching that returned resource path with fedora's rest point for that resource.Options are to match the host header to where fedora is running, this way, at least we would get a constant base uri, others are more complex (like living with this)Make URL configurations external (not sure, do we wan't to keep service as slim as possible?)