-
Notifications
You must be signed in to change notification settings - Fork 371
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[api] Improve normal file upload design for public APIs (#3878)
- The original file upload implementation is super old and directly uploads the file by checking the available upload handlers set in `settings.py` when the request reaches the first middleware. - This flow did not follow the current Hue filebrowser design of routing the FS calls via ProxyFS. - The design also had historical flaws of uploading the file even if request was meant for other file operation (if you send a file with /copy call, it will still upload the file and then try doing the copy operation). - There was no flexibility for upload pre-checks and error handling. Most of such stuff was shifted post file upload. - The new implementation helps in solving all the above problem along with cleaner code, improved design, upload on-demand, greater control and performance improvements. - The new implementation is only available in API form and does not affect or modifies the older implementation (both are available in parallel for the time being) because we are in the process of phasing out the old filebrowser. - Once the new filebrowser is upto speed, the old implementation will be removed.
- Loading branch information
Showing
19 changed files
with
785 additions
and
182 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,234 @@ | ||
| #!/usr/bin/env python | ||
| # Licensed to Cloudera, Inc. under one | ||
| # or more contributor license agreements. See the NOTICE file | ||
| # distributed with this work for additional information | ||
| # regarding copyright ownership. Cloudera, Inc. licenses this file | ||
| # to you under the Apache License, Version 2.0 (the | ||
| # "License"); you may not use this file except in compliance | ||
| # with the License. You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import json | ||
| from unittest.mock import Mock, patch | ||
|
|
||
| from django.core.files.uploadedfile import SimpleUploadedFile | ||
|
|
||
| from filebrowser.api import upload_file | ||
| from filebrowser.conf import ( | ||
| MAX_FILE_SIZE_UPLOAD_LIMIT, | ||
| RESTRICT_FILE_EXTENSIONS, | ||
| ) | ||
|
|
||
|
|
||
| class TestNormalFileUpload: | ||
| def test_file_upload_success(self): | ||
| with patch('filebrowser.api.string_io') as string_io: | ||
| with patch('filebrowser.api.stat_absolute_path') as stat_absolute_path: | ||
| with patch('filebrowser.api._massage_stats') as _massage_stats: | ||
| request = Mock( | ||
| method='POST', | ||
| META=Mock(), | ||
| POST={'destination_path': 's3a://test-bucket/test-user/'}, | ||
| FILES={'file': SimpleUploadedFile('test_file.txt', b'Hello World!')}, | ||
| body=Mock(), | ||
| fs=Mock( | ||
| join=Mock(), | ||
| exists=Mock(side_effect=[False, True]), | ||
| isdir=Mock(return_value=False), | ||
| upload_v1=Mock(return_value=None), | ||
| stats=Mock(), | ||
| ), | ||
| ) | ||
|
|
||
| _massage_stats.return_value = { | ||
| "path": "s3a://test-bucket/test-user/test_file.txt", | ||
| "size": 12, | ||
| "atime": 1731527617, | ||
| "mtime": 1731527620, | ||
| "mode": 33188, | ||
| "user": "test-user", | ||
| "group": "test-user", | ||
| "blockSize": 134217728, | ||
| "replication": 3, | ||
| "type": "file", | ||
| "rwx": "-rw-r--r--", | ||
| } | ||
|
|
||
| resets = [ | ||
| RESTRICT_FILE_EXTENSIONS.set_for_testing(''), | ||
| MAX_FILE_SIZE_UPLOAD_LIMIT.set_for_testing(-1), | ||
| ] | ||
| try: | ||
| response = upload_file(request) | ||
| print(response.content) | ||
| response_data = json.loads(response.content) | ||
|
|
||
| assert response.status_code == 200 | ||
| assert response_data['uploaded_file_stats'] == { | ||
| "path": "s3a://test-bucket/test-user/test_file.txt", | ||
| "size": 12, | ||
| "atime": 1731527617, | ||
| "mtime": 1731527620, | ||
| "mode": 33188, | ||
| "user": "test-user", | ||
| "group": "test-user", | ||
| "blockSize": 134217728, | ||
| "replication": 3, | ||
| "type": "file", | ||
| "rwx": "-rw-r--r--", | ||
| } | ||
| finally: | ||
| for reset in resets: | ||
| reset() | ||
|
|
||
| def test_upload_invalid_file_type(self): | ||
| with patch('filebrowser.api.string_io') as string_io: | ||
| request = Mock( | ||
| method='POST', | ||
| META=Mock(), | ||
| POST={'destination_path': 's3a://test-bucket/test-user/'}, | ||
| FILES={'file': SimpleUploadedFile('test_file.txt', b'Hello World!')}, | ||
| body=Mock(), | ||
| fs=Mock( | ||
| join=Mock(), | ||
| exists=Mock(side_effect=[False, True]), | ||
| isdir=Mock(return_value=False), | ||
| upload_v1=Mock(return_value=None), | ||
| stats=Mock(), | ||
| ), | ||
| ) | ||
| resets = [ | ||
| RESTRICT_FILE_EXTENSIONS.set_for_testing('.exe,.txt'), | ||
| MAX_FILE_SIZE_UPLOAD_LIMIT.set_for_testing(-1), | ||
| ] | ||
| try: | ||
| response = upload_file(request) | ||
|
|
||
| assert response.status_code == 400 | ||
| assert response.content.decode('utf-8') == 'File type ".txt" is not allowed. Please choose a file with a different type.' | ||
| finally: | ||
| for reset in resets: | ||
| reset() | ||
|
|
||
| def test_upload_file_exceeds_max_size(self): | ||
| with patch('filebrowser.api.string_io') as string_io: | ||
| request = Mock( | ||
| method='POST', | ||
| META=Mock(), | ||
| POST={'destination_path': 's3a://test-bucket/test-user/'}, | ||
| FILES={'file': SimpleUploadedFile('test_file.txt', b'Hello World!')}, | ||
| body=Mock(), | ||
| fs=Mock( | ||
| join=Mock(), | ||
| exists=Mock(side_effect=[False, True]), | ||
| isdir=Mock(return_value=False), | ||
| upload_v1=Mock(return_value=None), | ||
| stats=Mock(), | ||
| ), | ||
| ) | ||
| resets = [ | ||
| RESTRICT_FILE_EXTENSIONS.set_for_testing(''), | ||
| MAX_FILE_SIZE_UPLOAD_LIMIT.set_for_testing(5), | ||
| ] | ||
| try: | ||
| response = upload_file(request) | ||
|
|
||
| assert response.status_code == 413 | ||
| assert response.content.decode('utf-8') == 'File exceeds maximum allowed size of 5 bytes. Please upload a smaller file.' | ||
| finally: | ||
| for reset in resets: | ||
| reset() | ||
|
|
||
| def test_upload_file_already_exists(self): | ||
| with patch('filebrowser.api.string_io') as string_io: | ||
| request = Mock( | ||
| method='POST', | ||
| META=Mock(), | ||
| POST={'destination_path': 's3a://test-bucket/test-user/'}, | ||
| FILES={'file': SimpleUploadedFile('test_file.txt', b'Hello World!')}, | ||
| body=Mock(), | ||
| fs=Mock( | ||
| join=Mock(return_value='s3a://test-bucket/test-user/test_file.txt'), | ||
| exists=Mock(return_value=True), | ||
| isdir=Mock(return_value=True), | ||
| upload_v1=Mock(return_value=None), | ||
| stats=Mock(), | ||
| ), | ||
| ) | ||
| resets = [ | ||
| RESTRICT_FILE_EXTENSIONS.set_for_testing(''), | ||
| MAX_FILE_SIZE_UPLOAD_LIMIT.set_for_testing(-1), | ||
| ] | ||
| try: | ||
| response = upload_file(request) | ||
|
|
||
| assert response.status_code == 409 | ||
| assert response.content.decode('utf-8') == 'The file path s3a://test-bucket/test-user/test_file.txt already exists.' | ||
| finally: | ||
| for reset in resets: | ||
| reset() | ||
|
|
||
| def test_destination_path_does_not_exists(self): | ||
| with patch('filebrowser.api.string_io') as string_io: | ||
| request = Mock( | ||
| method='POST', | ||
| META=Mock(), | ||
| POST={'destination_path': 's3a://test-bucket/test-user/'}, | ||
| FILES={'file': SimpleUploadedFile('test_file.txt', b'Hello World!')}, | ||
| body=Mock(), | ||
| fs=Mock( | ||
| join=Mock(), | ||
| exists=Mock(return_value=False), | ||
| isdir=Mock(return_value=True), | ||
| upload_v1=Mock(return_value=None), | ||
| stats=Mock(), | ||
| ), | ||
| ) | ||
| resets = [ | ||
| RESTRICT_FILE_EXTENSIONS.set_for_testing(''), | ||
| MAX_FILE_SIZE_UPLOAD_LIMIT.set_for_testing(-1), | ||
| ] | ||
| try: | ||
| response = upload_file(request) | ||
|
|
||
| assert response.status_code == 404 | ||
| assert response.content.decode('utf-8') == 'The destination path s3a://test-bucket/test-user/ does not exist.' | ||
| finally: | ||
| for reset in resets: | ||
| reset() | ||
|
|
||
| def test_file_upload_failure(self): | ||
| with patch('filebrowser.api.string_io') as string_io: | ||
| request = Mock( | ||
| method='POST', | ||
| META=Mock(), | ||
| POST={'destination_path': 's3a://test-bucket/test-user/'}, | ||
| FILES={'file': SimpleUploadedFile('test_file.txt', b'Hello World!')}, | ||
| body=Mock(), | ||
| fs=Mock( | ||
| join=Mock(return_value='s3a://test-bucket/test-user/test_file.txt'), | ||
| exists=Mock(side_effect=[False, True]), | ||
| isdir=Mock(return_value=True), | ||
| upload_v1=Mock(side_effect=Exception('Upload exception occured!')), | ||
| stats=Mock(), | ||
| ), | ||
| ) | ||
| resets = [ | ||
| RESTRICT_FILE_EXTENSIONS.set_for_testing(''), | ||
| MAX_FILE_SIZE_UPLOAD_LIMIT.set_for_testing(-1), | ||
| ] | ||
| try: | ||
| response = upload_file(request) | ||
|
|
||
| assert response.status_code == 500 | ||
| assert response.content.decode('utf-8') == 'Upload to s3a://test-bucket/test-user/test_file.txt failed: Upload exception occured!' | ||
| finally: | ||
| for reset in resets: | ||
| reset() |
Oops, something went wrong.