Problem/Motivation

When using the hal_json format with an entity that has a file field the hal_json output on GET does not provide enough information to access the file entity via the REST API.

I discovered this while reviewing #1927648: Allow creation of file entities from binary data via REST requests

I wanted to GET a node with a file field then be able to either GET, POST, PATCH or DELETE the file referenced. The example was working with had both a image and file field.

Here is the node GET response, the example has an image field, a public:// file field and a private:// file field.

{
  "_links": {
    "self": {
      "href": "http://d8.dev/node/37?_format=hal_json"
    },
    "type": {
      "href": "http://d8.dev/rest/type/node/article"
    },
    "http://d8.dev/rest/relation/node/article/revision_uid": [
      {
        "href": "http://d8.dev/user/1?_format=hal_json"
      }
    ],
    "http://d8.dev/rest/relation/node/article/uid": [
      {
        "href": "http://d8.dev/user/1?_format=hal_json",
        "lang": "en"
      }
    ],
    "http://d8.dev/rest/relation/node/article/field_image": [
      {
        "href": "http://d8.dev/sites/default/files/2017-09/webform1.png",
        "lang": "en"
      }
    ],
    "http://d8.dev/rest/relation/node/article/field_private_file": [
      {
        "href": "http://d8.dev/system/files/2017-09/interdiff-109-115.txt"
      }
    ],
    "http://d8.dev/rest/relation/node/article/field_public_file": [
      {
        "href": "http://d8.dev/sites/default/files/2017-09/interdiff-275-277.txt"
      }
    ],
    "http://d8.dev/rest/relation/node/article/field_tags": [
      {
        "href": "http://d8.dev/taxonomy/term/1?_format=hal_json",
        "lang": "en"
      }
    ]
  },
  "nid": [
    {
      "value": 37
    }
  ],
  "uuid": [
    {
      "value": "aef1528d-aedb-4afc-a20a-755a71805937"
    }
  ],
  "vid": [
    {
      "value": 53
    }
  ],
  "langcode": [
    {
      "value": "en",
      "lang": "en"
    }
  ],
  "type": [
    {
      "target_id": "article"
    }
  ],
  "revision_timestamp": [
    {
      "value": "2017-09-07T19:33:55+00:00",
      "format": "Y-m-d\\TH:i:sP"
    }
  ],
  "_embedded": {
    "http://d8.dev/rest/relation/node/article/revision_uid": [
      {
        "_links": {
          "self": {
            "href": "http://d8.dev/user/1?_format=hal_json"
          },
          "type": {
            "href": "http://d8.dev/rest/type/user/user"
          }
        },
        "uuid": [
          {
            "value": "3d76e74e-25b5-45e9-af89-3d94f253b46c"
          }
        ]
      }
    ],
    "http://d8.dev/rest/relation/node/article/uid": [
      {
        "_links": {
          "self": {
            "href": "http://d8.dev/user/1?_format=hal_json"
          },
          "type": {
            "href": "http://d8.dev/rest/type/user/user"
          }
        },
        "uuid": [
          {
            "value": "3d76e74e-25b5-45e9-af89-3d94f253b46c"
          }
        ],
        "lang": "en"
      }
    ],
    "http://d8.dev/rest/relation/node/article/field_image": [
      {
        "_links": {
          "self": {
            "href": "http://d8.dev/sites/default/files/2017-09/webform1.png"
          },
          "type": {
            "href": "http://d8.dev/rest/type/file/file"
          }
        },
        "uuid": [
          {
            "value": "ce48df6b-83e9-47b8-9875-c2fe391ef9b1"
          }
        ],
        "uri": [
          {
            "value": "http://d8.dev/sites/default/files/2017-09/webform1.png"
          }
        ],
        "lang": "en"
      }
    ],
    "http://d8.dev/rest/relation/node/article/field_private_file": [
      {
        "_links": {
          "self": {
            "href": "http://d8.dev/system/files/2017-09/interdiff-109-115.txt"
          },
          "type": {
            "href": "http://d8.dev/rest/type/file/file"
          }
        },
        "uuid": [
          {
            "value": "50ab6ba6-7a86-4027-a86d-a26ebf0e09b5"
          }
        ],
        "uri": [
          {
            "value": "http://d8.dev/system/files/2017-09/interdiff-109-115.txt"
          }
        ]
      }
    ],
    "http://d8.dev/rest/relation/node/article/field_public_file": [
      {
        "_links": {
          "self": {
            "href": "http://d8.dev/sites/default/files/2017-09/interdiff-275-277.txt"
          },
          "type": {
            "href": "http://d8.dev/rest/type/file/file"
          }
        },
        "uuid": [
          {
            "value": "bb9ed1d1-f613-4647-b932-2e2f52143bfd"
          }
        ],
        "uri": [
          {
            "value": "http://d8.dev/sites/default/files/2017-09/interdiff-275-277.txt"
          }
        ]
      }
    ],
    "http://d8.dev/rest/relation/node/article/field_tags": [
      {
        "_links": {
          "self": {
            "href": "http://d8.dev/taxonomy/term/1?_format=hal_json"
          },
          "type": {
            "href": "http://d8.dev/rest/type/taxonomy_term/tags"
          }
        },
        "uuid": [
          {
            "value": "5228c13c-265a-42d1-bbca-ea4a30385cfc"
          }
        ],
        "lang": "en"
      }
    ]
  },
  "status": [
    {
      "value": true,
      "lang": "en"
    }
  ],
  "title": [
    {
      "value": "uuuuu",
      "lang": "en"
    }
  ],
  "created": [
    {
      "value": "2017-09-06T14:22:27+00:00",
      "lang": "en",
      "format": "Y-m-d\\TH:i:sP"
    }
  ],
  "changed": [
    {
      "value": "2017-09-07T19:33:55+00:00",
      "lang": "en",
      "format": "Y-m-d\\TH:i:sP"
    }
  ],
  "promote": [
    {
      "value": true,
      "lang": "en"
    }
  ],
  "sticky": [
    {
      "value": false,
      "lang": "en"
    }
  ],
  "default_langcode": [
    {
      "value": true,
      "lang": "en"
    }
  ],
  "revision_translation_affected": [
    {
      "value": true,
      "lang": "en"
    }
  ],
  "path": [
    {
      "alias": null,
      "pid": null,
      "langcode": "en",
      "lang": "en"
    }
  ],
  "content_translation_source": [
    {
      "value": "und",
      "lang": "en"
    }
  ],
  "content_translation_outdated": [
    {
      "value": false,
      "lang": "en"
    }
  ],
  "comment": [
    {
      "status": 2,
      "cid": 0,
      "last_comment_timestamp": 1504707773,
      "last_comment_name": null,
      "last_comment_uid": 1,
      "comment_count": 0,
      "lang": "en"
    }
  ]
}

Nowhere in the response do I have access to the file entity's id or url to access the file entity via the REST API.
This not true for the other entity reference fields.
For the term I am given: "href": "http://d8.dev/taxonomy/term/1?_format=hal_json
and for the user I am given: "href": "http://d8.dev/taxonomy/term/1?_format=hal_json

Therefore I wanted to view, update or delete the file entity I would not be able to via REST.

Proposed resolution

Not sure but somehow return http://d8.dev/entity/file/1?_format=hal_json

Remaining tasks

User interface changes

API changes

Response for hal_json normalized file fields would be changed.

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

Wim Leers’s picture

Title: Hal normalization for file fields don't provide file id or access to rest link » HAL normalization of file fields don't provide file entity id or file entity REST URL
Issue summary: View changes
Issue tags: +API-First Initiative
Wim Leers’s picture

Assigned: Unassigned » damiankloip

The root cause:
\Drupal\hal\Normalizer\FileEntityNormalizer::normalize():

  public function normalize($entity, $format = NULL, array $context = []) {
    $data = parent::normalize($entity, $format, $context);
    // Replace the file url with a full url for the file.
    $data['uri'][0]['value'] = $this->getEntityUri($entity);

    return $data;
  }

Assigning to @damiankloip for feedback.

Wim Leers’s picture

Quoting #1927648-426: Allow creation of file entities from binary data via REST requests:

The "return full URL" thing was added in #1988426: Support deserialization of file entities in HAL. The IS there says:

Temporarily, create a FileEntityNormalizer. Once File is an EntityNG entity, and its properties are handled as such, this could potentially be handled in a field normalizer instead.

This follow-up never happened, in part because there was no @todo

Ironically, you're the one who added this in the first place, at #1988426-25: Support deserialization of file entities in HAL 😀

The goal AFAICT was always to add sensible properties to access the full URL…

Wim Leers’s picture

See #2825487-141: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field:

Related: #2907402: HAL normalization of file fields don't provide file entity id or file entity REST URL. Ironically, that is a bug reporting exactly what is wrong in HEAD, but we can't fix HEAD because it'd break BC. Which is why I explained in #128 why we need this BC layer (which the patch now has). Confusing? Yes!

So, AFAICT, #2907402: HAL normalization of file fields don't provide file entity id or file entity REST URL is unfixable. It can only be fixed on existing sites by having them opt out of the BC layer. This issue will fix it on new sites though.

Thoughts, @damiankloip? Do you agree?

damiankloip’s picture

Hmm, I think with #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field we will get something like this with the new behaviour:

"http://d8.dev/rest/relation/node/article/field_image": [
      {
        "_links": {
          "self": {
            "href": "http://d8.dev/sites/default/files/2017-09/webform1.png"
          },
          "type": {
            "href": "http://d8.dev/rest/type/file/file"
          }
        },
        "uuid": [
          {
            "value": "ce48df6b-83e9-47b8-9875-c2fe391ef9b1"
          }
        ],
        "uri": [
          {
            "value": "public://2017-09/webform1.png",
            "url": "/sites/default/files/2017-09/webform1.png",
          }
        ],
        "lang": "en"
      }
    ],

So, I think we will still have no way to get the file resource url (entity/file/1?_format=hal_json). However, the file entity also doesn't have any link templates to actually construct this? '_links.self.href' will still return the url created by file_create_url(), as it calls $file->url() from \Drupal\hal\Normalizer\ContentEntityNormalizer::getEntityUri().

It seems like it would be more correct to return what you suggest, not sure we can due to BC concerns though?

Wim Leers’s picture

not sure we can due to BC concerns though?

Indeed :(

But so then how do we evolve this from "it doesn't make sense right now but we can't change it due to BC" to a place where it does make sense, gradually, without breaking BC?

It seems impossible?

Oh heh, I'm basically again arriving at the same conclusion I already posted in #5/#2825487-141: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field:

So, AFAICT, #2907402: HAL normalization of file fields don't provide file entity id or file entity REST URL is unfixable. It can only be fixed on existing sites by having them opt out of the BC layer. This issue will fix it on new sites though.

Do you agree? That'd mean this is Closed (works as designed) and #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field would fix it for new sites, but wouldn't fix it for existing sites.

damiankloip’s picture

Hmm, I was thinking in #6 that [#12357069] doesn't actually fix it for any sites. Yes, it would give them the uri field value and (new) url property, but I think @tedbow is getting at the fact that he is wanting to find something like /entity/file/1 - to be able to arrive at the file entity endpoint from a link here.

This might well not be fixable. To me (from #6 too) is the crux of the problem:

So, I think we will still have no way to get the file resource url (entity/file/1?_format=hal_json). However, the file entity also doesn't have any link templates to actually construct this? '_links.self.href' will still return the url created by file_create_url(), as it calls $file->url() from \Drupal\hal\Normalizer\ContentEntityNormalizer::getEntityUri().

Hal generically gets links to the entity resource, but because file entity is 'special' that yields the path to the file the file entity represents, not the entity itself.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

Status: Closed (duplicate) » Needs review
FileSize
18.62 KB

Actually, I decided to re-open this after talking with @alexpott. Posting the patch from the other issue here. Not sure yet if we'll only do the addition here, in which case we wouldn't need any setting or so. And then deal with files entirely in #2925520: Add a computed 'file_url' property to FileItem (for exposing file URL in file field normalization)

Berdir’s picture

Ok, this is now solely a backwards-compatible addition/bugfix (empty self.href definition), all file stuff removed. Might still get into 8.8 then?

Could still do a change record I suppose.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I first found #3009854-39: Fix "The "serializer.normalizer.file_entity.hal" normalizer service is deprecated: it is obsolete, it only remains available for backwards compatibility." deprecation error and was very confused. I wondered:

[…] but the _links.self URI was a desirable change?

… but #14 does exactly that; it adds that _links.self URI 🙂

I think this is a sensible improvement.

Berdir’s picture

Version: 8.8.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Needs review
FileSize
6.71 KB

Rerolled, not sure why this wasn't set back to needs work.

neel24’s picture

FileSize
1006 bytes

Attaching interdiff for patch in #14 and #16.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 2907402-16.patch, failed testing. View results

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

Random fail in ckeditor tests.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 2907402-16.patch, failed testing. View results

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

Again a random fail, in 1) Drupal\Tests\taxonomy\Functional\TermAutocompleteTest::testAutocompleteOrderedResults, see https://www.drupal.org/pift-ci-job/1531654.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

  • catch committed df12720 on 9.1.x
    Issue #2907402 by Berdir, neel24, Wim Leers, damiankloip, tedbow: HAL...

  • catch committed 0e496e7 on 9.0.x
    Issue #2907402 by Berdir, neel24, Wim Leers, damiankloip, tedbow: HAL...

  • catch committed 32a93fa on 8.9.x
    Issue #2907402 by Berdir, neel24, Wim Leers, damiankloip, tedbow: HAL...
catch’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.0.x and cherry-picked back to 8.9.x, I think parity between 9.x and 8.9 makes this a good backport candidate. Thanks everyone!

Wim Leers’s picture

🥳

Status: Fixed » Closed (fixed)

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