Implement PATCH request for FedoraResource entities

[dannylamb]

Implement an endpoint that accepts PATCH requests at http://localhost:8000/fedora_resource/{id}.

• It should respect the Drupal/Symfony _format parameter.
• It should accept the Vclock header to perform conditional updates. E.g. if the provided header and the value in the DB are not the same, abort the operation because someone else has already updated it. This will be used for optimistic locking.

[whikloj]

Just to flesh this out. What would the possible contents of the BODY of this request be?

[dannylamb]

@whikloj It would be equivalent to what PATCH accepts for XML or JSON, but JSON-LD. So it would contain triples you want updated, but not the entire representation of the entity. Something akin to https://www.drupal.org/docs/8/core/modules/rest/4-patch-for-updating-content-entities, which shows you how it's done with HAL.

[ajs6f]

Is it something like this?

[dannylamb]

@ajs6f Not really. I was rolling with Drupal on this one. It expects enough info to load the entity (which is more than just an id, you need to know bundle as well), and then only the properties you want to get updated. There may be a little more wrapped in there, but couldn't tell you without more exploration.

[whikloj]

So funny story, this works for json right now. No actual work involved.

I enabled PATCH with json and basic auth. Then I created a RDF Source with some name.

Then I did

> curl -i -XPATCH -uadmin:islandora -H"Content-type: application/json" --data-binary '{"type":[{"target_id":"rdf_source"}], "name":[{"value":"BOOM"}]}' "http://localhost:8000/fedora_resource/1" 

HTTP/1.1 200 OK
Date: Thu, 23 Mar 2017 00:50:02 GMT
Server: Apache/2.4.18 (Ubuntu)
Cache-Control: must-revalidate, no-cache, private
X-UA-Compatible: IE=edge
Content-language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
X-Generator: Drupal 8 (https://www.drupal.org)
Content-Length: 758
Content-Type: application/json

{"id":[{"value":"1"}],"vid":[{"value":1}],"type":[{"target_id":"rdf_source","target_type":"fedora_resource_type","target_uuid":"af1cb3d8-2c1f-4ca1-a1c2-b1173704807f"}],"uuid":[{"value":"ded658d0-4210-4972-bd62-3090ea806ff9"}],"user_id":[{"target_id":"1","target_type":"user","target_uuid":"8bbfb873-e28c-486d-84a1-c200d5f0529d","url":"\/user\/1"}],"fedora_has_parent":[],"name":[{"value":"BOOM"}],"status":[{"value":"1"}],"langcode":[{"value":"en"}],"created":[{"value":"1490227017"}],"changed":[{"value":1490230202}],"promote":[{"value":"1"}],"sticky":[{"value":"0"}],"revision_timestamp":[{"value":"1490227017"}],"revision_uid":[],"revision_log":[],"revision_translation_affected":[{"value":"1"}],"default_langcode":[{"value":"1"}],"field_ldp_contains":[]}

and lo and behold the name of the resource had been updated.

If I don't pass the basic auth parameters you get a HTML page for access denied, but the headers are

> curl -i -XPATCH -H"Content-type: application/json" --data-binary '{"type":[{"target_id":"rdf_source"}], "name":[{"value":"Smack that"}]}' "http://localhost:8000/fedora_resource/1" 

HTTP/1.1 401 Unauthorized
Date: Thu, 23 Mar 2017 00:52:10 GMT
Server: Apache/2.4.18 (Ubuntu)
Cache-Control: must-revalidate, no-cache, private
WWW-Authenticate: Basic realm="Islandora-CLAW"
X-UA-Compatible: IE=edge
Content-language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
X-Generator: Drupal 8 (https://www.drupal.org)
Content-Length: 6527
Content-Type: text/html; charset=UTF-8

Thoughts?

[whikloj]

So I'm guessing this is not going to work for json-ld format, nor does it care about the vclock parameter. The json-ld is probably something we need to develop centrally so it can be used by all the HTTP operations, but I'll look to adding a requirement for the vclock.

[DiegoPino]

@whikloj but if it works for JSON, most of the heavy work is done, good finding!. the JSON-LD part would just need to map back to JSON representation Drupal already knows of (which are the fields, most likely the way post is doing that) and then execute all the logic JSON already applies. Sounds easy, but for sure it can have more complications. Probably when that logic goes into the serializer this operation should work natively for json-ld also.

[dannylamb]

Yeah, totally works right now. If we can't sort out the serializer in both directions before deadline, this is my fallback, lol.

[dannylamb]

@whikloj It's tricky. There doesn't seem to be a way to really alter existing REST resources, per se. Just ways to register your own plugin instead of the default. It's akin to making your own custom controller but having it work under the guise of the Drupal's REST system.

Maybe we could implement conditional updates with the version counter via middleware, but I haven't explored that approach enough.

[whikloj]

So what I am doing right now is just implementing hook_rest_resource_alter

Then pointing to my own class that extends the normal entity REST response class and just implementing my own patch function.

This seems to be behind the serialization layer as patch just gets the old entity and the new entity and you do your thing.
public function patch(EntityInterface $original_entity, EntityInterface $entity = NULL)

[dannylamb]

@whikloj Precisely. If you can get at the headers and add the conditional part there, I think it'd be great and provide a seamless Drupal experience.

I got pretty frustrated with it earlier and gave up on REST system, so maybe you're just the man we need to work through it.

[whikloj]

@dannylamb oh right I forgot that the vclock was a header 🤦‍♂️

This is going to need to be a routing alter, as the requirements are set on routes.

[whikloj]

Ok, I need some help here.

This is what I know. If the vclock is in the request body, I can compare it once we start processing the actual PATCH. That isn't really the Drupal way as the check should occur before it, but we don't get that until the entity is ready.

We can leave it as a header and perform a test like the CSRF-Token check, except that to validate the vclock we need to load the entity, to get the uuid to get the current vclock value.

But (and this is my lack of understanding of the "Drupal" way) you don't have both the request and the entity at the same time.

[DiegoPino]

@whikloj you could have request and the entity at the same time. Worst scenario: at least enough data to load the entity base on the request. Can you point me to a piece of code/existing functionality you would like to extend?
One good example of entity and request being at the same contex/same time are controllers that upcast (convert argument into object) entities like https://github.com/Islandora-CLAW/islandora/pull/33/files#diff-80c2a2a62b59c06233b678757a7bc4d6R27. I guess the REST project does something similar.

last idea, if you have the request object around, as we did in silex, you can further mash-it-up, decorate it, and add stuff to it (like an internal communication decive) only because the request object Drupal uses is not PSR-7 (inmutable)

[whikloj]

I got it, it took me a bit but I can just add the Request to the patch function signature and boom it is there. So I am closer.

[dannylamb]

The version counter is passed as a header and is not stored on the entity, so it could in theory be handled totally before-hand. But yeah... that's the problem with the whole REST system, the plugins don't act until too far downstream.

I think you can auto-inject Requests by just putting them in the function.

[dannylamb]

HA! Ninja'd by @whikloj

Good form.

[whikloj]

Before I get too far... Do we care about this warning showing up in watchdog and on the CLI when clearing cache?

PHP Warning: Declaration of Drupal\islandora\Plugin\rest\resource\FedoraResourceEntity::patch(Drupal\Core\Entity\EntityInterface $original_entity, Drupal\Core\Entity\EntityInterface $entity = NULL, Symfony\Component\HttpFoundation\Request $request) should be compatible with Drupal\rest\Plugin\rest\resource\EntityResource::patch(Drupal\Core\Entity\EntityInterface $original_entity, Drupal\Core\Entity\EntityInterface $entity = NULL)

Basically because I extended EntityResource and it's signature for patch() doesn't have the Request argument. Not sure how to avoid this though.

[DiegoPino]

@whikloj easy or the complex answer?

UPDATE: easier way
https://api.drupal.org/api/drupal/core!lib!Drupal.php/function/Drupal%3A%3Arequest/8.2.x or

$yourcontainer->get('request_stack')->getCurrentRequest(); 

And keep the signature intact

(don't read after this block, your eyes will hurt)

Easy (for me, not for the developer): We should care because it will fail on tests and it is not cool: breaks http://en.wikipedia.org/wiki/SOLID
PHP thing, because maybe JAVA would allow some trick there.

Complex (for all of us): You will need to extend from the same interface it is expecting and add methods that are missing/you need via private methods. It is basically duplicating code. Other option is creating internally a new object from the class you need by copying attributes from one (compatible interface) to another(the one you want) (code duplication sorry) or add/reuse some type of Trait.

Can you share some snippet?
(sorry, all this sounds so hard)

[whikloj]

Hi all,

Okay I have gotten to the point where I have the JSON-LD patch at the ContentEntityNormalizer in claw-jsonld.

There is all goes to hell, mostly because it does not seem to do anything with the @graph, it checks for a field with that name and dies on an uncaught exception.

I'm going to push up branches to Islandora and claw-jsonld so everyone can see what I'm doing.

I feel like I'm close, but not overly confident it this serialization land.

[whikloj]

Here is the comparison of my Islandora branch issue-527. This works for JSON, and is pretty good for json-ld. I get the header into the data (actually into the context) but the json-ld denormalizer might need adjusting or I might need to be putting the entity id and bundle id in different spots.

Here is the claw-jsonld branch comparison, I tried to just get the entity/bundle to load and thought the rest would work...but it doesn't seem to.

[DiegoPino]

@whikloj I think what you are seeing is correct and expected. claw-jsonld never got to that part. All the coding was done on the normalizing/serializing part but not backward fucntionality was added, remember this code comes from a different dimension.

If you are willing to give this next week some try, i will grab both your branches and do some testing tomorrow sunday to have something to share (no tests!) next week. Could need some guides to get to the same part/see what you are seeing and understand how you would like that part to behave. (and of course would need you to review my code and finally, maybe marge)

Good work.

[whikloj]

@DiegoPino sounds good, just knowing that this part may not be functional before helps me out. I can work through some of it. I am wondering is should we be standardizing our JSON-LD before this...like should we transform whatever we get into expanded so we can only deal with one format.

I also realized I have some changes, I injected a database connection to make a VersionCounter I should have just injected the VersionCounter.

I'll play around with this and see what I can come up with.

[DiegoPino]

@whikloj want me to pull against your https://github.com/Islandora-CLAW/claw-jsonld/compare/8.x-1.x...whikloj:issue-527?expand=1 ?
That way you can keep your own Pull request open and you can enjoy of clutterless git-merge-committer approval experience!

@Natkeeran pinging you here, because basically what @whikloj needs is the important json-ld to entity structure part (the one @dannylamb was asking you) from your POST generalized into claw-jsonld.

@whikloj dhlamb: last question: how does PATCH works internally in Drupal if we are not using an query language like SPARLQ-UPDATE here. Does it replace values if the exist? (and how do we know if they exist in the same position?) or will that be always additive?

[whikloj]

@DiegoPino that actually isn't a PR yet. That was just some stuff I had tried in claw-jsonld. If you have something better, just open your own PR and if I have any suggestions I can pull against your PR.

Regarding the question of how PATCH works internally, in the case of JSON it overwrites the values you send as long as you have permissions. In the case of JSON-LD, well we get to decide how it works.

I have been trying to implement the functional tests for the FedoraResource Rest and those seems like a huge pain. I'll add in some simple kernel tests for the things I can think of and open a PR on Islandora.

[dannylamb]

@DiegoPino If you're just using plain old JSON, it replaces entirely. So if you want to add you need to have all the old values plus your new ones. I believe the order in which you provide them becomes the order in which they are persisted and subsequently returned in GET requests.

[dannylamb]

@whikloj Test what you can. Reach out if you want/need help.

[DiegoPino]

@dannylamb @whikloj @ajs6f @acoburn as discussed in IRC today, PATCHING linked data without clear semantics /query-update language is weird/complicated and prone to explode(or not work at all). Good for now of course to have this code, but good to know that if we are going to add this formally to the API in the future we or you all should discuss a lightweight php implementation of an SPARQL-UPDATE parser or going with something like what GraphQL proposes like (mutations)
http://graphql.org/learn/queries/#mutations

Again, not saying this should go in/into/or via PATCH (lol) into the MVP, and not saying anything at all more than: we will need to deal with this anyway in mid-term

[whikloj]

So now my tests are failing due to

4) Drupal\Tests\islandora\Functional\FedoraResourceAnonJsonTest::testPatch
Drupal\Core\Entity\EntityStorageException: preg_match_all() expects
parameter 2 to be string, array given

/var/www/html/drupal/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:770
/var/www/html/drupal/web/core/lib/Drupal/Core/Entity/Entity.php:364
/home/ubuntu/all_claw/islandora/tests/src/Functional/FedoraResourceRestTestBase.php:109
/home/ubuntu/all_claw/islandora/vendor/drupal/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:165

Caused by
PHPUnit_Framework_Error_Warning: preg_match_all() expects parameter 2 to be
string, array given

/var/www/html/drupal/web/modules/contrib/typed_data/src/PlaceholderResolver.php:188
/var/www/html/drupal/web/modules/contrib/rules/src/Plugin/RulesDataProcessor/TokenProcessor.php:63
/var/www/html/drupal/web/modules/contrib/rules/src/Context/ContextHandlerTrait.php:296
/var/www/html/drupal/web/modules/contrib/rules/src/Context/ContextHandlerTrait.php:85
/var/www/html/drupal/web/modules/contrib/rules/src/Plugin/RulesExpression/RulesAction.php:92
/var/www/html/drupal/web/modules/contrib/rules/src/Plugin/RulesExpression/ActionSet.php:31
/var/www/html/drupal/web/modules/contrib/rules/src/Plugin/RulesExpression/Rule.php:94
/var/www/html/drupal/web/modules/contrib/rules/src/Plugin/RulesExpression/ActionSet.php:31
/var/www/html/drupal/web/modules/contrib/rules/src/EventSubscriber/GenericEventSubscriber.php:131
/var/www/html/drupal/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:111
/var/www/html/drupal/web/modules/contrib/rules/rules.module:124
/var/www/html/drupal/web/core/lib/Drupal/Core/Extension/ModuleHandler.php:402
/var/www/html/drupal/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:169
/var/www/html/drupal/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:418
/var/www/html/drupal/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:470
/var/www/html/drupal/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:304
/var/www/html/drupal/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:395
/var/www/html/drupal/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:761
/var/www/html/drupal/web/core/lib/Drupal/Core/Entity/Entity.php:364
/home/ubuntu/all_claw/islandora/tests/src/Functional/FedoraResourceRestTestBase.php:109
/home/ubuntu/all_claw/islandora/vendor/drupal/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:165

That seems to be because the PlaceHolderResolver in typed_data expects the value to be text and we are sending an array

array (
  0 => 'activemq:queue:islandora-indexing-fcrepo',
  1 => 'activemq:queue:islandora-indexing-triplestore',
);

So nothing seems wrong, these tokens are correct. But the resolver can't process the replacement, so the rule fails and the test fails.

weird one.

[DiegoPino]

@whikloj it feels hard to say, but rules and typed data are pretty much beta with some tastes of Alpha, but so are we also, we have to live with it.
It does make me a bit nervous that typed data is not covered by Drupal's security adv. policy as stated in https://www.drupal.org/project/typed_data

Also: i thought there was a typo and a bug (says Mr. typo here) in the spoy your test code was failing:
at RulesDataProcessor/TokenProcessor.php:63

 $placeholders_by_data = $this->placeholder**Resovler**->scan($value);

It is consistently all around the file, so not a bug but a typo "Resovler". Which helps explain maybe why that method is badly implemented, maybe lacked enough reviews... (explained further down because RULES seems to have a bug there)

The PlaceholderResolverInterface is pretty explicit about ::scan method and it defines its only argument as of type text. So what should happen is that ::scan needs to be applied to every array value (making sure they are text!), which would need additionally a cast to array in case the token passed is just a string.

Confirmed by/ allowed in

interface DataProcessorInterface {
at /web/modules/contrib/rules/src/Context/DataProcessorInterface.php, which is implemented in the failing class of Rules.

for its ::process method which allows the argument $value to be of type mixed.

In less number of words: There is a bug in RULES there.

[DiegoPino]

@whikloj 👀
https://www.drupal.org/node/2824331
The Error you are seeing with a patch (exactly what I was suggesting!!) but never merged and already 5 months passed. What should we do?

And that explains/should/could explain why you are lacking certain triggered events?

[dannylamb]

@DiegoPino @whikloj Have you considered applying the patch and seeing if it works? If that fixes the testing issue, you should bump the issue on Drupal.org and see if you can get some action.

[DiegoPino]

@dannylamb the patch works, but will likely be rejected because the coding is lazy and because well nobody is assigned to test it... I mean, there are for sure better ways to code that and typecasting to array before iterating seems more optimal, anyway, it works.

But also, the issue has been open for 5 months.
Do you think we should sometime in the future talk about what type of involvement we need on external projects and in specific Drupal community?Our architecture/approach somehow implies/requires that. @jonathangreen has for sure some ideas on that since he was pretty involved in JWT (but that one had a github account)

[jonathangreen]

@DiegoPino I think that we should be contributing back to Drupal modules anytime we can be. As part of the open source Drupal ecosystem, I think its important that we do so, and that we shepard our patches though review and get them landed in the appropriate modules. Especially now that we are trying to bring Islandora much more into the Drupal ecosystem. In this instance if there is an issue, and the patch isn't great, we should probably take the patch and submit one that is more likely to get accepted.

From what I've been seeing a lot of D8 modules are actually doing most of the development on Github, with DO tickets (like we are doing with JIRA in 7.x). Rules actually has a repo here, with a fair number of PR against it (the repo is buried in the readme on DO). For these repos patches tend to linger, but PR are addressed more quickly. Which makes sense the DO patch workflow is pretty brutal sometimes. Maybe we could submit a PR with a better version of this patch, and tag the maintainer. Might get the issue fixed a little faster.

For the JWT module I tracked down the maintainer on the Drupal IRC channel, and asked him about getting the patches I wanted landed in the module before I started working on them, because there were some pretty major changes there. He seemed happy for the help.

[dannylamb]

@jonathangreen Agreed. And in many ways it's the path of least resistance and most karma to push changes upstream. Certainly beats re-coding already existing functionality elsewhere, which may seem easier at the time, but incurs technical debt in the long term.

[dannylamb]

FYI, there's a PR to address this in Github: fago/rules#480