Эх сурвалжийг харах

[api] Improve sorting logic in the /list API and add unit tests (#3999)

Harsh Gupta 9 сар өмнө
parent
commit
91bbc6bae9

+ 45 - 19
apps/filebrowser/src/filebrowser/api.py

@@ -250,11 +250,18 @@ def listdir_paged(request):
   A paginated version of listdir.
 
   Query parameters:
-    pagenum           - The page number to show. Defaults to 1.
-    pagesize          - How many to show on a page. Defaults to 30.
-    sortby=?          - Specify attribute to sort by. Accepts: (type, name, atime, mtime, size, user, group). Defaults to name.
-    descending        - Specify a descending sort order. Default to false.
-    filter=?          - Specify a substring filter to search for in the filename field.
+    pagenum (int): The page number to show. Defaults to 1.
+    pagesize (int): How many items to show on a page. Defaults to 30.
+    sortby (str): The attribute to sort by. Valid options: 'type', 'name', 'atime', 'mtime', 'user', 'group', 'size'.
+                  Defaults to 'name'.
+    descending (bool): Sort in descending order when true. Defaults to false.
+    filter (str): Substring to filter filenames. Optional.
+
+  Returns:
+    JsonResponse: Contains 'files' and 'page' info.
+
+  Raises:
+    HttpResponse: With appropriate status codes for errors.
   """
   path = request.GET.get('path', '/')  # Set default path for index directory
   path = _normalize_path(path)
@@ -262,37 +269,56 @@ def listdir_paged(request):
   if not request.fs.isdir(path):
     return HttpResponse(f'{path} is not a directory.', status=400)
 
+  # Extract pagination parameters
   pagenum = int(request.GET.get('pagenum', 1))
   pagesize = int(request.GET.get('pagesize', 30))
 
+  # Determine if operation should be performed as another user
   do_as = None
   if is_admin(request.user) or request.user.has_hue_permission(action="impersonate", app="security"):
     do_as = request.GET.get('doas', request.user.username)
   if hasattr(request, 'doas'):
     do_as = request.doas
 
+  # Get stats for all files in the directory
   try:
     if do_as:
       all_stats = request.fs.do_as_user(do_as, request.fs.listdir_stats, path)
     else:
       all_stats = request.fs.listdir_stats(path)
   except (S3ListAllBucketsException, GSListAllBucketsException) as e:
-    return HttpResponse(f'Bucket listing is not allowed: {str(e)}', status=403)
+    return HttpResponse(f'Bucket listing is not allowed: {e}', status=403)
 
-  # Filter first
+  # Apply filter first if specified
   filter_string = request.GET.get('filter')
   if filter_string:
-    filtered_stats = [sb for sb in all_stats if filter_string in sb['name']]
-    all_stats = filtered_stats
-
-  # Sort next
-  sortby = request.GET.get('sortby')
-  descending_param = request.GET.get('descending')
-  if sortby:
-    if sortby not in ('type', 'name', 'atime', 'mtime', 'user', 'group', 'size'):
-      LOG.info(f'Invalid sort attribute {sortby} for list directory operation. Skipping it.')
-    else:
-      all_stats = sorted(all_stats, key=operator.attrgetter(sortby), reverse=coerce_bool(descending_param))
+    all_stats = [sb for sb in all_stats if filter_string in sb['name']]
+
+  # Next, sort with proper handling of None values
+  sortby = request.GET.get('sortby', 'name')
+  descending = coerce_bool(request.GET.get('descending', False))
+  valid_sort_fields = {'type', 'name', 'atime', 'mtime', 'user', 'group', 'size'}
+
+  if sortby not in valid_sort_fields:
+    LOG.info(f"Ignoring invalid sort attribute '{sortby}' for list directory operation.")
+  else:
+    numeric_fields = {'size', 'atime', 'mtime'}
+
+    def sorting_key(item):
+      """Generate a sorting key that handles None values for different field types."""
+      value = getattr(item, sortby)
+      if sortby in numeric_fields:
+        # Treat None as 0 for numeric fields for comparison
+        return 0 if value is None else value
+      else:
+        # Treat None as an empty string for non-numeric fields
+        return '' if value is None else value
+
+    try:
+      all_stats = sorted(all_stats, key=sorting_key, reverse=descending)
+    except Exception as sort_error:
+      LOG.error(f"Error during sorting with attribute '{sortby}': {sort_error}")
+      return HttpResponse("An error occurred while sorting the directory contents.", status=500)
 
   # Do pagination
   try:
@@ -302,7 +328,7 @@ def listdir_paged(request):
   except EmptyPage:
     message = "No results found for the requested page."
     LOG.warning(message)
-    return HttpResponse(message, status=404)  # TODO: status code?
+    return HttpResponse(message, status=404)
 
   if page:
     page.object_list = [_massage_stats(request, stat_absolute_path(path, s)) for s in shown_stats]

+ 205 - 2
apps/filebrowser/src/filebrowser/api_test.py

@@ -16,11 +16,12 @@
 # limitations under the License.
 
 import json
-from unittest.mock import Mock, patch
+from unittest.mock import MagicMock, Mock, patch
 
 from django.core.files.uploadedfile import SimpleUploadedFile
 
-from filebrowser.api import copy, get_all_filesystems, mkdir, move, rename, upload_file
+from aws.s3.s3fs import S3ListAllBucketsException
+from filebrowser.api import copy, get_all_filesystems, listdir_paged, mkdir, move, rename, upload_file
 from filebrowser.conf import (
   MAX_FILE_SIZE_UPLOAD_LIMIT,
   RESTRICT_FILE_EXTENSIONS,
@@ -945,3 +946,205 @@ class TestCopyAPI:
 
     assert response.status_code == 409
     assert response.content.decode('utf-8') == 'File or folder already exists at destination path.'
+
+
+class TestListAPI:
+  def _create_mock_file_stats(self, name, path, size, user, group):
+    mock_stats = MagicMock()
+    mock_stats.path = path
+    mock_stats.name = name
+    mock_stats.size = size
+    mock_stats.atime = 0
+    mock_stats.mtime = 0
+    mock_stats.type = 'file'
+    mock_stats.user = user
+    mock_stats.group = group
+    mock_stats.mode = 33188
+    mock_stats.to_json_dict = Mock(
+      return_value={
+        'path': path,
+        'aclBit': False,
+        'size': size,
+        'atime': 0,
+        'mtime': 0,
+        'type': 'file',
+        'user': user,
+        'group': group,
+        'mode': 33188,
+      }
+    )
+    return mock_stats
+
+  def _create_mock_paginator(self, all_stats):
+    mock_page = MagicMock()
+    mock_page.object_list = all_stats
+    mock_page.has_next.return_value = False
+    mock_page.has_previous.return_value = False
+    mock_page.number = 1
+
+    mock_paginator = MagicMock()
+    mock_paginator.page.return_value = mock_page
+    mock_paginator.per_page = 30
+    mock_paginator.count = 2
+    mock_paginator.num_pages = 1
+
+    return mock_paginator
+
+  def test_listdir_paged_success(self):
+    with patch('filebrowser.api.is_admin') as is_admin:
+      is_admin.return_value = False
+
+      file1_stats = self._create_mock_file_stats(
+        name='file1.txt', path='s3a://test-bucket/test-user/test-dir/file1.txt', size=100, user='user1', group='group1'
+      )
+      file2_stats = self._create_mock_file_stats(
+        name='file2.txt', path='s3a://test-bucket/test-user/test-dir/file2.txt', size=200, user='user2', group='group2'
+      )
+
+      all_stats = [file2_stats, file1_stats]
+
+      request = Mock(
+        method='GET',
+        GET={'pagenum': '1', 'pagesize': '30', 'path': 's3a://test-bucket/test-user/test-dir', 'sortby': 'name', 'descending': 'False'},
+        user=Mock(has_hue_permission=Mock(return_value=False)),
+        fs=Mock(
+          listdir_stats=Mock(return_value=all_stats),
+          do_as_user=Mock(return_value=all_stats),
+          isdir=Mock(return_value=True),
+          normpath=Mock(side_effect=['s3a://test-bucket/test-user/test-dir/file1.txt', 's3a://test-bucket/test-user/test-dir/file2.txt']),
+        ),
+      )
+
+      mock_paginator = self._create_mock_paginator(all_stats)
+
+      with patch('filebrowser.api.Paginator', return_value=mock_paginator) as mock_paginator:
+        response = listdir_paged(request)
+        response_data = json.loads(response.content)
+
+        assert response.status_code == 200
+        assert response_data == {
+          'files': [
+            {
+              'path': 's3a://test-bucket/test-user/test-dir/file1.txt',
+              'aclBit': False,
+              'size': 200,
+              'atime': 0,
+              'mtime': 0,
+              'type': 'file',
+              'user': 'user2',
+              'group': 'group2',
+              'mode': 33188,
+              'rwx': '-rw-r--r--+',
+            },
+            {
+              'path': 's3a://test-bucket/test-user/test-dir/file2.txt',
+              'aclBit': False,
+              'size': 100,
+              'atime': 0,
+              'mtime': 0,
+              'type': 'file',
+              'user': 'user1',
+              'group': 'group1',
+              'mode': 33188,
+              'rwx': '-rw-r--r--+',
+            },
+          ],
+          'page': {'page_number': 1, 'page_size': 30, 'total_pages': 1, 'total_size': 2},
+        }
+
+        # Assert correct sorting
+        assert response_data['files'][0]['path'] == 's3a://test-bucket/test-user/test-dir/file1.txt'
+        assert response_data['files'][1]['path'] == 's3a://test-bucket/test-user/test-dir/file2.txt'
+
+  def test_listdir_paged_sorting_by_size_descending(self):
+    with patch('filebrowser.api.is_admin') as is_admin:
+      is_admin.return_value = False
+
+      file1_stats = self._create_mock_file_stats(
+        name='file1.txt', path='s3a://test-bucket/test-user/test-dir/file1.txt', size=100, user='user1', group='group1'
+      )
+      file2_stats = self._create_mock_file_stats(
+        name='file2.txt', path='s3a://test-bucket/test-user/test-dir/file2.txt', size=200, user='user2', group='group2'
+      )
+      file3_stats = self._create_mock_file_stats(
+        name='file3.txt', path='s3a://test-bucket/test-user/test-dir/file3.txt', size=300, user='user3', group='group3'
+      )
+      file4_stats = self._create_mock_file_stats(
+        name='file4.txt', path='s3a://test-bucket/test-user/test-dir/file4.txt', size=400, user='user4', group='group4'
+      )
+
+      all_stats = [file1_stats, file2_stats, file3_stats, file4_stats]
+
+      request = Mock(
+        method='GET',
+        GET={
+          'pagenum': '1',
+          'pagesize': '30',
+          'path': 's3a://test-bucket/test-user/test-dir',
+          'sortby': 'name',
+          'descending': 'True',
+        },
+        user=Mock(has_hue_permission=Mock(return_value=False)),
+        fs=Mock(
+          listdir_stats=Mock(side_effect=[all_stats, all_stats]),
+          do_as_user=Mock(return_value=all_stats),
+          isdir=Mock(return_value=True),
+          normpath=Mock(
+            side_effect=[
+              's3a://test-bucket/test-user/test-dir/file1.txt',
+              's3a://test-bucket/test-user/test-dir/file2.txt',
+              's3a://test-bucket/test-user/test-dir/file3.txt',
+              's3a://test-bucket/test-user/test-dir/file4.txt',
+            ]
+          ),
+        ),
+      )
+
+      mock_paginator = self._create_mock_paginator(sorted(all_stats, key=lambda x: x.size, reverse=True))
+
+      with patch('filebrowser.api.Paginator', return_value=mock_paginator) as mock_paginator:
+        response = listdir_paged(request)
+        response_data = json.loads(response.content)
+
+        assert response.status_code == 200
+        assert 'files' in response_data
+        assert len(response_data['files']) == 4
+
+        # Assert correct sorting
+        assert response_data['files'][0]['size'] == 400
+        assert response_data['files'][1]['size'] == 300
+        assert response_data['files'][2]['size'] == 200
+        assert response_data['files'][3]['size'] == 100
+
+  def test_listdir_paged_invalid_path(self):
+    request = Mock(
+      method='GET',
+      GET={'pagenum': '1', 'pagesize': '30', 'path': 's3a://test-bucket/test-user/test-dir/test-file'},
+      fs=Mock(
+        isdir=Mock(return_value=False),
+      ),
+    )
+
+    response = listdir_paged(request)
+
+    assert response.status_code == 400
+    assert response.content.decode('utf-8') == 's3a://test-bucket/test-user/test-dir/test-file is not a directory.'
+
+  def test_listdir_paged_error_while_listing_bucket(self):
+    with patch('filebrowser.api.is_admin') as is_admin:
+      is_admin.return_value = False
+
+      request = Mock(
+        method='GET',
+        GET={'pagenum': '1', 'pagesize': '30', 'path': 's3a://test-bucket/'},
+        fs=Mock(
+          isdir=Mock(return_value=True),
+          listdir_stats=Mock(side_effect=S3ListAllBucketsException('Failed to list all buckets')),
+          do_as_user=Mock(side_effect=S3ListAllBucketsException('Failed to list all buckets')),
+        ),
+      )
+
+      response = listdir_paged(request)
+
+      assert response.status_code == 403
+      assert response.content.decode('utf-8') == 'Bucket listing is not allowed: Failed to list all buckets'