Add a file upload utility #818

rthideaway · 2019-06-24T13:48:05Z

No description provided.

codecov · 2019-06-24T13:59:22Z

Codecov Report

Merging #818 into 8.x-4.x will decrease coverage by 4.92%.
The diff coverage is 0%.

@@              Coverage Diff              @@
##             8.x-4.x     #818      +/-   ##
=============================================
- Coverage      76.08%   71.15%   -4.93%     
- Complexity       583      606      +23     
=============================================
  Files             85       86       +1     
  Lines           1271     1359      +88     
=============================================
  Hits             967      967              
- Misses           304      392      +88

Impacted Files	Coverage Δ	Complexity Δ
src/GraphQL/Utility/FileUpload.php	`0% <0%> (ø)`	`23 <23> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cb3594...f4ab005. Read the comment docs.

codecov · 2019-06-24T13:59:22Z

Codecov Report

❗ No coverage uploaded for pull request base (8.x-4.x@a7328ad). Click here to learn what that means.
The diff coverage is 41.54%.

@@            Coverage Diff             @@
##             8.x-4.x     #818   +/-   ##
==========================================
  Coverage           ?   51.68%           
  Complexity         ?      651           
==========================================
  Files              ?      115           
  Lines              ?     1662           
  Branches           ?        0           
==========================================
  Hits               ?      859           
  Misses             ?      803           
  Partials           ?        0

Impacted Files	Coverage Δ	Complexity Δ
src/Access/ExplorerAccessCheck.php	`0.00% <0.00%> (ø)`	`2.00 <2.00> (?)`
src/Access/VoyagerAccessCheck.php	`0.00% <0.00%> (ø)`	`2.00 <2.00> (?)`
src/Cache/Context/StaticCacheContext.php	`0.00% <0.00%> (ø)`	`4.00 <4.00> (?)`
src/Controller/SubrequestExtractionController.php	`0.00% <0.00%> (ø)`	`1.00 <0.00> (?)`
src/GraphQL/Buffers/EntityBuffer.php	`80.76% <0.00%> (ø)`	`7.00 <0.00> (?)`
src/GraphQL/Buffers/EntityRevisionBuffer.php	`0.00% <0.00%> (ø)`	`7.00 <7.00> (?)`
src/GraphQL/Resolver/Condition.php	`0.00% <0.00%> (ø)`	`8.00 <8.00> (?)`
src/GraphQL/Resolver/Context.php	`0.00% <0.00%> (ø)`	`5.00 <5.00> (?)`
src/GraphQL/Resolver/Map.php	`0.00% <0.00%> (ø)`	`4.00 <4.00> (?)`
src/GraphQL/Resolver/Path.php	`0.00% <0.00%> (ø)`	`4.00 <4.00> (?)`
... and 76 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7328ad...40f23c8. Read the comment docs.

fubhy · 2019-06-26T16:39:37Z

@rthideaway Instead of making this a service, this should just be a DataProducer. Can you rewrite it accordingly? To understand how file uploads work in this module, check out this test: https://github.com/drupal-graphql/graphql/blob/8.x-4.x/tests/src/Kernel/Framework/UploadMutationTest.php

fubhy · 2019-06-26T16:43:44Z

To understand how the file makes its way from the file content of the request into the variable, check out this: https://github.com/drupal-graphql/graphql/blob/8.x-4.x/src/Routing/QueryRouteEnhancer.php#L38-L42

rthideaway · 2019-07-23T11:37:00Z

will address this today/tomorrow.

rthideaway · 2019-07-24T08:53:40Z

@fubhy how did imagine this to work? What should be the outcome of data producer, the File entity? Was playing with it but figured out that having separate file upload data producer makes the logic more complicated for us.

Imagine the following very basic use case:

Given I have a mutation which creates a Resume (Profile entity) along with cv file attached to some file field of Resume (Profile entity)

How the file upload data producer is integrated to it? Because if there should be separate file upload data producer upfront, I guess we need to use compose. But how? File data producer needs some file field config metadata so it can validate the file (list of allowed extensions, max upload size, destination file path, etc). So my compose already needs three steps:

Get the file field config settings of the file we are going to attach. Make contexts out of it for file data producer.
Inject as contexts to file data producer, validate file, eventually create File entity
Create resume. But here I can find out that user don't have permission to create a Resume and all the file upload stuff was uselessly executed. File is already in destination, which I don't want.

In theory also, what if I don't know to which file field I am uploading beforehand as it will be decided during execution of Resume data producer. Can't think of any case now, but for me the file upload works better when it's processed within specific data producer as a service. Like with this resume, if user does not have permission to create a resume I simply don't process the file, right?

Am I missing something here? Sorry if yes :)

fubhy · 2019-07-24T09:02:17Z

That all sounds like it might be better done in a Trait then?! Or a service after all? Mutations are different and more complicated than normal field resolvers I guess and harder to compose.

rthideaway · 2019-07-24T09:07:16Z

We already have a service (in this patch). But trait is fine for me as well. Maybe you could take a look at this patch what would you change here? The thing is we need to also get the violations of that trait/service, so we can display them to user (incorrect file extension, file too big, etc)

fubhy · 2019-07-24T09:20:36Z

src/GraphQL/Utility/FileUpload.php

+   * @param mixed $violations
+   *   List of violations.
+   */
+  protected function registerViolations($violations) {


This is a service. You can't store stuff within it across potentially multiple mutations or you'll leak violations from one mutations into another.

The service itself (if we add one) should only do low level operations worthy of a service that do not assume too many concrete implementation details.

Atm, there are only two public methods, getViolations and createTemporaryFileUpload. The second one is resetting violations at the start of the process. We used it like calling method for creating file and when empty then getting violations. Next mutation can be called without interfering the first one, as the violations got reseted.

But agree this does not look right. So what do you suggest to do here then, how to return violations? Some object? Could return something like FileUploadResponse, would be that ok?

will just try something and let's see

I'd like to avoid side-effects as much as possible in our code base. Hence, returning a wrapper object instead might make sense indeed.

fubhy · 2019-07-24T09:24:18Z

src/GraphQL/Utility/FileUpload.php

+   * @param array $settings
+   *   Fle settings as specified in regular file field config.
+   *
+   * @return \Drupal\file\FileInterface|bool


Instead of returning FALSE i'd vote for returning NULL in case of a failure.

fubhy · 2019-07-24T09:24:30Z

src/GraphQL/Utility/FileUpload.php

+        $this->logger->error(sprintf('Unknown error while uploading the file "%s".', $file->getFilename()));
+        return FALSE;
+    }
+    $upload_dir = $settings['uri_scheme'] . '://' . trim($settings['file_directory'], '/');


We use camelCase across the board!

src/GraphQL/Utility/FileUpload.php

+    $name = $file->getClientOriginalName();
+    $mime = $this->mimeTypeGuesser->guess($name);
+    $destination = $this->fileSystem->getDestinationFilename("{$upload_dir}/{$name}", $this->fileSystem::EXISTS_RENAME);
+    // Begin building file entity.


fubhy · 2019-07-24T10:10:10Z

src/GraphQL/Utility/FileUpload.php

+   * @param mixed $violations
+   *   List of violations.
+   */
+  protected function registerViolations($violations) {


I'd like to avoid side-effects as much as possible in our code base. Hence, returning a wrapper object instead might make sense indeed.

rthideaway · 2019-07-24T22:04:56Z

@fubhy addressed your remarks, please re-check it, once you find the time

bird-cage · 2020-06-19T22:01:12Z

This would be a useful feature.
Is there any intention of finishing and merging this?

klausi

So I see 3 things we need to do here:

Adapt Response objects to structure from #1013
Add test overage. This is security critical code since file uploads are a big attack vector.
Update documentation how you have to assemble a file upload in your custom data producer

We could split up 2. and 3. into new pull requests to not make this too long?

klausi · 2020-06-24T13:51:35Z

src/Wrappers/FileUploadResponse.php

+/**
+ * File upload response wrapper.
+ */
+class FileUploadResponse implements FileUploadResponseInterface {


we now started a Response namespace in #1013 . We should put any new response classes into this same namespace for consistency.

We can probably unify this with the ResponseInterface we have now?

joaogarin · 2020-06-27T09:34:31Z

I could take care of number 3 no problem. @rthideaway would you adapt this to the new structure (1) and maybe make a test for it

klausi · 2020-11-11T08:51:22Z

I will look into test coverage now and what we could do here.

klausi · 2020-11-11T11:04:38Z

src/GraphQL/Utility/FileUpload.php

+    $validators['file_validate_size'] = [$this->getMaxUploadSize($settings)];
+
+    // Add the extension check if necessary.
+    if (!empty($settings['file_extensions'])) {


For security reasons we should have a default value of "txt" here if the user forgot to configure allowed file extensions by accident.

klausi · 2020-11-11T17:21:21Z

First set of tests created in #1112

klausi · 2020-11-17T09:56:42Z

Merging this here for credits, will then merge #1112

File upload GraphQL service.

f4ab005

rthideaway mentioned this pull request Jun 24, 2019

Introduce file upload service to be used in mutations #819

Closed

This comment has been minimized.

Sign in to view

rthideaway added 2 commits June 24, 2019 16:17

Fix comment.

00ddf2d

Namespace and service name fix.

79f807e

fubhy changed the title ~~Introduce file upload GraphQL service.~~ Add file upload data producer. Jun 26, 2019

fubhy requested changes Jul 24, 2019

View reviewed changes

rthideaway added 2 commits July 24, 2019 23:56

Merge branch '8.x-4.x' into feature/file-upload-service

3ad436a

Return FileUploadResponse for FileUpload service.

0118de6

Merge branch '8.x-4.x' into feature/file-upload-service

d2291da

fubhy changed the title ~~Add file upload data producer.~~ Add a file upload utility Oct 15, 2019

pmelab added the 4.x label Oct 30, 2019

Merge branch '8.x-4.x' into feature/file-upload-service

794ec4f

klausi requested changes Jun 24, 2020

View reviewed changes

Merge branch '8.x-4.x' into feature/file-upload-service

40f23c8

klausi reviewed Nov 11, 2020

View reviewed changes

klausi mentioned this pull request Nov 11, 2020

feat(files): Add File upload service wit tests #1112

Merged

klausi merged commit 86d503f into drupal-graphql:8.x-4.x Nov 17, 2020

Add a file upload utility #818

Add a file upload utility #818

Uh oh!

Conversation

rthideaway commented Jun 24, 2019

Uh oh!

This comment has been minimized.

codecov bot commented Jun 24, 2019

Codecov Report

Uh oh!

codecov bot commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fubhy commented Jun 26, 2019

Uh oh!

fubhy commented Jun 26, 2019

Uh oh!

rthideaway commented Jul 23, 2019

Uh oh!

rthideaway commented Jul 24, 2019

Uh oh!

fubhy commented Jul 24, 2019

Uh oh!

rthideaway commented Jul 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rthideaway commented Jul 24, 2019

Uh oh!

bird-cage commented Jun 19, 2020

Uh oh!

klausi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaogarin commented Jun 27, 2020

Uh oh!

klausi commented Nov 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klausi commented Nov 11, 2020

Uh oh!

klausi commented Nov 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Jun 24, 2019 •

edited

Loading