Now that nodes are converted to EntityNG we should make sure in rest.module that they work in the REST API as expected. I think testing deletion should be easy, we can add that to the existing test case. Maybe also read/GET. Create/POST and Update/PATCH might be trickier and could uncover some node related issues, so that might get separate test methods (or classes).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi’s picture

Status: Active » Needs review
FileSize
4.29 KB

Here is a patch that adds tests for GET and DELETE, which is relatively easy and uses the same code as entity_test.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks like a reasonable way to expand test coverage. We can do more in followups.

klausi’s picture

Status: Reviewed & tested by the community » Needs review

Heh, maybe I should have mentioned that this patch is not finished yet :-)

We need tests for PATCH and POST here as well, since Lin already mentioned problems when POSTing nodes.

klausi’s picture

Adapted creation test to also include nodes, works fine in this minimal version of POSTing the node type and title only.

TODO: test coverage for PATCH.

klausi’s picture

I did some manual POST testing with nodes and it is important that you remove all special fields from your content type that you want to test. Taxonomy term reference fields, file fields etc. do not work right now.

Here is a cURL example request I used successfully to create a primitive article that has a title and a body field:

curl --include --request POST --cookie SESSf722138df952ff0f3000137055cb16f4=OKpUoonJqjekZCKQUw5ZM4kGX--nJ6AN0qWTLZLobrA --header 'Content-type: application/hal+json' --header 'X-CSRF-Token: zC9uOOrmCbOEqDXhgdcEqgFD2ft6RGJzKpTCfUzSJHw' http://drupal-8.localhost/entity/node --data-binary '{"_links":{"self":{"href":"http://drupal-8.localhost/node/"},"type":{"href":"http://drupal-8.localhost/rest/type/node/article"}},"type":[{"value":"article"}],"title":[{"value":"test title"}]}'

You have to replace the cookie and the CSRF token (can be retrieved at /rest/session/token) accordingly.

linclark’s picture

I also had to disable comment module to make this work.

linclark’s picture

The offending code in comment module is this:

function comment_node_insert(EntityInterface $node) {
  // Allow bulk updates and inserts to temporarily disable the
  // maintenance of the {node_comment_statistics} table.
  if (variable_get('comment_maintain_node_statistics', TRUE)) {
    db_insert('node_comment_statistics')
      ->fields(array(
        ......
        'last_comment_uid' => $node->uid,
        ......
      ))
      ->execute();
  }
}

If the $node doesn't have a uid set, then there is an integrity constraint violation because it can't be NULL.

REST module should probably set the $node->uid anyway, using the user id of the user who made the request. I don't think we want a user with POST privileges to be able to create a node and assign it to another uid.

webchick’s picture

Priority: Normal » Major

This sounds like it's probably at least major, given the number of bugs digging into this is uncovering.

linclark’s picture

benjifisher’s picture

#4: node-tests-1972116-4.patch queued for re-testing.

klausi’s picture

Implemented a test case for nodes with comment module enabled, so that a missing node author does not trigger errors in comment module.

What do we want to test for PATCH regarding nodes?

moshe weitzman’s picture

For PATCH, the simplest is to change required fields like created, author, title. More advanced PATCH could be handled in these tests or in serialization (e.g. date fields, entity ref fields, etc.).

klausi’s picture

PATCH is really broken right now, I added a test case to demonstrate that a title field change fails.

The problem is that serializer (the denormalier) uses entity_create() for building the node object. During that default values get applied to the fields, which we do not want in the PATCH case. We only want to update some specific fields on an existing node.

We could pass the node ID to serializer and it could perform an entity_load() to get the existing node, but then we don't know which fields where updated later in REST module in order to perform field access checks on them. So we could also move the access checks into serializer, but then it does way to much business logic outside the scope of just deserializing.

One more time I realize that we need #1965780: Add EntityDiff entity type for handling PATCH requests.

Status: Needs review » Needs work

The last submitted patch, node-tests-1972116-13.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
1.1 KB
20.37 KB

New idea: use a deserialization context to indicate PATCH request data. The denormalizer can use that to simply unset all fields on the prepared entity and then fill it with incoming data. That is one way to get rid of the default values.

This would also mean that we don't need an extra EntityDiff approach. What do you think?

linclark’s picture

I'm at NYC Camp, so haven't had a chance to do a full review yet, but the basic principle sounds good to me.

Do you have a test to ensure that a field can be deleted using the empty array, and that a field that wasn't in the request does not get deleted? I thought we got rid of the distinction between empty values and NULL values in Portland.

klausi’s picture

Deleting field with an empty array works as we expect, we already have a test case for that. I added a test to make sure that omitted fields in a PATCH request do not delete the field.

I also documented the "request_method" context array key on the denormalizer.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me, and much needed.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityNormalizer.phpundefined
@@ -89,6 +101,15 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
+    // Special handling for PATCH: destroy all possible default values that
+    // might have been set on entity creation. We want an "empty" entity that
+    // will only get filled with fields from the data array.
+    if (isset($context['request_method']) && $context['request_method'] == 'patch') {
+      foreach ($entity as $field_name => $field) {
+        $entity->set($field_name, NULL);
+      }

+++ b/core/modules/rest/lib/Drupal/rest/Tests/NodeTest.phpundefined
@@ -0,0 +1,76 @@
+    // Reload the node from the DB and check if the title was correctly updated.
+    $node = entity_load('node', $node->id(), TRUE);
+    $this->assertEqual($node->get('title')->get('value')->getValue(), $new_title);

Shouldn't we be testing the code that prevents changing of values to default here too. That is, shouldn't we be testing other values have not changed.

+++ b/core/modules/rest/lib/Drupal/rest/Tests/UpdateTest.phpundefined
@@ -65,8 +65,18 @@ public function testPatchUpdate() {
+    // Make sure that the field does not get deleted if it is not present in the
+    // PATCH request.
     $normalized = $serializer->normalize($patch_entity, $this->defaultFormat);
+    unset($normalized['field_test_text']);
+    $serialized = $serializer->encode($normalized, $this->defaultFormat);
+    $this->httpRequest('entity/' . $entity_type . '/' . $entity->id(), 'PATCH', $serialized, $this->defaultMimeType);
+    $this->assertResponse(204);
+
+    $entity = entity_load($entity_type, $entity->id(), TRUE);
+    $this->assertNotNull($entity->field_test_text->value. 'Test field has not been deleted.');

So I removed the code in EntityNormalizer::denormalize concerning the patch method (see above) and ran Drupal\rest\Tests\UpdateTest - which still passes. I thought this might be testing that code? I guess not - which means we have no test coverage of it.

klausi’s picture

Status: Needs work » Needs review
FileSize
3.22 KB
24.68 KB

Thanks Alex, very good points! I added an explicit test for the HAL denormalizer + a check for the UUID in the node test.

Status: Needs review » Needs work
Issue tags: -WSCCI

The last submitted patch, node-tests-1972116-20.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI

#20: node-tests-1972116-20.patch queued for re-testing.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

more test coverage is good. back to rtbc.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed cde3c7d and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.