瀏覽代碼

[api][filebrowser] Dynamically set upload handler in APIView's dispatch method to prevent potential race condition (#4213)

- The previous approach of setting the dynamic upload handler in the initial() method was failing for requests originating from the UI due to a potential race condition. Middleware or DRF's parsers would access request.data prematurely, causing the upload to be processed by default handlers before the custom one could be set.

- This commit refactors the logic into the dispatch() method. As the primary entry point for the view, dispatch() guarantees that the handler is configured before the request body is accessed, resolving the race condition.

Key improvements in this change:

- The handler-setting logic is moved from initial() to dispatch(), which is the correct lifecycle method for this kind of pre-processing.

- Robust exception handling is implemented within dispatch using JsonResponse to gracefully manage errors that occur before DRF's rendering engine is fully initialized.

- A comprehensive suite of pytest unit tests has been added for the dispatch method, covering the success path as well as 400, 404, and 500 error conditions.
Harsh Gupta 3 月之前
父節點
當前提交
c209f6e99f

+ 54 - 24
apps/filebrowser/src/filebrowser/api.py

@@ -24,12 +24,12 @@ from urllib.parse import quote
 
 from django.core.files.uploadhandler import StopUpload
 from django.core.paginator import EmptyPage, Paginator
-from django.http import HttpResponse, HttpResponseNotModified, HttpResponseRedirect, StreamingHttpResponse
+from django.http import HttpResponse, HttpResponseNotModified, HttpResponseRedirect, JsonResponse, StreamingHttpResponse
 from django.utils.http import http_date
 from django.views.static import was_modified_since
 from rest_framework import status
 from rest_framework.decorators import api_view, parser_classes
-from rest_framework.exceptions import NotFound
+from rest_framework.exceptions import APIException, NotFound
 from rest_framework.parsers import JSONParser, MultiPartParser
 from rest_framework.response import Response
 from rest_framework.views import APIView
@@ -40,7 +40,6 @@ from desktop.auth.backend import is_admin
 from desktop.conf import TASK_SERVER_V2
 from desktop.lib import fsmanager, i18n
 from desktop.lib.conf import coerce_bool
-from desktop.lib.django_util import JsonResponse
 from desktop.lib.exceptions_renderable import PopupException
 from desktop.lib.export_csvxls import file_reader
 from desktop.lib.fs.gc.gs import get_gs_home_directory, GSListAllBucketsException
@@ -523,43 +522,74 @@ def upload_complete(request):
 class UploadFileAPI(APIView):
   parser_classes = [MultiPartParser]
 
-  def initial(self, request, *args, **kwargs):
-    """Dynamically select and set the upload handler.
+  def dispatch(self, request, *args, **kwargs):
+    """
+    Overrides dispatch to perform manual authentication and set a dynamic upload handler.
+
+    This is necessary to solve a lifecycle conflict where user authentication
+    (from a JWT token) must happen before DRF's parsing is triggered, but
+    the upload handler must be set before the request body is read.
 
-    This method is called before the upload handler is used.
-    It sets the upload handler for the request.
+    Exception handling within this method is done using Django's standard
+    `JsonResponse` instead of DRF's `Response`. This is a deliberate
+    choice to bypass the DRF rendering lifecycle, which is not fully
+    initialized at this early stage and would otherwise cause errors.
     """
-    LOG.info(f"UploadFileAPI.initial called by user: {request.user.username}")
+    # Manually perform authentication if the user is not already authenticated
+    # by a preceding middleware (like SessionMiddleware).
+    if not request.user.is_authenticated:
+      LOG.debug("User not authenticated, attempting manual authentication in UploadFileAPI.dispatch...")
+      try:
+        # Use the view's configured authenticators to check for credentials (e.g., JWT).
+        for authenticator in self.get_authenticators():
+          user_auth_tuple = authenticator.authenticate(request)
+          if user_auth_tuple is not None:
+            # On successful authentication, attach the user to the request.
+            request.user, request.auth = user_auth_tuple
+            LOG.debug(f"Manual authentication successful for user '{request.user.username}'.")
+            break
+      except APIException as e:
+        # If authentication itself fails (e.g., invalid token), return an error.
+        LOG.warning(f"Manual authentication failed in UploadFileAPI.dispatch: {e.detail}")
+        return JsonResponse(e.detail, status=e.status_code)
+
+    # After authentication, we can now proceed with the main logic.
+    if not request.user.is_authenticated:
+      # If still not authenticated, it's a genuine credentials issue.
+      return JsonResponse({"error": "Authentication credentials were not provided or were invalid."}, status=status.HTTP_401_UNAUTHORIZED)
 
     try:
-      # Validate and parse request parameters
-      serializer = UploadFileSerializer(data=request.query_params)
+      # IMPORTANT: Validate query parameters from request.GET for upload handler configuration.
+      serializer = UploadFileSerializer(data=request.GET)
       serializer.is_valid(raise_exception=True)
+      validated_data = serializer.validated_data
+      username = request.user.username
 
-      destination_path = serializer.validated_data["destination_path"]
-      overwrite = serializer.validated_data["overwrite"]
+      destination_path = validated_data["destination_path"]
+      overwrite = validated_data["overwrite"]
 
-      LOG.debug(f"Upload request - destination: {destination_path}, overwrite: {overwrite}")
+      LOG.debug(f"Dispatching upload for user '{username}' to '{destination_path}' (overwrite: {overwrite}).")
 
-      username = request.user.username
+      # Retrieve user-specific filesystem and the appropriate handler.
       fs = get_user_fs(username)
-
-      LOG.debug(f"Retrieved filesystem for user: {username}")
-
-      # Get the appropriate upload handler
       upload_handler = fs.get_upload_handler(destination_path, overwrite)
+
       if not upload_handler:
-        LOG.error(f"No upload handler found for path: {destination_path}")
+        LOG.error(f"No supported upload handler found for user '{username}' at path: {destination_path}")
         raise NotFound({"error": f"No supported upload handler found for path: {destination_path}"})
 
-      LOG.info(f"Selected upload handler: {upload_handler.__class__.__name__} for destination: {destination_path}")
+      LOG.info(f"Applying upload handler '{upload_handler.__class__.__name__}' for user '{username}'.")
       request.upload_handlers = [upload_handler]
 
-      super().initial(request, *args, **kwargs)
-
+    except APIException as e:
+      LOG.warning(f"API Exception during upload setup in UploadFileAPI.dispatch: {e.detail}")
+      return JsonResponse(e.detail, status=e.status_code)
     except Exception as e:
-      LOG.error(f"Error in UploadFileAPI.initial: {e}")
-      raise
+      LOG.exception(f"An unexpected error occurred while setting the upload handler in UploadFileAPI.dispatch: {e}")
+      return JsonResponse({"error": "A server error occurred during upload initialization."}, status=status.HTTP_500_INTERNAL_SERVER_ERROR)
+
+    # IMPORTANT: After setup, continue to the standard DRF dispatch process.
+    return super().dispatch(request, *args, **kwargs)
 
   def post(self, request, *args, **kwargs):
     """Handles the file upload response after the upload handler has done its work.

+ 132 - 47
apps/filebrowser/src/filebrowser/api_test.py

@@ -19,11 +19,12 @@ import json
 from io import BytesIO as string_io
 from unittest.mock import MagicMock, Mock, patch
 
-import pytest
-from django.http import HttpResponseNotModified, HttpResponseRedirect, StreamingHttpResponse
+from django.contrib.auth.models import AnonymousUser
+from django.http import HttpResponseNotModified, HttpResponseRedirect, JsonResponse, StreamingHttpResponse
 from rest_framework import status
-from rest_framework.exceptions import NotFound
+from rest_framework.exceptions import AuthenticationFailed, ValidationError
 from rest_framework.test import APIRequestFactory
+from rest_framework.views import APIView
 
 from aws.s3.s3fs import S3ListAllBucketsException
 from desktop.lib.exceptions_renderable import PopupException
@@ -1460,50 +1461,6 @@ class TestUploadFileAPI:
     self.request.query_params = {}
     self.request.FILES = {}
 
-  @patch("filebrowser.api.APIView.initial")
-  @patch("filebrowser.api.UploadFileSerializer")
-  @patch("filebrowser.api.get_user_fs")
-  def test_initial_success(self, get_user_fs_mock, upload_file_serializer_mock, api_view_initial_mock):
-    self.request.query_params = {"destination_path": "s3a://test-bucket/test-user/test/dest", "overwrite": False}
-
-    mock_serializer_instance = Mock()
-    mock_serializer_instance.is_valid.return_value = True
-    mock_serializer_instance.validated_data = {"destination_path": "s3a://test-bucket/test-user/test/dest", "overwrite": False}
-    upload_file_serializer_mock.return_value = mock_serializer_instance
-
-    mock_fs = Mock()
-    mock_upload_handler = Mock()
-    mock_fs.get_upload_handler.return_value = mock_upload_handler
-    get_user_fs_mock.return_value = mock_fs
-
-    self.view.initial(self.request)
-
-    upload_file_serializer_mock.assert_called_once_with(data=self.request.query_params)
-    mock_serializer_instance.is_valid.assert_called_once_with(raise_exception=True)
-    get_user_fs_mock.assert_called_once_with("test_user")
-    mock_fs.get_upload_handler.assert_called_once_with("s3a://test-bucket/test-user/test/dest", False)
-    assert self.request.upload_handlers == [mock_upload_handler]
-
-  @patch("filebrowser.api.UploadFileSerializer")
-  @patch("filebrowser.api.get_user_fs")
-  def test_initial_no_upload_handler_found(self, get_user_fs_mock, upload_file_serializer_mock):
-    self.request.query_params = {"destination_path": "s3a://test-bucket/test-user/test/dest", "overwrite": False}
-
-    mock_serializer_instance = Mock()
-    mock_serializer_instance.is_valid.return_value = True
-    mock_serializer_instance.validated_data = {"destination_path": "s3a://test-bucket/test-user/test/dest", "overwrite": False}
-    upload_file_serializer_mock.return_value = mock_serializer_instance
-
-    mock_fs = Mock()
-    mock_fs.get_upload_handler.return_value = None
-    get_user_fs_mock.return_value = mock_fs
-
-    with pytest.raises(NotFound):
-      self.view.initial(self.request)
-
-    get_user_fs_mock.assert_called_once_with("test_user")
-    mock_fs.get_upload_handler.assert_called_once_with("s3a://test-bucket/test-user/test/dest", False)
-
   def test_post_success(self):
     self.request.FILES = {"file": {"path": "s3a://test-bucket/test-user/test/dest/file.txt", "size": 123}}
 
@@ -1538,3 +1495,131 @@ class TestUploadFileAPI:
 
     assert response.status_code == 500
     assert response.data == {"error": "An unexpected error occurred while uploading the file."}
+
+
+class TestUploadFileAPIDispatch:
+
+  def setup_method(self):
+    self.factory = APIRequestFactory()
+    self.view = UploadFileAPI()
+    # A mock authenticated user for session-based tests
+    self.authenticated_user = Mock(username="test_user", is_authenticated=True)
+
+  @patch("filebrowser.api.UploadFileSerializer")
+  @patch("filebrowser.api.get_user_fs")
+  @patch.object(APIView, "dispatch")
+  def test_dispatch_with_token_success(self, mock_super_dispatch, mock_get_user_fs, mock_serializer):
+    request = self.factory.post("/fake-url?destination_path=s3a://path&overwrite=true")
+    request.user = AnonymousUser()
+
+    mock_authenticator = Mock()
+    mock_authenticator.authenticate.return_value = (self.authenticated_user, "mock_token")
+    self.view.get_authenticators = Mock(return_value=[mock_authenticator])
+
+    mock_serializer.return_value.is_valid.return_value = True
+    mock_serializer.return_value.validated_data = {"destination_path": "s3a://path", "overwrite": True}
+    mock_get_user_fs.return_value.get_upload_handler.return_value = Mock(name="S3Handler")
+    mock_super_dispatch.return_value = "SuccessResponse"
+
+    response = self.view.dispatch(request)
+
+    mock_authenticator.authenticate.assert_called_once_with(request)
+    mock_get_user_fs.assert_called_once_with(self.authenticated_user.username)
+    assert request.user == self.authenticated_user
+    assert response == "SuccessResponse"
+
+  def test_dispatch_with_invalid_token(self):
+    request = self.factory.post("/fake-url")
+    request.user = AnonymousUser()
+
+    mock_authenticator = Mock()
+    error_detail = {"detail": "Invalid token."}
+    mock_authenticator.authenticate.side_effect = AuthenticationFailed(error_detail)
+    self.view.get_authenticators = Mock(return_value=[mock_authenticator])
+
+    response = self.view.dispatch(request)
+    response_content = json.loads(response.content)
+
+    assert isinstance(response, JsonResponse)
+    assert response.status_code == 401
+    assert response_content == error_detail
+
+  def test_dispatch_with_no_credentials(self):
+    request = self.factory.post("/fake-url")
+    request.user = AnonymousUser()
+
+    mock_authenticator = Mock()
+    mock_authenticator.authenticate.return_value = None
+    self.view.get_authenticators = Mock(return_value=[mock_authenticator])
+
+    response = self.view.dispatch(request)
+    response_content = json.loads(response.content)
+
+    assert isinstance(response, JsonResponse)
+    assert response.status_code == 401
+    assert "not provided" in response_content["error"]
+
+  @patch("filebrowser.api.UploadFileSerializer")
+  @patch("filebrowser.api.get_user_fs")
+  @patch.object(APIView, "dispatch")
+  def test_dispatch_ui_pre_authenticated_user_success(self, mock_super_dispatch, mock_get_user_fs, mock_serializer):
+    request = self.factory.post("/fake-url?destination_path=s3a://path&overwrite=true")
+    request.user = self.authenticated_user
+
+    mock_serializer.return_value.is_valid.return_value = True
+    mock_serializer.return_value.validated_data = {"destination_path": "s3a://path", "overwrite": True}
+    mock_get_user_fs.return_value.get_upload_handler.return_value = Mock(name="S3Handler")
+    mock_super_dispatch.return_value = "SuccessResponse"
+
+    response = self.view.dispatch(request)
+
+    assert response == "SuccessResponse"
+
+  @patch("filebrowser.api.UploadFileSerializer")
+  def test_dispatch_ui_pre_authenticated_user_validation_error(self, mock_serializer):
+    request = self.factory.post("/fake-url")
+    request.user = self.authenticated_user
+
+    error_detail = {"destination_path": ["This field is required."]}
+    mock_serializer.return_value.is_valid.side_effect = ValidationError(error_detail)
+
+    response = self.view.dispatch(request)
+    response_content = json.loads(response.content)
+
+    assert isinstance(response, JsonResponse)
+    assert response.status_code == 400
+    assert response_content == error_detail
+
+  @patch("filebrowser.api.get_user_fs")
+  @patch("filebrowser.api.UploadFileSerializer")
+  def test_dispatch_ui_pre_authenticated_user_handler_not_found(self, mock_serializer, mock_get_user_fs):
+    request = self.factory.post("/fake-url?destination_path=unsupported://path")
+    request.user = self.authenticated_user
+
+    mock_serializer.return_value.is_valid.return_value = True
+    mock_serializer.return_value.validated_data = {"destination_path": "unsupported://path", "overwrite": False}
+    mock_get_user_fs.return_value.get_upload_handler.return_value = None
+
+    response = self.view.dispatch(request)
+    response_content = json.loads(response.content)
+
+    assert isinstance(response, JsonResponse)
+    assert response.status_code == 404
+    assert "No supported upload handler found" in response_content["error"]
+
+  @patch("filebrowser.api.get_user_fs")
+  @patch("filebrowser.api.UploadFileSerializer")
+  def test_dispatch_ui_pre_authenticated_user_unexpected_exception(self, mock_serializer, mock_get_user_fs):
+    request = self.factory.post("/fake-url?destination_path=s3a://path")
+    request.user = self.authenticated_user
+
+    mock_serializer.return_value.is_valid.return_value = True
+    mock_serializer.return_value.validated_data = {"destination_path": "s3a://path", "overwrite": False}
+    mock_get_user_fs.side_effect = Exception("Something went wrong!")
+
+    response = self.view.dispatch(request)
+    response_content = json.loads(response.content)
+
+    assert isinstance(response, JsonResponse)
+    assert response.status_code == 500
+    assert "server error" in response_content["error"]

+ 5 - 5
desktop/core/src/desktop/lib/fs/gc/upload.py

@@ -232,7 +232,7 @@ class GSNewFileUploadHandler(FileUploadHandler):
     try:
       LOG.debug(f"Initiating GS multipart upload to target path: {self.target_key_path}")
       self.multipart_upload = self._bucket.initiate_multipart_upload(self.target_key_path)
-      LOG.info(f"Multipart upload initiated successfully for: {self.target_key_path}")
+      LOG.info(f"Multipart upload initiated successfully for {self.target_key_path}")
     except Exception as e:
       LOG.error(f"Failed to initiate GS multipart upload for {self.target_key_path}: {e}")
       raise PopupException(f"Failed to initiate GS multipart upload to target path: {self.target_key_path}", error_code=500)
@@ -313,22 +313,22 @@ class GSNewFileUploadHandler(FileUploadHandler):
 
   def upload_chunk(self, raw_chunk):
     try:
-      LOG.debug(f"Uploading part {self.part_number}, size: {len(raw_chunk)} bytes")
+      LOG.debug(f"Uploading part {self.part_number} for {self.target_key_path}, size: {len(raw_chunk)} bytes")
       self.multipart_upload.upload_part_from_file(fp=stream_io(raw_chunk), part_num=self.part_number)
       self.part_number += 1
     except Exception as e:
-      LOG.error(f"Failed to upload part {self.part_number}: {e}")
+      LOG.error(f"Failed to upload part {self.part_number} for {self.target_key_path}: {e}")
       self.multipart_upload.cancel_upload()
       raise PopupException(f"Failed to upload part: {e}", error_code=500)
 
   def file_complete(self, file_size):
     # Finish the upload
-    LOG.info(f"Completing multipart upload - total size: {file_size} bytes, parts: {self.part_number - 1}")
+    LOG.info(f"Completing multipart upload for {self.target_key_path} - total size: {file_size} bytes, parts: {self.part_number - 1}")
     self.multipart_upload.complete_upload()
 
     file_stats = self._fs.stats(f"gs://{self.bucket_name}/{self.target_key_path}")
     file_stats = massage_stats(file_stats)
 
-    LOG.info(f"Upload completed successfully: {self.target_key_path}")
+    LOG.info(f"Upload completed successfully for {self.target_key_path}")
 
     return file_stats

+ 16 - 18
desktop/core/src/desktop/lib/fs/ozone/upload.py

@@ -332,10 +332,10 @@ class OFSNewFileUploadHandler(FileUploadHandler):
         delete=False,  # We'll handle deletion manually for better error handling
       )
       self._temp_file_path = self._temp_file.name
-      LOG.info(f"Created temporary file for OFS upload: {self._temp_file_path}")
+      LOG.info(f"Created temporary file for OFS upload at {self._temp_file_path}")
     except Exception as ex:
       LOG.error(f"Failed to create temporary file for upload: {ex}")
-      raise PopupException("Failed to create temporary upload file: %s" % ex, error_code=500)
+      raise PopupException(f"Failed to create temporary upload file: {ex}", error_code=500)
 
     LOG.debug("OFS upload initialization completed successfully")
 
@@ -398,7 +398,7 @@ class OFSNewFileUploadHandler(FileUploadHandler):
         self._fs.remove(target_file_path, skip_trash=True)
       else:
         LOG.warning(f"File already exists and overwrite is disabled: {target_file_path}")
-        raise PopupException("File already exists: %s" % target_file_path, error_code=409)
+        raise PopupException(f"File already exists: {target_file_path}", error_code=409)
 
     LOG.debug("Upload prerequisites validation completed successfully")
 
@@ -414,7 +414,7 @@ class OFSNewFileUploadHandler(FileUploadHandler):
     if max_size != -1 and max_size >= 0 and self.total_bytes_received > max_size:
       LOG.error(f"File size exceeded limit - received: {self.total_bytes_received}, max: {max_size}")
       self._cleanup_temp_file()
-      raise PopupException("File exceeds maximum allowed size of %d bytes." % max_size, error_code=413)
+      raise PopupException(f"File exceeds maximum allowed size of {max_size} bytes.", error_code=413)
 
     # Write the data chunk to the temporary file
     try:
@@ -424,7 +424,7 @@ class OFSNewFileUploadHandler(FileUploadHandler):
     except Exception as e:
       LOG.exception(f"Error writing to temporary file {self._temp_file_path}")
       self._cleanup_temp_file()
-      raise PopupException("Failed to buffer upload data: %s" % e, error_code=500)
+      raise PopupException(f"Failed to buffer upload data: {e}", error_code=500)
 
     return None
 
@@ -438,12 +438,12 @@ class OFSNewFileUploadHandler(FileUploadHandler):
       LOG.error(f"OFS upload size mismatch - expected: {file_size} bytes, received: {self.total_bytes_received} bytes")
       self._cleanup_temp_file()
       raise PopupException(
-        "Upload data size mismatch: expected %d bytes, received %d bytes." % (file_size, self.total_bytes_received), error_code=422
+        f"Upload data size mismatch: expected {file_size} bytes, received {self.total_bytes_received} bytes.", error_code=422
       )
 
     try:
       # Stream from temp file directly to Ozone
-      LOG.info("Creating file %s with %d bytes from temporary file" % (self.target_file_path, file_size))
+      LOG.info(f"Creating file {self.target_file_path} with {file_size} bytes from temporary file")
 
       # Open temp file for reading and pass the file handle
       # The requests library will stream from the file handle automatically
@@ -462,27 +462,25 @@ class OFSNewFileUploadHandler(FileUploadHandler):
       actual_size = file_stats.size
 
       if actual_size != file_size:
-        LOG.error(
-          "OFS upload size mismatch after write for %s: expected %d bytes, got %d bytes" % (self.target_file_path, file_size, actual_size)
-        )
+        LOG.error(f"OFS upload size mismatch after write for {self.target_file_path}: expected {file_size} bytes, got {actual_size} bytes")
 
         # Clean up the corrupted file
         try:
           self._fs.remove(self.target_file_path, skip_trash=True)
         except Exception as cleanup_error:
-          LOG.warning("Failed to clean up corrupted file %s: %s" % (self.target_file_path, cleanup_error))
+          LOG.warning(f"Failed to clean up corrupted file {self.target_file_path}: {cleanup_error}")
 
         # Raise exception to fail the upload
         raise PopupException(
-          "Upload verification failed: expected %d bytes, but only %d bytes were written. "
-          "The incomplete file has been removed." % (file_size, actual_size),
+          f"Upload verification failed: expected {file_size} bytes, but only {actual_size} bytes were written. "
+          "The incomplete file has been removed.",
           error_code=422,
         )
 
-      LOG.info("OFS upload completed successfully: %d bytes written to %s" % (file_size, self.target_file_path))
+      LOG.info(f"OFS upload completed successfully for {self.target_file_path}, {file_size} bytes written")
 
     except Exception as e:
-      LOG.exception('Error creating file "%s" in OFS' % self.target_file_path)
+      LOG.exception(f'Error creating file "{self.target_file_path}" in OFS')
 
       # Try to clean up if file was partially created
       try:
@@ -494,7 +492,7 @@ class OFSNewFileUploadHandler(FileUploadHandler):
       if isinstance(e, PopupException):
         raise
       else:
-        raise PopupException("Failed to upload file in OFS: %s" % str(e), error_code=500)
+        raise PopupException(f"Failed to upload file in OFS: {str(e)}", error_code=500)
     finally:
       # Always clean up the temporary file
       self._cleanup_temp_file()
@@ -514,6 +512,6 @@ class OFSNewFileUploadHandler(FileUploadHandler):
       try:
         if os.path.exists(self._temp_file_path):
           os.unlink(self._temp_file_path)
-          LOG.debug("Cleaned up temporary file: %s" % self._temp_file_path)
+          LOG.debug(f"Cleaned up temporary file: {self._temp_file_path}")
       except Exception as e:
-        LOG.exception("Failed to clean up temporary file %s: %s" % (self._temp_file_path, e))
+        LOG.exception(f"Failed to clean up temporary file {self._temp_file_path}: {e}")

+ 5 - 5
desktop/libs/aws/src/aws/s3/upload.py

@@ -289,7 +289,7 @@ class S3NewFileUploadHandler(FileUploadHandler):
     try:
       LOG.debug(f"Initiating S3 multipart upload to target path: {self.target_key_path}")
       self.multipart_upload = self._bucket.initiate_multipart_upload(self.target_key_path)
-      LOG.info(f"Multipart upload initiated successfully for: {self.target_key_path}")
+      LOG.info(f"Multipart upload initiated successfully for {self.target_key_path}")
     except Exception as e:
       LOG.error(f"Failed to initiate S3 multipart upload for {self.target_key_path}: {e}")
       raise PopupException(f"Failed to initiate S3 multipart upload to target path: {self.target_key_path}", error_code=500)
@@ -370,11 +370,11 @@ class S3NewFileUploadHandler(FileUploadHandler):
 
   def upload_chunk(self, raw_chunk):
     try:
-      LOG.debug(f"Uploading part {self.part_number}, size: {len(raw_chunk)} bytes")
+      LOG.debug(f"Uploading part {self.part_number} for {self.target_key_path}, size: {len(raw_chunk)} bytes")
       self.multipart_upload.upload_part_from_file(fp=self._get_file_part(raw_chunk), part_num=self.part_number)
       self.part_number += 1
     except Exception as e:
-      LOG.error(f"Failed to upload part {self.part_number}: {e}")
+      LOG.error(f"Failed to upload part {self.part_number} for {self.target_key_path}: {e}")
       self.multipart_upload.cancel_upload()
       raise PopupException(f"Failed to upload part: {e}", error_code=500)
 
@@ -386,12 +386,12 @@ class S3NewFileUploadHandler(FileUploadHandler):
 
   def file_complete(self, file_size):
     # Finish the upload
-    LOG.info(f"Completing multipart upload - total size: {file_size} bytes, parts: {self.part_number - 1}")
+    LOG.info(f"Completing multipart upload for {self.target_key_path} - total size: {file_size} bytes, parts: {self.part_number - 1}")
     self.multipart_upload.complete_upload()
 
     file_stats = self._fs.stats(f"s3a://{self.bucket_name}/{self.target_key_path}")
 
-    LOG.info(f"Upload completed successfully: {self.target_key_path}")
+    LOG.info(f"Upload completed successfully for {self.target_key_path}")
 
     file_stats = massage_stats(file_stats)
 

+ 3 - 3
desktop/libs/azure/src/azure/abfs/upload.py

@@ -376,7 +376,7 @@ class ABFSNewFileUploadHandler(FileUploadHandler):
 
   def upload_chunk(self, raw_chunk, start):
     try:
-      LOG.debug(f"Appending chunk to ABFS file - position: {start}, size: {len(raw_chunk)} bytes")
+      LOG.debug(f"Appending chunk to ABFS file at {self.target_path} - position: {start}, size: {len(raw_chunk)} bytes")
       buffered_data = BytesIO(raw_chunk)
       # TODO: Try encapsulating the _append method in the ABFS class with correct refactoring
       self._fs._append(self.target_path, buffered_data, params={"position": int(start)})
@@ -387,7 +387,7 @@ class ABFSNewFileUploadHandler(FileUploadHandler):
 
   def file_complete(self, file_size):
     # Finish the upload by flushing
-    LOG.info(f"Flushing ABFS file - total size: {file_size} bytes")
+    LOG.info(f"Flushing ABFS file at {self.target_path} - total size: {file_size} bytes")
     self._fs.flush(self.target_path, {"position": int(file_size)})
 
     file_stats = self._fs.stats(self.target_path)
@@ -410,7 +410,7 @@ class ABFSNewFileUploadHandler(FileUploadHandler):
         error_code=422,
       )
 
-    LOG.info(f"ABFS upload completed successfully: {file_size} bytes written to {self.target_path}")
+    LOG.info(f"ABFS upload completed successfully for {self.target_path}, {file_size} bytes written")
 
     file_stats = massage_stats(file_stats)
 

+ 7 - 5
desktop/libs/hadoop/src/hadoop/fs/upload.py

@@ -450,13 +450,13 @@ class HDFSNewFileUploadHandler(FileUploadHandler):
 
     # Create the file directly at the destination
     try:
-      LOG.debug(f"Creating HDFS file at: {self.target_file_path}")
+      LOG.debug(f"Creating HDFS file at {self.target_file_path}")
       self._fs.create(
         self.target_file_path,
         overwrite=False,  # We already handled overwrite above
         permission=self._fs.getDefaultFilePerms(),
       )
-      LOG.info(f"HDFS file created successfully: {self.target_file_path}")
+      LOG.info(f"HDFS file created successfully for {self.target_file_path}")
     except Exception as ex:
       LOG.error(f"Failed to create HDFS file for upload: {ex}")
       raise PopupException(f"Failed to initiate HDFS upload: {ex}", error_code=500)
@@ -535,13 +535,15 @@ class HDFSNewFileUploadHandler(FileUploadHandler):
 
     # Append the chunk directly to the destination file
     try:
-      LOG.debug(f"Appending chunk to HDFS file - size: {len(raw_data)} bytes, total: {self.total_bytes_received} bytes")
+      LOG.debug(
+        f"Appending chunk to HDFS file at {self.target_file_path} - size: {len(raw_data)} bytes, total: {self.total_bytes_received} bytes"
+      )
       self._fs.append(self.target_file_path, raw_data)
       return None
     except Exception as e:
       LOG.exception(f'Error appending data to file "{self.target_file_path}"')
       try:  # Try to clean up the partial file
-        LOG.info(f"Attempting to clean up partial file: {self.target_file_path}")
+        LOG.info(f"Attempting to clean up partial file at {self.target_file_path}")
         self._fs.remove(self.target_file_path, skip_trash=True)
       except Exception:
         pass
@@ -571,7 +573,7 @@ class HDFSNewFileUploadHandler(FileUploadHandler):
         error_code=422,
       )
     else:
-      LOG.info(f"Upload completed successfully: {self.target_file_path}, size: {file_size} bytes")
+      LOG.info(f"Upload completed successfully for {self.target_file_path}, {file_size} bytes written")
 
     file_stats = massage_stats(file_stats)
     return file_stats