[FEATURE] Provide support for uploading files

Combined with info from duplicate issue #2868787: Support POSTing and PATCHing of file entities.

@e0ipso Feel free to change / holler at me if you don't like ;)

#4 is consistent with the current recommendation from JSONAPI community: https://github.com/json-api/json-api/issues/246#issuecomment-282192434

Happy with the suggested approach. My understanding is that we're leaving the 2nd option for core contributors to provide more comprehensive solution, which though doesn't comply with jsonapi standard because of additional http requests to a seperate endpoint. Having this said, I'm going to work on the implementation of option #1 for json api.

Yes, I'm thinking that waiting on Core's implementation of file uploads probably makes more sense than implementing #2 in the JSON API module. Existing JSON API libraries and clients won't be able to utilize a custom method without adding implementation-specific code, which misses much of the value of using JSON API in the first place.

Great, thanks for the update guys.

I've got one thing to clarify before we get started: am I right saying that currently there is no case / example in JSON API which would support creation of content entity together with a new content entity as a relationship? Initially I was thinking about something like this:

POST /jsonapi/node/article
{
  "data": {
    "type": "node--article",
    "attributes": {
      "title": "My title"
    },
    "relationships": {
      "field_image": {
        "data": [
          {
            "type": "file--file",
            "encodedData": "<base64data>"
          }
        ]
      }
    }
  }
}

However, according to the JSON API specification it looks like we have to create a new file entity first, and then post a new node entity with relationship to the recently created file. The source code confirmed my concern, see this - it is currently required for a related object to have uuid specified (which is correct, according to the specification).

So my Qs are:

1. Am I missing anything obvious? Is there a way to create a content entity with relationship to the not yet existing entity?
2. I had another idea about usage of "meta" property of the related object instead of "data", which actually might eventually work out. However my worry here is if it a valid approach at all.

Speaking about https://www.drupal.org/project/subrequests - I'm not sure if this will be possible to use this module to combine two requests: the second request has to use the value returned from the first request (file uuid). I've briefly checked the module and haven't found this feature there. Going to check Better Normalizers module to see if it solves the issue.

Btw, I might be raising more high level issue now, but have you thought about saving of object in the relationships in the same POST / PATCH request for the parent entity? It looks like JSON API specification doesn't strictly rule this. This would make forms creation / integration on the decoupled frontend much more pleasant task. Avoiding multiple requests would result in a much better dev experience. Even having great initialive like https://www.drupal.org/project/subrequests doesn't seem to resolve all issues with related entities.

Very nice, thanks! Wondering how I've missed that. Anyway, I think it's a part of larger conversation and I've created a separate issue for this #2874751: [DISCUSSION] POST of new entities from relationship alongside with parent entity.

I took a look at Better Normalizers. Here are my thoughts:

1. The module is designed to work with HAL. It needs several fixes to make it work with JSON API.
2. It outputs file entity as base64 data. Not sure if this is needed in JSON API. My understanding is that we need to have denormalizer which would save the base64-encoded file, but the output should be standard.
3. I assume when devs will use JSON API they would expect to have files uploading working out of the box, not as a separate contrib.
4. All features of File Field are getting lost. You can't validate the submitted file: mimetype, filesize, min/max resolution (for images). Also the upload folder for the field is not respected. The concern here is if you upload a file separately then attach it to the entity's file field, it might end up having invalid file (Q1: Will the entity validation raise this issue on entity save? Q2: Should we reject uploading of such file BEFORE it gets into Drupal?)

After having really interesting discussion above, the final plan is:
1. Stick to #6 as a high level concept.
2. Use Better Normalizers as an inspiration.
3. Files should be uploaded to the separate JSON API endpoint /jsonapi/file/file. Keep Subrequests in mind as a solution for multiple operations per single http request.

Discussion is still opened regarding:
1. Shall we mark files as temporary until it's linked to a content entity?
2. Shall we provide an option to output file entity as base64 data?

Mateu or other guys, do you have any strong opinions about the discussion topics in #17?

Mateu, not sure I undertand:

I would defer that to core.

What is the other suggested endpoint to upload a file using JSON API? I thought that we will use /jsonapi/file/file endpoint for this feature.

Right, now I see that we're on the same page. Finally it's time to let the code magic happen :)

I've been working on this issue recently and eventually got it working locally. However, during the development I've found several issues which made me think that this feature can't be added to JSON API:

1. File upload permission. Drupal Core doesn't not define permissions to operate with File entity. Therefore, it's not allowed to create new file entities. What I mean here is that this code always returns FALSE:

$entity_access = $file_entity->access('create', NULL, TRUE);
return $entity_access->isAllowed();

I don't think it should be JSON API's job to add the File entity permissions and to implement hook_file_create_access() to make file uploads work.

2. Security issue. Drupal Core can validate a file only if it's attached to the File Field. Thefore there is no pure file upload validation. It exposes the door for malicious file uploads. We could have some basic file upload validation implemented, but unfortunately Drupal Core doesn't contain list of safe file extensions / mime types to validate against. I don't think we should reinvent the wheel and put into JSON API opinionated list of supported file extensions, mime types and max file upload limit.

Actually, the second issue makes me think that Better Normalizers exposes a security vulnerability, as there is no file upload permission check, as well as no file validation. However, it's a separate story.

Proposed Solution

Both mentioned issues are solved in File Entity module. Therefore my proposed solution would be to create a new contrib which would use File Entity as a depencendy, and then add (de)normalizer to get the file uploads working properly. In the first version it will support only "api_json" format, but it should be very easy to extend it to support other formats.

Mateu, could you please clarify: you mean you'd like to have this as a submodule of JSON API which ships together with the main module, or get it implemented straight into the main module? In the first case we could use power of File Entity module, while in the second case we will have to define additional permissions / configs just for file upload (and potentially override them when File Entity module is enabled, to keep the consistency across all files management).

Personally I'd really prefer the first option, because the second looks more like a workaround. However, you're the lead of this project and I trust your judgment.

Okay, this approach is a good trade-off I think. However, 2 potential issues kick in:

1. Testability. I'm not sure that Drupal.org test bot can support third-party contrib modules if they are not part of module's dependencies (needs additional info).

2. I've heard (tell me it was not just a dream) that JSON API might eventually end up in Drupal core. So I assume it can't contain references to third-party contrib modules.

Have I missed anything?

#2831274: Bring Media entity module to core as Media module This is interesting. I took a look at the resulting patch and think that in the future it can be a good replacement for the File Entity integration. I'm assuming that for now it's okay to rely on File Entity, but in the future in might be replaced by Media when both get in core.

The question regarding testing is still opened. Will update the issue as soon as I find out the answer.

Found resolution for the first issue in #28 regarding testability. If we add test_dependencies to the module's *.info.yml file then Drupal.org's test bot will download necessary dependencies. However, there's a small issue with this, see this article: if we add test_dependencies and tests into the same patch - it will not work. But I'm sure we will work around this.

The main thing now is that we know the approach to take, no more blockers. Happy to move on with the development.

Attaching the first patch version just to get the balls rolling. File uploads already work, though we need to test it additionally.
My next step will be to cover new functionality with tests. Critics and feedback for the given patch are welcome.

Looks like check of enabled File Entity during containers registration was not the best idea. Fixed the issue, re-submitting the new patch.

Replaced usage of trait_exists(), as it seems to be unstable.

Notes on usage:
1. Install File Entity module.
2. Configure permission to upload new files.
3. Send POST request to /jsonapi/file/document (instead of document you can use any other file entity bundle):

{
  "data": {
    "type": "file--document",
    "attributes": {
      "data": "SGV5LCBpdCB3b3JrcyE=",
      "filename": "file.txt"
    }
  }
}

Got inspired by examples in Drupal core and enhanced registration of new serializer.normalizer.file.jsonapi service on the fly depending on File Entity availability. The main logic remained untouched.

Getting to tests coverage. Mateu, due to the issue described in #31 with test dependencies, do you mind committing the attached patch ahead of the main one? Otherwise all my upcoming patches with tests will be failing.

Thanks for the feedback, @e0ipso.

1. In the FileEntityNormalizer class we use UploadValidatorsTrait which comes with File Entity module. So without the module the class throws error, because the trait is not found. Alternative solution here is to copy-paste the necessary trait's method to FileEntityNormalizer - then we will be able to define the service in .yml file and check File Entity availability in supportsDenormalization(). However, I'm not sure that this is the best approach.
2. Done.
3. Yes, see Unicode::strlen()
4. Fixed
5. Fixed
6. Fixed

The further progress is blocked by #2877589: Fix failing test in the dev branch, because tests fail.

The blocker is cleared now, submitting the patch for re-testing.

All tests pass now, which is great. Our next step should be getting this patch into the module. Otherwise, as I've already mentioned, all tests relying on File Entity will be failing, because d.org test bot download dependencies before patch is applied, and not after.

@e0ipso, let me know if you have any questions or concerns.

Thanks for your hard work on this! I've been focusing most of my time on other work, and lost track of this. It was great to come back and see how much progress there has been!

Some review notes/questions based on #40:

1. +++ b/src/Normalizer/FileEntityNormalizer.php
   @@ -0,0 +1,205 @@
   +    $file->setMimeType($mime_type);
   

   If the POST specifies a MIME type, should this take precedence over the guessed MIME type? How reliable is the guesser?

2. +++ b/src/Normalizer/FileEntityNormalizer.php
   @@ -0,0 +1,205 @@
   +    $destination = file_default_scheme() . '://' . $file->getFilename();
   

   I can see this as a requirement for security reasons, but I think we should add a "hook" of some sort here to, for example, allow setting the destination path based on the JSON API request's "uri" attribute. Maybe a getDestination() method that can be overridden in a subclass?

3. +++ b/src/Normalizer/FileEntityNormalizer.php
   @@ -0,0 +1,205 @@
   +    $allow_insecure_uploads = $this->configFactory->get('system.file')
   +      ->get('allow_insecure_uploads');
   +    $insecure_filename = preg_match('/\.(php|pl|py|cgi|asp|js)(\.|$)/i', $file->getFilename());
   +    $text_file = substr($file->getFilename(), -4) == '.txt';
   +    if (!$allow_insecure_uploads && $insecure_filename && !$text_file) {
   +      $file->setMimeType('text/plain');
   +      // The destination filename will also later be used to create the URI.
   +      $file->setFilename($file->getFilename() . '.txt');
   +      // The .txt extension may not be in the allowed list of extensions. We
   +      // have to add it here or else the file upload will fail.
   +      if (!empty($allowed_extensions)) {
   +        $validators['file_validate_extensions'][0] .= ' txt';
   +      }
   +    }
   

   This code seems familiar. Did you pull it in from elsewhere? Is it possible to reuse the code from its original module/class/location rather than duplicating it here?

In response to #44, #40, #26, #27: This may be an indication that this functionality really does belong in a submodule which has a dependency on the File Entity module. Doing so would be consistent with how other Drupal contrib modules have generally been organized.

I definitely like the idea of keeping JSON API as much of a "one-click install" as possible, but this may be a case where sticking to that ideal could lead to more confusion, rather than less.

Thanks @hampercm for the review. Here's my feedback:

1. Very good question. I took a look inside of FileEntity module, and it looks like file mimetype gets set explicitely (see FileEntity::preSave(). So there is no option to send mimetype from the frontend app. Having this said, there is no point in setting mimetype at jsonapi end as well. I've removed it.

2. Right now you can set filename to something like "subfolder/filename.jpg" and it will put the file to the correct subfolder. I believe it covers most of the use cases (definitely worth adding to the docs). If developers will want something more advanced - I advocate for a separate issue dedicated to the specific use case.

3. Great memory :) It was taken from file_save_upload. Unfortunately, there is no chance we could reuse some of that code, because it's just a huge function, which contains only several relevant partials.

Re #48:

2. ... Right now you can set filename to something like "subfolder/filename.jpg" and it will put the file to the correct subfolder.

Good to know there is a work-around without requiring custom code, but that approach feels a bit awkward: it is inconsistent with how the JSON API operates when GETing a resource. I'd prefer using the uri attribute when POSTing files, and deriving the filename from that value, if that is possible.

3. ... Unfortunately, there is no chance we could reuse some of that code, because it's just a huge function, which contains only several relevant partials.

Thanks for the details :) I figured it was probably something like that.

Can we create a follow up ticket to cover: definitely worth adding to the docs

Yup, done #2878200: Provide documentation for files upload.

Good to know there is a work-around without requiring custom code, but that approach feels a bit awkward: it is inconsistent with how the JSON API operates when GETing a resource. I'd prefer using the uri attribute when POSTing files, and deriving the filename from that value, if that is possible.

Fair enough. It provides better DX for frontend devs. However, I've got two concerns:
1. How fair is to require from frontend developers to understand the file streams of Drupal? This public:// and other stuff?
2. Will it open any potential security issues, if frontend apps will decide how to store files?

#38 is not committed. Should it?

Yes, it should go into the module ahead of the main patch.

Great work here!

However, test coverage is definitely very much needed for this to land.

@Spleshka: have you looked at #1927648: Allow creation of file entities from binary data via REST requests at all?

@Wim, yes, I did. It's a bit different case, because the referenced issue uploads file to the field, which has better core support than a pure file upload. However, it has several takeaways which potentially applicable to this story, so I assume it makes sense to bring them to the table:
1. If specified destination is not writable - the file upload fails, while the patch in #48 tries to make the destination writable first, if possible. The first approach seems to be safer, while the latter - more developer-friendly. Not sure if there is any security issue in letting frontend app to decide which folders to create within the public:// steam. Please let me know if there are any.
2. The filename is taken from the Content-Disposition header. I doubt this is applicable to jsonapi, I just found it curious.
3. The default filename is set to uniqid() until explicit filename is specified.
Personally I can't imagine a good use case where random filename with unspecified mimetype would be beneficial. However, I'm opened for other opinions.
4. File is being written to the temporary folder first (broken by 8192 bytes pieces), then moved to the destination. I'd really appreciate if anyone can explain the rational behind this. I feel it might be potentially beneficial for jsonapi's case as well.

tries to make the destination writable first, if possible

This sounds dangerous…

The filename is taken from the Content-Disposition header

This is still being debated. Also: a big part of the reason is simply that the request body is in application/octet-stream format, to allow the file to be uploaded efficiently. Therefore there can't be JSON in the request body. Hence a request header is used.

The default filename is set to uniqid() until explicit filename is specified.

That is just WIP code, you can see the @todo just below.

I'd really appreciate if anyone can explain the rational behind this. I feel it might be potentially beneficial for jsonapi's case as well

Note that the core patch allows uploads of an arbitrary size, without violating the PHP memory limit.

Please post your remarks as a review on #1927648: Allow creation of file entities from binary data via REST requests rather than observing differences here. Let's collaborate! :)

Hey guys, thanks for the feedback.

This sounds dangerous…

My favorite answer : yes and no. I usually become paranoic every single time I need to let the code to change something in the file system. However, it seems to be the only viable way to let frontend app to decide the folders structure. In the related core issue #1927648: Allow creation of file entities from binary data via REST requests it's possible to set the file upload folder in field settings and it will be created and prepared by Drupal. II have already mentioned this issue in #16. Unfortunately, the direct file upload doesn't have this option available. We don't want to turn the root public:// folder into the files hell. If there's a good security reason to disable this feature, then we might want to hardcode the uploaded folder to public://jsonapi_files or provide a small configuration option for a list of allowed folders to use.

Let's collaborate! :)

Oh yes, I'm all up for this! I'm just getting back into active contribution after several years of offline stuff, so still getting up to speed. I've decided so far to avoid participiation in too many issues at the same time: I'd better work on 5-10 issues per month with guaranteed delivery than being everywhere and nowhere at the same time :) As most of other contributors, I do this outside of working hours, so have to be very careful about the time usage, just to make sure it brings the max benefit into the community.

We talked about this in the API-First meeting yesterday and concluded that the best way to move forward it to land the core issue and have uniformity in here.

Looks like I missed a good party yesterday! My 2 coins (or 50 cents) regarding the file upload issue:

1. I'm not sure if there's a good reason to postpone the progress. What would I do is to make the patch look good for everyone, get it in, and then keep monitoring the related issue for possible beneficial takeaways. Then we can simply enhance the feature through follow-ups. I would not expect any dramatical changes in the API, because the core issue works around file field uploads, which will never be the case for JSON API.

2. I want to be 100% clear that I'm not against the idea of putting the patch into a submodule for JSON API, or JSON API Extras, or even as a standalone contrib. The goal here is always to get the feature working, and I'm happy with either option we all find the most beneficial.

Does that sound like a good compromise?

It does, happy with this option. If no objections, I'll create the contrib within the next day or two. Possible names:

• jsonapi_files
• jsonapi_file
• jsonapi_file_entity
• jsonapi_file_upload

Thanks so much for helping out here, of all the available contrib <3

I did mention it in Twitter and going to repeat myself: you deserve a good help with maintenance of all your headless contribs. JSON API is not just yet another module, it's the whole development philosophy.

As promised, got the module published: meet JSON API File. I've made a small enhancement to it: according to #50, the uri field became mandatory instead of filename, which makes more sense to me. All other issues could be created against the new module.

Thanks for all your input guys, file uploads is finally available for JSON API!

P.S. Gave co-maintainer perms to @e0ipso, because I trust him enough to manage submodules for JSON API :)