Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save user sent metadata #166

Merged
merged 11 commits into from
Aug 10, 2019
Merged

Save user sent metadata #166

merged 11 commits into from
Aug 10, 2019

Conversation

Steveb-p
Copy link
Contributor

Fixes #163.

I needed this functionality more or less, so I've given a crack at it.

Screenshot below is from Symfony dump for file upload completion event, which previously did not contain any user sent metadata.

2019-07-16_19-20

There are no tests yet added, wanted to have your opinion on this approach first.

Copy link
Contributor

@alucic alucic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent some time thinking about this problem and whatever way I took I always ended up breaking backward compatibility. Because of that, all of my suggestions are around visibility and limiting how people can use this package.

You have found a nice way to make sure that there are no breaking changes and I'd like to see this merged.

src/File.php Outdated Show resolved Hide resolved
src/File.php Outdated Show resolved Hide resolved
@Steveb-p
Copy link
Contributor Author

There are a couple tests that keep failing, so I'll have to take a look at them.

@Steveb-p
Copy link
Contributor Author

Steveb-p commented Jul 29, 2019

@alucic I've taken a look at failing tests and most were actually issues with mocks or expected cache changes (additional fields added). Overall I've got a feeling that some tests are redundant and check internal behavior, but that's outside the scope of this PR and it's only my personal opinion.

I've taken the liberty to:

  1. Switch from a specific count of calls to ->atLeast()->once() when getRequest was supposed to be called in tests related to this PR.
  2. Add file paths to exception message when File fails to copy.

@ariselseng
Copy link

I just tried this. It is working fine here. Looking forward to the merge.

Copy link
Owner

@ankitpokhrel ankitpokhrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Steveb-p thank you for working on this improvement. I want to suggest few changes.

Instead of modifying buildFile method to set meta info and using array_merge to merge meta during function call, It would be more clear and readable to directly call setUploadMetadata. Consider the diff below, we can do minimal change in Server.php to achieve what we want.

diff --git a/src/File.php b/src/File.php
index 7f20840..4a45769 100644
--- a/src/File.php
+++ b/src/File.php
@@ -46,6 +46,9 @@ class File
     /** @var int */
     protected $fileSize;

+    /** @var string[] */
+    private $uploadMetadata = [];
+
     /**
      * File constructor.
      *
@@ -246,6 +249,18 @@ class File
         return $this->filePath;
     }

+    /**
+     * @param string[] $metadata
+     *
+     * @return File
+     */
+    public function setUploadMetadata(array $metadata) : self
+    {
+        $this->uploadMetadata = $metadata;
+
+        return $this;
+    }
+
     /**
      * Get input stream.
      *
@@ -272,6 +287,7 @@ class File
             'checksum' => $this->checksum,
             'location' => $this->location,
             'file_path' => $this->filePath,
+            'metadata' => $this->uploadMetadata,
             'created_at' => $now->format($this->cache::RFC_7231),
             'expires_at' => $now->addSeconds($this->cache->getTtl())->format($this->cache::RFC_7231),
         ];
diff --git a/src/Request.php b/src/Request.php
index 9d4ce5f..f75dd64 100644
--- a/src/Request.php
+++ b/src/Request.php
@@ -155,6 +155,31 @@ class Request
         return '';
     }

+    /**
+     * Get all upload meta data from the request header.
+     *
+     * @return string[]
+     */
+    public function getAllMeta() : array
+    {
+        $uploadMetaData = $this->request->headers->get('Upload-Metadata');
+
+        if (empty($uploadMetaData)) {
+            return [];
+        }
+
+        $uploadMetaDataChunks = explode(',', $uploadMetaData);
+
+        $result = [];
+        foreach ($uploadMetaDataChunks as $chunk) {
+            list($key, $value) = explode(' ', $chunk);
+
+            $result[$key] = base64_decode($value);
+        }
+
+        return $result;
+    }
+
     /**
      * Extract partials from header.
      *
diff --git a/src/Tus/Server.php b/src/Tus/Server.php
index c6391e0..c720d4a 100644
--- a/src/Tus/Server.php
+++ b/src/Tus/Server.php
@@ -373,7 +373,7 @@ class Server extends AbstractTus
             'size' => $this->getRequest()->header('Upload-Length'),
             'file_path' => $filePath,
             'location' => $location,
-        ])->setKey($uploadKey)->setChecksum($checksum);
+        ])->setKey($uploadKey)->setChecksum($checksum)->setUploadMetadata($this->getRequest()->getAllMeta());

         $this->cache->set($uploadKey, $file->details() + ['upload_type' => $uploadType]);

@@ -414,7 +414,7 @@ class Server extends AbstractTus
             'size' => 0,
             'file_path' => $filePath,
             'location' => $location,
-        ])->setFilePath($filePath)->setKey($uploadKey);
+        ])->setFilePath($filePath)->setKey($uploadKey)->setUploadMetadata($this->getRequest()->getAllMeta());

         $file->setOffset($file->merge($files));

@@ -466,7 +466,7 @@ class Server extends AbstractTus
             return $this->response->send(null, $status);
         }

-        $file     = $this->buildFile($meta);
+        $file     = $this->buildFile($meta)->setUploadMetadata($meta['metadata']);
         $checksum = $meta['checksum'];

         try {

src/File.php Outdated Show resolved Hide resolved
Copy link
Owner

@ankitpokhrel ankitpokhrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ankitpokhrel ankitpokhrel merged commit 3d250bb into ankitpokhrel:master Aug 10, 2019
@Steveb-p Steveb-p deleted the patch-1 branch August 10, 2019 14:19
@ankitpokhrel ankitpokhrel added this to the Unreleased milestone Sep 21, 2019
@Amerlander Amerlander mentioned this pull request Feb 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Save user sent meta data
4 participants