From a398de73516955b614b4b36f958f6a62235108fd Mon Sep 17 00:00:00 2001 From: Przemyslaw Kukulski Date: Thu, 10 Sep 2026 09:32:53 +0200 Subject: [PATCH] fix(database): use set-based ordering for group-by fields in adhoc sorting path (#5938) --- .../baserow/contrib/database/api/constants.py | 11 + .../database/api/export/serializers.py | 7 + .../contrib/database/api/export/views.py | 20 +- .../contrib/database/api/rows/views.py | 2 +- .../contrib/database/api/views/grid/utils.py | 46 +-- .../contrib/database/api/views/grid/views.py | 72 +++- .../contrib/database/api/views/utils.py | 31 +- .../contrib/database/export/file_writer.py | 8 +- .../contrib/database/export/handler.py | 32 +- .../table_exporters/csv_table_exporter.py | 1 + .../contrib/database/fields/field_sortings.py | 86 ++++- .../contrib/database/fields/field_types.py | 149 ++++++-- .../baserow/contrib/database/table/models.py | 159 +++++--- .../baserow/contrib/database/views/handler.py | 79 ++-- .../baserow/contrib/database/views/models.py | 2 +- .../contrib/database/api/export/__init__.py | 0 .../export/test_export_group_by_contract.py | 150 ++++++++ .../database/api/export/test_export_views.py | 51 +++ .../grid/test_grid_view_group_by_contract.py | 242 +++++++++++++ .../grid/test_grid_view_group_by_data.py | 64 ++++ .../test_grid_view_ordering_regressions.py | 341 ++++++++++++++++++ .../api/views/grid/test_grid_view_views.py | 198 +++++++++- .../field/test_created_by_field_type.py | 4 +- .../database/field/test_formula_field_type.py | 34 +- .../field/test_last_modified_by_field_type.py | 4 +- .../database/field/test_lookup_field_type.py | 4 +- .../test_multiple_collaborators_field_type.py | 4 +- .../field/test_multiple_select_field_type.py | 8 +- .../field/test_single_select_field_type.py | 6 +- .../database/table/test_table_models.py | 135 +++++++ .../trash/test_database_trash_types.py | 4 +- .../database/view/test_view_handler.py | 60 +-- ...ring_in_grouped_grid_views_for_editor.json | 9 + .../data_sync/baserow_table_data_sync.py | 2 +- .../src/baserow_premium/api/views/views.py | 6 +- .../baserow_premium/export/exporter_types.py | 6 +- .../src/baserow_premium/views/handler.py | 2 +- .../test_public_export_group_by_contract.py | 46 +++ .../views/PublicViewExportMenuItem.vue | 6 + .../view/publicViewExportMenuItem.spec.js | 151 ++++++++ .../modules/database/services/view/grid.js | 8 +- web-frontend/modules/database/utils/view.js | 38 +- .../services/view/gridOrdering.spec.js | 182 ++++++++++ .../test/unit/database/utils/view.spec.js | 60 +++ 44 files changed, 2270 insertions(+), 260 deletions(-) create mode 100644 backend/tests/baserow/contrib/database/api/export/__init__.py create mode 100644 backend/tests/baserow/contrib/database/api/export/test_export_group_by_contract.py create mode 100644 backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_group_by_contract.py create mode 100644 backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_ordering_regressions.py create mode 100644 changelog/entries/unreleased/bug/5937_fixed_blank_rows_appearing_in_grouped_grid_views_for_editor.json create mode 100644 premium/backend/tests/baserow_premium_tests/api/views/views/test_public_export_group_by_contract.py create mode 100644 premium/web-frontend/test/unit/premium/view/publicViewExportMenuItem.spec.js create mode 100644 web-frontend/test/unit/database/services/view/gridOrdering.spec.js diff --git a/backend/src/baserow/contrib/database/api/constants.py b/backend/src/baserow/contrib/database/api/constants.py index b20f0781fe..9c048c0262 100644 --- a/backend/src/baserow/contrib/database/api/constants.py +++ b/backend/src/baserow/contrib/database/api/constants.py @@ -217,6 +217,17 @@ def make_adhoc_filter_api_params(combine_filters=True, view_is_aggregating=False "descending (Z-A).", ) +ADHOC_GROUP_BY_API_PARAM = OpenApiParameter( + name="group_by", + location=OpenApiParameter.QUERY, + type=OpenApiTypes.STR, + description="Optionally the rows can be grouped by the provided field ids " + "separated by comma. By default a field is grouped in ascending (A-Z) " + "order, but by prepending the field with a '-' it can be grouped " + "descending (Z-A). Fields listed here use group-by ordering instead of " + "regular sort ordering.", +) + PAGINATION_API_PARAMS = ( OpenApiParameter( name="limit", diff --git a/backend/src/baserow/contrib/database/api/export/serializers.py b/backend/src/baserow/contrib/database/api/export/serializers.py index 8bd4997db9..5ff95060b8 100644 --- a/backend/src/baserow/contrib/database/api/export/serializers.py +++ b/backend/src/baserow/contrib/database/api/export/serializers.py @@ -140,6 +140,13 @@ class BaseExporterOptionsSerializer(serializers.Serializer): "by comma. By default a field is ordered in ascending (A-Z) order, but by " "prepending the field with a '-' it can be ordered descending (Z-A).", ) + group_by = serializers.CharField( + required=False, + allow_null=True, + allow_blank=True, + help_text="Optionally the rows can be grouped by provided field ids separated " + "by comma. Group-by fields are ordered before sort fields.", + ) fields = serializers.ListField( required=False, allow_null=True, diff --git a/backend/src/baserow/contrib/database/api/export/views.py b/backend/src/baserow/contrib/database/api/export/views.py index 44c9b9eae1..4a1515c404 100644 --- a/backend/src/baserow/contrib/database/api/export/views.py +++ b/backend/src/baserow/contrib/database/api/export/views.py @@ -20,6 +20,7 @@ ) from baserow.contrib.database.api.export.serializers import ( BaseExporterOptionsSerializer, + DisplayChoiceField, ExportJobSerializer, ) from baserow.contrib.database.api.fields.errors import ( @@ -32,6 +33,7 @@ ERROR_VIEW_DOES_NOT_EXIST, ERROR_VIEW_FILTER_TYPE_DOES_NOT_EXIST, ERROR_VIEW_FILTER_TYPE_UNSUPPORTED_FIELD, + ERROR_VIEW_GROUP_BY_FIELD_NOT_SUPPORTED, ERROR_VIEW_NOT_IN_TABLE, ) from baserow.contrib.database.export.exceptions import ( @@ -52,6 +54,7 @@ ViewDoesNotExist, ViewFilterTypeDoesNotExist, ViewFilterTypeNotAllowedForField, + ViewGroupByFieldNotSupported, ViewNotInTable, ) from baserow.contrib.database.views.handler import ViewHandler @@ -73,14 +76,26 @@ def _validate_options(data: Dict[str, Any]) -> Dict[str, Any]: options serializer based on the exporter_type and finally validates the data using that serializer. + Uses ``return_validated=True`` so that omitted optional fields (e.g. + ``group_by``) stay absent instead of appearing as ``None``. Because + ``validated_data`` bypasses ``to_representation()``, we manually apply the + conversion for every ``DisplayChoiceField`` (delimiter, charset) so the + downstream code receives the actual Python values, not the display names. + :param data: A dict of data to serialize using an exporter options serializer. :return: validated export options data """ option_serializers = table_exporter_registry.get_option_serializer_map() validated_exporter_type = validate_data(BaseExporterOptionsSerializer, data) - serializer = option_serializers[validated_exporter_type["exporter_type"]] - return validate_data(serializer, data) + serializer_class = option_serializers[validated_exporter_type["exporter_type"]] + validated = validate_data(serializer_class, data, return_validated=True) + + for field_name, field in serializer_class().fields.items(): + if isinstance(field, DisplayChoiceField) and field_name in validated: + validated[field_name] = field.to_representation(validated[field_name]) + + return validated class ExportTableView(APIView): @@ -137,6 +152,7 @@ class ExportTableView(APIView): ViewFilterTypeNotAllowedForField: ERROR_VIEW_FILTER_TYPE_UNSUPPORTED_FIELD, OrderByFieldNotFound: ERROR_ORDER_BY_FIELD_NOT_FOUND, OrderByFieldNotPossible: ERROR_ORDER_BY_FIELD_NOT_POSSIBLE, + ViewGroupByFieldNotSupported: ERROR_VIEW_GROUP_BY_FIELD_NOT_SUPPORTED, } ) def post(self, request, table_id): diff --git a/backend/src/baserow/contrib/database/api/rows/views.py b/backend/src/baserow/contrib/database/api/rows/views.py index 857a636947..fc47cc23f9 100644 --- a/backend/src/baserow/contrib/database/api/rows/views.py +++ b/backend/src/baserow/contrib/database/api/rows/views.py @@ -442,7 +442,7 @@ def get(self, request, table_id, query_params): model = table.get_model() queryset = model.objects.all().enhance_by_fields(**field_kwargs) queryset = view_handler.apply_filters(view, queryset) - queryset = view_handler.apply_sorting(view, queryset) + queryset = view_handler.apply_ordering(view, queryset) else: model = table.get_model( fields=fields, diff --git a/backend/src/baserow/contrib/database/api/views/grid/utils.py b/backend/src/baserow/contrib/database/api/views/grid/utils.py index aa5fe96c98..33022dbdab 100644 --- a/backend/src/baserow/contrib/database/api/views/grid/utils.py +++ b/backend/src/baserow/contrib/database/api/views/grid/utils.py @@ -15,7 +15,6 @@ """ import json -import re from collections import defaultdict from typing import Any, Dict, Iterable, List, Optional, Tuple, Type @@ -31,14 +30,13 @@ from baserow.config.settings.utils import str_to_bool, try_int from baserow.contrib.database.api.views.utils import serialize_group_by_data_pages from baserow.contrib.database.fields.exceptions import OrderByFieldNotFound +from baserow.contrib.database.fields.field_sortings import parse_order_string from baserow.contrib.database.fields.models import Field from baserow.contrib.database.fields.registries import field_type_registry -from baserow.contrib.database.fields.utils import get_field_id_from_field_key from baserow.contrib.database.views.constants import GROUP_BY_DATA_DEFAULT_LIMIT from baserow.contrib.database.views.exceptions import ViewGroupByFieldNotSupported from baserow.contrib.database.views.handler import ViewHandler -from baserow.contrib.database.views.models import DEFAULT_SORT_TYPE_KEY, ViewGroupBy -from baserow.core.utils import split_comma_separated_string +from baserow.contrib.database.views.models import ViewGroupBy GROUP_BY_DATA_DESCENDANT_MAX_GROUPS = 2000 # Only a coarse backstop: a deep tree legitimately produces one parent page per internal @@ -75,41 +73,43 @@ def parse_adhoc_view_group_bys( or empty. """ - if not raw_group_by: + if raw_group_by is None: return None + if raw_group_by == "": + return [] if allowed_field_ids is not None: allowed_field_ids = set(allowed_field_ids) field_objects = model._field_objects + entries = parse_order_string(raw_group_by) + group_bys = [] - try: - raw_entries = split_comma_separated_string(raw_group_by) - except ValueError: - raise OrderByFieldNotFound(raw_group_by) - for raw_entry in raw_entries: - field_id = get_field_id_from_field_key(raw_entry, strict=False) + for entry in entries: if ( - field_id is None - or field_id not in field_objects - or (allowed_field_ids is not None and field_id not in allowed_field_ids) + entry.field_key is None + or entry.field_key not in field_objects + or ( + allowed_field_ids is not None + and entry.field_key not in allowed_field_ids + ) ): - raise OrderByFieldNotFound(raw_entry) + raise OrderByFieldNotFound(entry.raw) - order = "DESC" if raw_entry.startswith("-") else "ASC" - type_match = re.search(r"\[(.*?)\]", raw_entry) - sort_type = type_match.group(1) if type_match else DEFAULT_SORT_TYPE_KEY - - field_object = field_objects[field_id] + field_object = field_objects[entry.field_key] if not field_object["type"].check_can_group_by( - field_object["field"], sort_type + field_object["field"], entry.sort_type ): raise ViewGroupByFieldNotSupported( f"It is not possible to group by field type " - f"{field_object['type'].type} using sort type {sort_type}." + f"{field_object['type'].type} using sort type {entry.sort_type}." ) - group_bys.append(ViewGroupBy(field_id=field_id, order=order, type=sort_type)) + group_bys.append( + ViewGroupBy( + field_id=entry.field_key, order=entry.direction, type=entry.sort_type + ) + ) return group_bys diff --git a/backend/src/baserow/contrib/database/api/views/grid/views.py b/backend/src/baserow/contrib/database/api/views/grid/views.py index ab853048e8..819a50ad05 100644 --- a/backend/src/baserow/contrib/database/api/views/grid/views.py +++ b/backend/src/baserow/contrib/database/api/views/grid/views.py @@ -21,6 +21,7 @@ ADHOC_FILTERS_API_PARAMS_NO_COMBINE, ADHOC_FILTERS_API_PARAMS_WITH_AGGREGATION, ADHOC_FILTERS_API_PARAMS_WITH_AGGREGATION_NO_COMBINE, + ADHOC_GROUP_BY_API_PARAM, ADHOC_SORTING_API_PARAM, EXCLUDE_COUNT_API_PARAM, EXCLUDE_FIELDS_API_PARAM, @@ -231,6 +232,7 @@ def get_permissions(self): *PAGINATION_API_PARAMS, *ADHOC_FILTERS_API_PARAMS_NO_COMBINE, ADHOC_SORTING_API_PARAM, + ADHOC_GROUP_BY_API_PARAM, INCLUDE_FIELDS_API_PARAM, EXCLUDE_FIELDS_API_PARAM, SEARCH_VALUE_API_PARAM, @@ -278,6 +280,7 @@ def get_permissions(self): "ERROR_VIEW_FILTER_TYPE_DOES_NOT_EXIST", "ERROR_VIEW_FILTER_TYPE_UNSUPPORTED_FIELD", "ERROR_FILTERS_PARAM_VALIDATION_ERROR", + "ERROR_VIEW_GROUP_BY_FIELD_NOT_SUPPORTED", ] ), 404: get_error_schema( @@ -295,6 +298,7 @@ def get_permissions(self): ViewFilterTypeDoesNotExist: ERROR_VIEW_FILTER_TYPE_DOES_NOT_EXIST, ViewFilterTypeNotAllowedForField: ERROR_VIEW_FILTER_TYPE_UNSUPPORTED_FIELD, FieldDoesNotExist: ERROR_FIELD_DOES_NOT_EXIST, + ViewGroupByFieldNotSupported: ERROR_VIEW_GROUP_BY_FIELD_NOT_SUPPORTED, } ) @allowed_includes("field_options", "row_metadata", "group_by_metadata") @@ -321,6 +325,7 @@ def get( exclude_fields = request.GET.get("exclude_fields") adhoc_filters = AdHocFilters.from_request(request) order_by = request.GET.get("order_by") + group_by = request.GET.get("group_by") view_handler = ViewHandler() view = view_handler.get_view_as_user( @@ -353,6 +358,7 @@ def get( order_by, query_params, hidden_field_ids=hidden_field_ids, + group_by=group_by, ) model = queryset.model @@ -363,15 +369,36 @@ def get( queryset, request, field_ids, exclude_field_ids=hidden_field_ids ) - if group_by_metadata and view_type.can_group_by and view.viewgroupby_set.all(): - group_by_fields = [ - model._field_objects[group_by.field_id]["field"] - for group_by in view.viewgroupby_set.all() - ] - serialized_group_by_metadata = serialize_group_by_fields_metadata( - queryset, group_by_fields, page + if group_by_metadata and view_type.can_group_by: + visible_field_ids = ( + {fid for fid in model._field_objects if fid not in hidden_field_ids} + if hidden_field_ids + else None ) - response.data.update(group_by_metadata=serialized_group_by_metadata) + if group_by is not None: + adhoc_group_bys = parse_adhoc_view_group_bys( + group_by, model, allowed_field_ids=visible_field_ids + ) + group_by_fields = ( + [ + model._field_objects[gb.field_id]["field"] + for gb in adhoc_group_bys + ] + if adhoc_group_bys + else [] + ) + else: + group_by_fields = [ + model._field_objects[gb.field_id]["field"] + for gb in view.viewgroupby_set.all() + if not hidden_field_ids or gb.field_id not in hidden_field_ids + ] + + if group_by_fields: + serialized_group_by_metadata = serialize_group_by_fields_metadata( + queryset, group_by_fields, page + ) + response.data.update(group_by_metadata=serialized_group_by_metadata) if field_options: response.data.update( @@ -561,21 +588,38 @@ def get(self, request, view_id, query_params): serialize_group_by_data_pages([empty_group_by_data_page()], []) ) + hidden_field_ids = get_hidden_field_ids_for_view_user(request.user, view) queryset = get_view_filtered_queryset( request.user, view, adhoc_filters, order_by=None, query_params=query_params, + hidden_field_ids=hidden_field_ids, ) # Users who can list but not update the view's group-bys (e.g. viewers) # group ad hoc, so an explicit `group_by` parameter takes precedence over # the saved configuration. + visible_field_ids = ( + { + fid + for fid in queryset.model._field_objects + if fid not in hidden_field_ids + } + if hidden_field_ids + else None + ) view_group_bys = parse_adhoc_view_group_bys( - request.GET.get("group_by"), queryset.model + request.GET.get("group_by"), + queryset.model, + allowed_field_ids=visible_field_ids, ) if view_group_bys is None: - view_group_bys = list(view.viewgroupby_set.all()) + view_group_bys = [ + gb + for gb in view.viewgroupby_set.all() + if not hidden_field_ids or gb.field_id not in hidden_field_ids + ] if not view_group_bys: return Response( @@ -1029,7 +1073,11 @@ def get(self, request, slug, query_params): allowed_field_ids=visible_field_ids, ) if view_group_bys is None: - view_group_bys = list(view.viewgroupby_set.all()) + view_group_bys = [ + gb + for gb in view.viewgroupby_set.all() + if gb.field_id in visible_field_ids + ] if not view_group_bys: return Response( @@ -1150,6 +1198,7 @@ class PublicGridViewRowsView(APIView): "ERROR_VIEW_FILTER_TYPE_DOES_NOT_EXIST", "ERROR_VIEW_FILTER_TYPE_UNSUPPORTED_FIELD", "ERROR_FILTERS_PARAM_VALIDATION_ERROR", + "ERROR_VIEW_GROUP_BY_FIELD_NOT_SUPPORTED", ] ), 401: get_error_schema(["ERROR_NO_AUTHORIZATION_TO_PUBLICLY_SHARED_VIEW"]), @@ -1169,6 +1218,7 @@ class PublicGridViewRowsView(APIView): ViewFilterTypeNotAllowedForField: ERROR_VIEW_FILTER_TYPE_UNSUPPORTED_FIELD, FieldDoesNotExist: ERROR_FIELD_DOES_NOT_EXIST, NoAuthorizationToPubliclySharedView: ERROR_NO_AUTHORIZATION_TO_PUBLICLY_SHARED_VIEW, + ViewGroupByFieldNotSupported: ERROR_VIEW_GROUP_BY_FIELD_NOT_SUPPORTED, } ) @allowed_includes("field_options", "group_by_metadata") diff --git a/backend/src/baserow/contrib/database/api/views/utils.py b/backend/src/baserow/contrib/database/api/views/utils.py index e1faa81ca0..337f3b4a55 100644 --- a/backend/src/baserow/contrib/database/api/views/utils.py +++ b/backend/src/baserow/contrib/database/api/views/utils.py @@ -25,6 +25,7 @@ get_row_serializer_class, ) from baserow.contrib.database.api.views.serializers import serialize_group_by_metadata +from baserow.contrib.database.fields.field_sortings import serialize_sorts_to_string from baserow.contrib.database.fields.models import Field from baserow.contrib.database.fields.registries import field_type_registry from baserow.contrib.database.rows.registries import row_metadata_registry @@ -76,6 +77,7 @@ def get_view_filtered_queryset( query_params: Optional[Dict[str, Any]] = None, model: Optional[GeneratedTableModel] = None, hidden_field_ids: Optional[Set[int]] = None, + group_by: Optional[str] = None, ) -> QuerySet: """ Returns a queryset that is filtered based on the provided view, adhoc filters, and @@ -90,6 +92,9 @@ def get_view_filtered_queryset( :param model: The model to filter the queryset by. :param hidden_field_ids: Optional set of field IDs hidden from the user. When provided, search will be restricted to visible fields only. + :param group_by: The raw ``group_by`` query parameter string. Fields listed + here will use ``get_group_by_sort_order`` instead of ``get_order`` so + that their row ordering matches the group tree. :return: The filtered queryset. """ @@ -100,10 +105,13 @@ def get_view_filtered_queryset( query_params = {} has_adhoc_filters = filters is not None and filters.has_any_filters - has_adhoc_sorts = order_by is not None search_value = query_params.get("search") search_mode = query_params.get("search_mode") + has_adhoc_sorting = order_by is not None + has_adhoc_grouping = group_by is not None + has_any_adhoc_ordering = has_adhoc_sorting or has_adhoc_grouping + only_search_by_field_ids = None if hidden_field_ids: only_search_by_field_ids = [ @@ -115,7 +123,7 @@ def get_view_filtered_queryset( queryset = ViewHandler().get_queryset( user, view, - apply_sorts=not has_adhoc_sorts, + apply_sorts=not has_any_adhoc_ordering, apply_filters=not has_adhoc_filters, search=search_value, search_mode=search_mode, @@ -123,8 +131,23 @@ def get_view_filtered_queryset( only_search_by_field_ids=only_search_by_field_ids, ) - if has_adhoc_sorts: - queryset = queryset.order_by_fields_string(order_by, False) + if has_any_adhoc_ordering: + effective_group_by = group_by or "" + effective_order_by = order_by or "" + + if not has_adhoc_grouping: + view_type = view_type_registry.get_by_model(view.specific_class) + if view_type.can_group_by: + effective_group_by = serialize_sorts_to_string( + view.viewgroupby_set.all() + ) + + if not has_adhoc_sorting: + effective_order_by = serialize_sorts_to_string(view.viewsort_set.all()) + + queryset = queryset.order_by_fields_string( + effective_order_by, False, group_by_string=effective_group_by or None + ) if has_adhoc_filters: queryset = filters.apply_to_queryset(model, queryset) diff --git a/backend/src/baserow/contrib/database/export/file_writer.py b/backend/src/baserow/contrib/database/export/file_writer.py index 063a764b1f..00260bfc11 100644 --- a/backend/src/baserow/contrib/database/export/file_writer.py +++ b/backend/src/baserow/contrib/database/export/file_writer.py @@ -275,9 +275,13 @@ def add_ad_hoc_filters_dict_to_queryset(self, filters_dict, only_by_field_ids=No filters.only_filter_by_field_ids = only_by_field_ids self.queryset = filters.apply_to_queryset(self.queryset.model, self.queryset) - def add_add_hoc_order_by_to_queryset(self, order_by, only_by_field_ids=None): + def add_ad_hoc_order_by_to_queryset( + self, order_by, only_by_field_ids=None, group_by_string=None + ): self.queryset = self.queryset.order_by_fields_string( - order_by, only_order_by_field_ids=only_by_field_ids + order_by, + only_order_by_field_ids=only_by_field_ids, + group_by_string=group_by_string, ) def _get_field_serializer(self, field_object: FieldObject) -> Callable[[Any], Any]: diff --git a/backend/src/baserow/contrib/database/export/handler.py b/backend/src/baserow/contrib/database/export/handler.py index 3d9dd50559..02078ab917 100755 --- a/backend/src/baserow/contrib/database/export/handler.py +++ b/backend/src/baserow/contrib/database/export/handler.py @@ -21,6 +21,7 @@ ) from baserow.contrib.database.export.operations import ExportTableOperationType from baserow.contrib.database.export.tasks import run_export_job +from baserow.contrib.database.fields.field_sortings import serialize_sorts_to_string from baserow.contrib.database.table.models import Table from baserow.contrib.database.views.exceptions import ViewNotInTable from baserow.contrib.database.views.filters import AdHocFilters @@ -266,9 +267,12 @@ def _raise_if_invalid_order_by_or_filters( # Validate the sort object before the job start, so that the validation error # can be shown to the user. order_by = export_options.get("order_by", None) - if order_by is not None: + group_by = export_options.get("group_by", None) + if order_by is not None or group_by is not None: queryset.order_by_fields_string( - order_by, only_order_by_field_ids=only_by_field_ids + order_by or "", + only_order_by_field_ids=only_by_field_ids, + group_by_string=group_by, ) @@ -358,6 +362,7 @@ def _open_file_and_run_export(job: ExportJob) -> ExportJob: filters = job.export_options.pop("filters", None) order_by = job.export_options.pop("order_by", None) + group_by = job.export_options.pop("group_by", None) visible_fields_in_order = job.export_options.pop("fields", None) include_row_id = job.export_options.pop("include_row_id", True) include_primary_field = job.export_options.pop("include_primary_field", True) @@ -385,9 +390,26 @@ def _open_file_and_run_export(job: ExportJob) -> ExportJob: filters, only_by_field_ids=only_by_field_ids ) - if order_by is not None: - serializer.add_add_hoc_order_by_to_queryset( - order_by, only_by_field_ids=only_by_field_ids + if order_by is not None or group_by is not None: + effective_order_by = order_by or "" + effective_group_by = group_by or "" + + if job.view is not None: + if order_by is None: + effective_order_by = serialize_sorts_to_string( + job.view.viewsort_set.all() + ) + if group_by is None: + vt = view_type_registry.get_by_model(job.view.specific_class) + if vt.can_group_by: + effective_group_by = serialize_sorts_to_string( + job.view.viewgroupby_set.all() + ) + + serializer.add_ad_hoc_order_by_to_queryset( + effective_order_by, + only_by_field_ids=only_by_field_ids, + group_by_string=effective_group_by or None, ) serializer.write_to_file( diff --git a/backend/src/baserow/contrib/database/export/table_exporters/csv_table_exporter.py b/backend/src/baserow/contrib/database/export/table_exporters/csv_table_exporter.py index 991c2e6052..131d2bf70d 100644 --- a/backend/src/baserow/contrib/database/export/table_exporters/csv_table_exporter.py +++ b/backend/src/baserow/contrib/database/export/table_exporters/csv_table_exporter.py @@ -54,6 +54,7 @@ def write_to_file( export_charset="utf-8", csv_column_separator=",", csv_include_header=True, + **kwargs, ): """ Writes the queryset to the provided file in csv format using the provided diff --git a/backend/src/baserow/contrib/database/fields/field_sortings.py b/backend/src/baserow/contrib/database/fields/field_sortings.py index 4df62793f4..ef8532cda3 100644 --- a/backend/src/baserow/contrib/database/fields/field_sortings.py +++ b/backend/src/baserow/contrib/database/fields/field_sortings.py @@ -1,8 +1,92 @@ +import re from dataclasses import dataclass -from typing import Any, Dict, List, Optional +from typing import Any, Dict, List, NamedTuple, Optional from django.db.models.expressions import OrderBy +from baserow.contrib.database.fields.exceptions import OrderByFieldNotFound +from baserow.contrib.database.fields.utils import get_field_id_from_field_key +from baserow.core.utils import split_comma_separated_string + +DEFAULT_SORT_TYPE_KEY = "default" + + +class ParsedOrderEntry(NamedTuple): + raw: str + field_key: Any + direction: str + sort_type: str + + +def parse_order_string( + order_string: str, + *, + user_field_names: bool = False, + field_name_parser=None, +) -> List[ParsedOrderEntry]: + """ + Parses a comma-separated ``field_X[type]`` order string into structured + entries without model validation. Callers apply their own capability + checks (``check_can_order_by`` or ``check_can_group_by``). + + :param order_string: e.g. ``"-field_1[default],field_2"`` + :param user_field_names: When True, treat entries as literal field names + instead of field-key IDs. + :param field_name_parser: Callable that strips prefixes from a raw entry + to extract the field name. Required when ``user_field_names=True``. + :raises OrderByFieldNotFound: When the string cannot be split. + :return: List of parsed entries. + """ + + try: + raw_fields = split_comma_separated_string(order_string) + except ValueError: + raise OrderByFieldNotFound(order_string) + + entries = [] + for raw in raw_fields: + if user_field_names: + field_key = field_name_parser(raw) if field_name_parser else raw + else: + field_key = get_field_id_from_field_key(raw, strict=False) + + direction = "DESC" if raw[:1] == "-" else "ASC" + type_match = re.search(r"\[(.*?)\]", raw) + sort_type = type_match.group(1) if type_match else DEFAULT_SORT_TYPE_KEY + + entries.append( + ParsedOrderEntry( + raw=raw, field_key=field_key, direction=direction, sort_type=sort_type + ) + ) + + return entries + + +def serialize_sort_to_string(sort_or_group_by) -> str: + """ + Serializes a ViewSort or ViewGroupBy instance back into the + ``field_X[type]`` transport format. + """ + + prefix = "-" if sort_or_group_by.order == "DESC" else "" + suffix = ( + f"[{sort_or_group_by.type}]" + if sort_or_group_by.type != DEFAULT_SORT_TYPE_KEY + else "" + ) + return f"{prefix}field_{sort_or_group_by.field_id}{suffix}" + + +def serialize_sorts_to_string(sorts) -> str: + """ + Serializes an iterable of ViewSort/ViewGroupBy instances into a + comma-separated ``field_X[type]`` string. + """ + + parts = [serialize_sort_to_string(s) for s in sorts] + return ",".join(parts) if parts else "" + @dataclass class OptionallyAnnotatedOrderBy: diff --git a/backend/src/baserow/contrib/database/fields/field_types.py b/backend/src/baserow/contrib/database/fields/field_types.py index 45e5772302..2551458bc7 100755 --- a/backend/src/baserow/contrib/database/fields/field_types.py +++ b/backend/src/baserow/contrib/database/fields/field_types.py @@ -5383,28 +5383,64 @@ def get_group_by_display_values(self, field, field_name, raw_values): for value in raw_values ] + def _get_through_sort_expression(self, field, field_name, sort_type): + """ + Returns the expression to aggregate inside a through-table subquery, + referencing the related model's column via ``related_field__``. + """ + + if sort_type == SINGLE_SELECT_SORT_BY_ORDER: + return "order" + return "value" + def get_order( self, field, field_name, order_direction, sort_type, table_model=None ): """ - Order by the concatenated values of the select options, separated by a comma. + Order by the concatenated values of the select options, separated by a + comma. Uses a correlated subquery against the through table when + ``table_model`` is available, preserving insertion order and preventing + M2M join multiplication. """ - # FIXME: this is broken because the field sort items by insertion order with the - # id in the through table. It's fixable here using a subquery on the m2m table - # instead of a `StringAgg`, but it will be very difficult to fix in the formula - # language. Also the frontend is not matching exactly the backend sorting and we - # should also consider the possibility that a comma can be part of the value. sort_column_name = f"{field_name}_agg_sort" - query = Coalesce( - StringAgg( - self.get_sortable_column_expression(field, field_name, sort_type), - ",", + + if table_model is not None: + through_model = table_model._meta.get_field(field_name).remote_field.through + reversed_field = through_model._meta.get_fields()[1].name + related_field = through_model._meta.get_fields()[2].name + sort_attr = self._get_through_sort_expression(field, field_name, sort_type) + + query = Coalesce( + Subquery( + through_model.objects.filter( + **{f"{reversed_field}_id": OuterRef("id")} + ) + .values(f"{reversed_field}_id") + .annotate( + _agg=StringAgg( + F(f"{related_field}__{sort_attr}"), + ",", + ordering=F("id"), + output_field=models.TextField(), + ) + ) + .values("_agg")[:1] + ), + Value(""), output_field=models.TextField(), - ), - Value(""), - output_field=models.TextField(), - ) + ) + else: + query = Coalesce( + StringAgg( + self.get_sortable_column_expression(field, field_name, sort_type), + ",", + output_field=models.TextField(), + ), + Value(""), + output_field=models.TextField(), + ) + annotation = {sort_column_name: query} order = collate_expression(F(sort_column_name)) @@ -5420,9 +5456,10 @@ def get_group_by_sort_order( ): """ Group-by treats a cell as a set of options: ``{A, B}`` and ``{B, A}`` - are the same group. Uses ``ArrayAgg(ARRAY[order, id])`` to produce a + are the same group. Uses ``ArrayAgg(ARRAY[order, id])`` to produce a collision-proof, deterministic sort key based on the field-defined - option order. + option order. Wrapped in a correlated subquery when ``table_model`` is + available to prevent M2M join multiplication. """ sort_column_name = f"{field_name}_group_by_agg_sort" @@ -5437,7 +5474,7 @@ def get_group_by_sort_order( output_field=pair_field, ) - query = Coalesce( + agg = Coalesce( ArrayAgg( option_key, filter=Q(**{f"{field_name}__id__isnull": False}), @@ -5447,6 +5484,16 @@ def get_group_by_sort_order( output_field=sort_key_field, ) + if table_model is not None: + query = Subquery( + table_model.objects.filter(id=OuterRef("id")) + .values("id") + .annotate(_group_agg=agg) + .values("_group_agg")[:1] + ) + else: + query = agg + annotation = {sort_column_name: query} order = F(sort_column_name) @@ -7339,24 +7386,49 @@ def get_order( self, field, field_name, order_direction, sort_type, table_model=None ): """ - If the user wants to sort the results they expect them to be ordered - alphabetically based on the user's name and not in the id which is - stored in the table. This method generates a Case expression which maps - the id to the correct position. + Sort collaborators alphabetically by name. Uses a correlated subquery + against the through table when ``table_model`` is available, preserving + insertion order and preventing M2M join multiplication. """ sort_column_name = f"{field_name}_agg_sort" - query = Coalesce( - StringAgg( - self.get_sortable_column_expression(field, field_name, sort_type), - "", + + if table_model is not None: + through_model = table_model._meta.get_field(field_name).remote_field.through + reversed_field = through_model._meta.get_fields()[1].name + related_field = through_model._meta.get_fields()[2].name + + query = Coalesce( + Subquery( + through_model.objects.filter( + **{f"{reversed_field}_id": OuterRef("id")} + ) + .values(f"{reversed_field}_id") + .annotate( + _agg=StringAgg( + F(f"{related_field}__first_name"), + "", + ordering=F("id"), + output_field=models.TextField(), + ) + ) + .values("_agg")[:1] + ), + Value(""), output_field=models.TextField(), - ), - Value(""), - output_field=models.TextField(), - ) - annotation = {sort_column_name: query} + ) + else: + query = Coalesce( + StringAgg( + self.get_sortable_column_expression(field, field_name, sort_type), + "", + output_field=models.TextField(), + ), + Value(""), + output_field=models.TextField(), + ) + annotation = {sort_column_name: query} order = collate_expression(F(sort_column_name)) if order_direction == "DESC": @@ -7371,9 +7443,10 @@ def get_group_by_sort_order( ): """ Group-by treats a cell as a set of collaborators: ``{A, B}`` and - ``{B, A}`` are the same group. Uses ``ArrayAgg(ARRAY[first_name, id])`` + ``{B, A}`` are the same group. Uses ``ArrayAgg(ARRAY[first_name, id])`` ordered by ``(first_name, id)`` to produce a collision-proof sort key - with alphabetical group ordering. + with alphabetical group ordering. Wrapped in a correlated subquery when + ``table_model`` is available to prevent M2M join multiplication. """ sort_column_name = f"{field_name}_group_by_agg_sort" @@ -7394,7 +7467,7 @@ def get_group_by_sort_order( output_field=pair_field, ) - query = Coalesce( + agg = Coalesce( ArrayAgg( option_key, filter=Q(**{f"{field_name}__id__isnull": False}), @@ -7404,6 +7477,16 @@ def get_group_by_sort_order( output_field=sort_key_field, ) + if table_model is not None: + query = Subquery( + table_model.objects.filter(id=OuterRef("id")) + .values("id") + .annotate(_group_agg=agg) + .values("_group_agg")[:1] + ) + else: + query = agg + annotation = {sort_column_name: query} order = F(sort_column_name) diff --git a/backend/src/baserow/contrib/database/table/models.py b/backend/src/baserow/contrib/database/table/models.py index b14c19e2e0..f4a0b630da 100644 --- a/backend/src/baserow/contrib/database/table/models.py +++ b/backend/src/baserow/contrib/database/table/models.py @@ -28,6 +28,7 @@ FilterBuilder, parse_ids_from_csv_string, ) +from baserow.contrib.database.fields.field_sortings import parse_order_string from baserow.contrib.database.fields.fields import IgnoreMissingForeignKey from baserow.contrib.database.fields.models import ( CreatedOnField, @@ -35,7 +36,6 @@ LastModifiedField, ) from baserow.contrib.database.fields.registries import FieldType, field_type_registry -from baserow.contrib.database.fields.utils import get_field_id_from_field_key from baserow.contrib.database.search.handler import ( ALL_SEARCH_MODES, SearchHandler, @@ -53,7 +53,6 @@ ) from baserow.contrib.database.table.queryset import BaserowCTEQuerySet from baserow.contrib.database.views.exceptions import ViewFilterTypeNotAllowedForField -from baserow.contrib.database.views.models import DEFAULT_SORT_TYPE_KEY from baserow.contrib.database.views.registries import view_filter_type_registry from baserow.core.cache import local_cache from baserow.core.db import MultiFieldPrefetchQuerysetMixin, specific_iterator @@ -70,7 +69,7 @@ TrashableModelMixin, ) from baserow.core.telemetry.utils import baserow_trace -from baserow.core.utils import are_kwargs_default, split_comma_separated_string +from baserow.core.utils import are_kwargs_default extract_filter_sections_regex = re.compile(r"filter__(.+)__(.+)$") field_id_regex = re.compile(r"field_(\d+)$") @@ -232,8 +231,77 @@ def _get_field_name(self, field: str) -> str: else: return field + def _parse_order_fields( + self, + order_string, + user_field_names, + field_object_dict, + only_order_by_field_ids, + for_group_by=False, + ): + """ + Parses a comma-separated order string into a list of + ``(field_object, order_direction, sort_type)`` tuples, validating each + entry against the model's field objects. + + When ``for_group_by`` is True, validation uses ``check_can_group_by`` + instead of ``check_can_order_by`` and raises + ``ViewGroupByFieldNotSupported`` on failure. + """ + + entries = parse_order_string( + order_string, + user_field_names=user_field_names, + field_name_parser=self._get_field_name if user_field_names else None, + ) + + parsed = [] + for entry in entries: + if entry.field_key not in field_object_dict or ( + only_order_by_field_ids is not None + and entry.field_key not in only_order_by_field_ids + ): + raise OrderByFieldNotFound(entry.raw) + + field_object = field_object_dict[entry.field_key] + field_type = field_object["type"] + field_name = field_object["name"] + user_field_name = field_object["field"].name + error_display_name = user_field_name if user_field_names else field_name + + if for_group_by: + if not field_type.check_can_group_by( + field_object["field"], entry.sort_type + ): + from baserow.contrib.database.views.exceptions import ( + ViewGroupByFieldNotSupported, + ) + + raise ViewGroupByFieldNotSupported( + f"It is not possible to group by field type " + f"{field_type.type} using sort type {entry.sort_type}." + ) + else: + if not field_type.check_can_order_by( + field_object["field"], entry.sort_type + ): + raise OrderByFieldNotPossible( + error_display_name, + field_type.type, + entry.sort_type, + f"It is not possible to order by field type " + f"{field_type.type} using sort type {entry.sort_type}.", + ) + + parsed.append((field_object, entry.direction, entry.sort_type)) + return parsed + def order_by_fields_string( - self, order_string, user_field_names=False, only_order_by_field_ids=None + self, + order_string, + user_field_names=False, + only_order_by_field_ids=None, + group_by_string=None, ): """ Orders the query by the given field order string. This string is often @@ -245,6 +313,10 @@ def order_by_fields_string( order_string is treated as a comma separated list of the actual field names, use quotes to wrap field names containing commas. + When ``group_by_string`` is provided, those fields are ordered first using + ``get_group_by_sort_order`` (set-based ordering for M2M fields), followed + by the regular sort fields from ``order_string`` using ``get_order``. + :param order_string: The field ids to order the queryset by separated by a comma. For example `field_1,2` which will order by field with id 1 first and then by field with id 2 second. @@ -256,6 +328,11 @@ def order_by_fields_string( ordered by. Other fields not in the iterable will be ignored and not be filtered. :type only_order_by_field_ids: Optional[Iterable[int]] + :param group_by_string: Optional comma-separated field string (same format + as ``order_string``) whose fields use ``get_group_by_sort_order`` + instead of ``get_order``. Group-by fields are ordered before sort + fields. + :type group_by_string: Optional[str] :raises OrderByFieldNotFound: when the provided field id is not found in the model. :raises OrderByFieldNotPossible: when it is not possible to order by the @@ -264,11 +341,6 @@ def order_by_fields_string( :rtype: QuerySet """ - try: - order_by_fields = split_comma_separated_string(order_string) - except ValueError: - raise OrderByFieldNotFound(order_string) - if user_field_names: field_object_dict = { o["field"].name: o for o in self.model._field_objects.values() @@ -278,47 +350,44 @@ def order_by_fields_string( annotations = {} order_by = [] - for order in order_by_fields: - if user_field_names: - field_name_or_id = self._get_field_name(order) - else: - field_name_or_id = get_field_id_from_field_key(order, False) - if field_name_or_id not in field_object_dict or ( - only_order_by_field_ids is not None - and field_name_or_id not in only_order_by_field_ids + if group_by_string: + for field_object, direction, sort_type in self._parse_order_fields( + group_by_string, + user_field_names, + field_object_dict, + only_order_by_field_ids, + for_group_by=True, ): - raise OrderByFieldNotFound(order) - - order_direction = "DESC" if order[:1] == "-" else "ASC" - type_match = re.search(r"\[(.*?)\]", order) - sort_type = type_match.group(1) if type_match else DEFAULT_SORT_TYPE_KEY - field_object = field_object_dict[field_name_or_id] - field_type = field_object["type"] - field_name = field_object["name"] - field = field_object["field"] - user_field_name = field_object["field"].name - error_display_name = user_field_name if user_field_names else field_name - - if not field_object["type"].check_can_order_by( - field_object["field"], sort_type + annotated = field_object["type"].get_group_by_sort_order( + field_object["field"], + field_object["name"], + direction, + sort_type, + table_model=self.model, + ) + if annotated.annotation is not None: + annotations = {**annotations, **annotated.annotation} + order_by.extend(annotated.order_bys) + + # Sort fields — use get_order for regular ordering. + if order_string: + for field_object, direction, sort_type in self._parse_order_fields( + order_string, + user_field_names, + field_object_dict, + only_order_by_field_ids, ): - raise OrderByFieldNotPossible( - error_display_name, - field_type.type, + annotated = field_object["type"].get_order( + field_object["field"], + field_object["name"], + direction, sort_type, - f"It is not possible to order by field type {field_type.type} using sort type {sort_type}.", + table_model=self.model, ) - - field_annotated_order_by = field_type.get_order( - field, field_name, order_direction, sort_type, table_model=self.model - ) - - if field_annotated_order_by.annotation is not None: - annotations = {**annotations, **field_annotated_order_by.annotation} - field_order_bys = field_annotated_order_by.order_bys - for field_order_by in field_order_bys: - order_by.append(field_order_by) + if annotated.annotation is not None: + annotations = {**annotations, **annotated.annotation} + order_by.extend(annotated.order_bys) order_by.append("order") order_by.append("id") diff --git a/backend/src/baserow/contrib/database/views/handler.py b/backend/src/baserow/contrib/database/views/handler.py index 36302917c3..e8314c1512 100644 --- a/backend/src/baserow/contrib/database/views/handler.py +++ b/backend/src/baserow/contrib/database/views/handler.py @@ -48,7 +48,10 @@ AdvancedFilterBuilder, FilterBuilder, ) -from baserow.contrib.database.fields.field_sortings import OptionallyAnnotatedOrderBy +from baserow.contrib.database.fields.field_sortings import ( + OptionallyAnnotatedOrderBy, + serialize_sorts_to_string, +) from baserow.contrib.database.fields.models import Field, LinkRowField from baserow.contrib.database.fields.operations import ReadFieldOperationType from baserow.contrib.database.fields.registries import ( @@ -405,7 +408,7 @@ def get_index( field_order_bys = [] - for view_sort_or_group_by in view.get_all_sorts(): + for view_sort_or_group_by in view.get_all_ordering(): field_object = model._field_objects[view_sort_or_group_by.field_id] annotated_order_by = field_object["type"].get_order( field_object["field"], @@ -2306,7 +2309,7 @@ def get_view_order_bys( """ order_by = [] - for view_sort_or_group_by in view.get_all_sorts(restrict_to_field_ids): + for view_sort_or_group_by in view.get_all_ordering(restrict_to_field_ids): # If the to be sort field is not present in the `_field_objects` we # cannot filter so we raise a ValueError. if view_sort_or_group_by.field_id not in model._field_objects: @@ -2348,36 +2351,25 @@ def get_view_order_bys( return order_by, queryset - def apply_sorting( + def apply_ordering( self, view: View, queryset: QuerySet, restrict_to_field_ids: Optional[Iterable[int]] = None, ) -> QuerySet: """ - Applies the view's sorting to the given queryset. The first sort, which for now - is the first created, will always be applied first. Secondary sortings are - going to be applied if the values of the first sort rows are the same. - - Example: - - id | field_1 | field_2 - 1 | Bram | 20 - 2 | Bram | 10 - 3 | Elon | 30 - - If we are going to sort ascending on field_1 and field_2 the resulting ids are - going to be 2, 1 and 3 in that order. + Applies the view's full ordering — group-bys first, then sorts — to the + given queryset. Group-by fields use ``get_group_by_sort_order`` (set-based + ordering for M2M fields), while sort fields use ``get_order``. - :param view: The view where to fetch the sorting from. - :param queryset: The queryset where the sorting need to be applied to. + :param view: The view whose group-bys and sorts to apply. + :param queryset: The queryset to order. :param restrict_to_field_ids: Only field ids in this iterable will have their - view sorts applied in the resulting queryset. + view sorts/group-bys applied in the resulting queryset. :raises ValueError: When the queryset's model is not a table model or if the - table model does not contain the one of the fields. - :raises ViewSortDoesNotExist: When the view is trashed - - :return: The queryset where the sorting has been applied to. + table model does not contain one of the fields. + :raises ViewSortDoesNotExist: When the view is trashed. + :return: The queryset with ordering applied. """ model = queryset.model @@ -3310,7 +3302,7 @@ def get_queryset( if view_type.can_filter and apply_filters: queryset = self.apply_filters(view, queryset) if view_type.can_sort and apply_sorts: - queryset = self.apply_sorting( + queryset = self.apply_ordering( view, queryset, only_sort_by_field_ids, @@ -4120,23 +4112,28 @@ def get_public_rows_queryset_and_field_ids( queryset = table_model.objects.all().enhance_by_fields() queryset = self.apply_filters(view, queryset) - if view_type.can_group_by: - has_group_by = group_by is not None and group_by != "" - has_order_by = order_by is not None and order_by != "" - # If both the group by and order by string is set, then we must merge the - # two so that it will be sorted the right way because the grouping is - # basically just sorting for the backend. However, the group by will take - # precedence. - if has_group_by and has_order_by: - order_by = f"{group_by},{order_by}" - # If only the group_by is set, then we can simply replace the order_by - # because that must be applied to the queryset. - elif has_group_by: - order_by = group_by - - if order_by is not None and order_by != "": + has_adhoc_sorting = order_by is not None + group_by_for_ordering = group_by if view_type.can_group_by else None + has_adhoc_grouping = group_by_for_ordering is not None + has_any_adhoc_ordering = has_adhoc_sorting or has_adhoc_grouping + + if has_any_adhoc_ordering: + effective_group_by = group_by_for_ordering or "" + effective_order_by = order_by or "" + + if not has_adhoc_grouping and view_type.can_group_by: + effective_group_by = serialize_sorts_to_string( + view.viewgroupby_set.all() + ) + + if not has_adhoc_sorting: + effective_order_by = serialize_sorts_to_string(view.viewsort_set.all()) + queryset = queryset.order_by_fields_string( - order_by, False, visible_field_ids + effective_order_by, + False, + visible_field_ids, + group_by_string=effective_group_by or None, ) if adhoc_filters.has_any_filters: diff --git a/backend/src/baserow/contrib/database/views/models.py b/backend/src/baserow/contrib/database/views/models.py index e27a705884..62405ec76f 100644 --- a/backend/src/baserow/contrib/database/views/models.py +++ b/backend/src/baserow/contrib/database/views/models.py @@ -238,7 +238,7 @@ class Meta: ), ] - def get_all_sorts( + def get_all_ordering( self, restrict_to_field_ids: Optional[Iterable[int]] = None ) -> Iterable["Union[ViewGroupBy, ViewSort]"]: """ diff --git a/backend/tests/baserow/contrib/database/api/export/__init__.py b/backend/tests/baserow/contrib/database/api/export/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/backend/tests/baserow/contrib/database/api/export/test_export_group_by_contract.py b/backend/tests/baserow/contrib/database/api/export/test_export_group_by_contract.py new file mode 100644 index 0000000000..dbac24d4fe --- /dev/null +++ b/backend/tests/baserow/contrib/database/api/export/test_export_group_by_contract.py @@ -0,0 +1,150 @@ +"""Export payload compatibility and independent grouping/sorting semantics.""" + +import csv +from unittest.mock import patch + +from django.core.files.storage import FileSystemStorage +from django.urls import reverse + +import pytest +from rest_framework.status import HTTP_200_OK + +from baserow.contrib.database.export.handler import ExportHandler +from baserow.contrib.database.export.models import ( + EXPORT_JOB_FINISHED_STATUS, + ExportJob, +) + + +@pytest.fixture +def export_grid(data_fixture): + user, token = data_fixture.create_user_and_token() + table = data_fixture.create_database_table(user=user) + group = data_fixture.create_text_field(table=table, name="Group", primary=True) + rank = data_fixture.create_number_field(table=table, name="Rank") + view = data_fixture.create_grid_view(table=table) + data_fixture.create_view_group_by(view=view, field=group, order="ASC") + data_fixture.create_view_sort(view=view, field=rank, order="DESC") + model = table.get_model() + rows = [ + model.objects.create(order=index, **{group.db_column: value, rank.db_column: n}) + for index, (value, n) in enumerate([("B", 2), ("A", 3), ("B", 4), ("A", 1)]) + ] + return token, table, view, group, rank, rows + + +@pytest.mark.django_db +@pytest.mark.parametrize("scope", ["table", "view"]) +@pytest.mark.parametrize("group_mode", ["omitted", "clear", "replace"]) +def test_export_job_preserves_group_by_presence( + export_grid, api_client, scope, group_mode +): + token, table, view, group, _, _ = export_grid + payload = {"exporter_type": "csv"} + if scope == "view": + payload["view_id"] = view.id + if group_mode != "omitted": + payload["group_by"] = "" if group_mode == "clear" else f"-{group.db_column}" + + response = api_client.post( + reverse("api:database:export:export_table", kwargs={"table_id": table.id}), + payload, + format="json", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + + assert response.status_code == HTTP_200_OK, response.json() + job = ExportJob.objects.get(id=response.json()["id"]) + assert job.view_id == (view.id if scope == "view" else None) + if group_mode == "omitted": + # This JSON payload crosses a deployment boundary: an older Celery worker + # does not consume group_by and forwards unknown keys to the CSV writer. + assert "group_by" not in job.export_options, ( + "An export request without group_by must not persist group_by=None. " + "Older workers forward that unexpected keyword to the exporter. " + f"Persisted options: {job.export_options}" + ) + else: + assert job.export_options["group_by"] == payload["group_by"] + + +@pytest.mark.django_db +@pytest.mark.parametrize("group_mode", ["inherit", "clear", "replace"]) +@pytest.mark.parametrize("sort_mode", ["inherit", "clear", "replace"]) +def test_csv_export_keeps_grouping_and_sorting_overrides_independent( + export_grid, api_client, tmp_path, settings, group_mode, sort_mode +): + token, table, view, group, rank, rows = export_grid + payload = {"exporter_type": "csv", "view_id": view.id} + if group_mode != "inherit": + payload["group_by"] = "" if group_mode == "clear" else f"-{group.db_column}" + if sort_mode != "inherit": + payload["order_by"] = "" if sort_mode == "clear" else rank.db_column + + response = api_client.post( + reverse("api:database:export:export_table", kwargs={"table_id": table.id}), + payload, + format="json", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + assert response.status_code == HTTP_200_OK, response.json() + job = ExportJob.objects.get(id=response.json()["id"]) + storage = FileSystemStorage(location=str(tmp_path), base_url="http://localhost") + with patch("baserow.core.storage.get_default_storage", return_value=storage): + ExportHandler.run_export_job(job) + + job.refresh_from_db() + assert job.state == EXPORT_JOB_FINISHED_STATUS + path = tmp_path / settings.EXPORT_FILES_DIRECTORY / job.exported_file_name + with path.open(encoding="utf-8-sig", newline="") as exported_file: + exported_rows = list(csv.DictReader(exported_file)) + + expected_order = { + ("inherit", "inherit"): [1, 3, 2, 0], + ("inherit", "clear"): [1, 3, 0, 2], + ("inherit", "replace"): [3, 1, 0, 2], + ("clear", "inherit"): [2, 1, 0, 3], + ("clear", "clear"): [0, 1, 2, 3], + ("clear", "replace"): [3, 0, 1, 2], + ("replace", "inherit"): [2, 0, 1, 3], + ("replace", "clear"): [0, 2, 1, 3], + ("replace", "replace"): [0, 2, 3, 1], + } + assert [int(row["id"]) for row in exported_rows] == [ + rows[index].id for index in expected_order[group_mode, sort_mode] + ] + + +@pytest.mark.django_db +def test_csv_exporter_write_to_file_tolerates_unknown_kwargs( + export_grid, api_client, tmp_path, settings +): + """ + During a rolling deploy an old Celery worker may forward unknown export + options (like group_by) to write_to_file. All exporters must accept + **kwargs so this does not crash. + """ + + token, table, view, group, rank, rows = export_grid + payload = {"exporter_type": "csv", "view_id": view.id} + + response = api_client.post( + reverse("api:database:export:export_table", kwargs={"table_id": table.id}), + payload, + format="json", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + assert response.status_code == HTTP_200_OK, response.json() + job = ExportJob.objects.get(id=response.json()["id"]) + # Simulate old worker not popping group_by from export_options. + job.export_options["group_by"] = f"field_{group.id}" + job.save(update_fields=["export_options"]) + + storage = FileSystemStorage(location=str(tmp_path), base_url="http://localhost") + with patch("baserow.core.storage.get_default_storage", return_value=storage): + ExportHandler.run_export_job(job) + + job.refresh_from_db() + assert job.state == EXPORT_JOB_FINISHED_STATUS, ( + f"Export should succeed even with unknown kwargs, but state={job.state}" + ) diff --git a/backend/tests/baserow/contrib/database/api/export/test_export_views.py b/backend/tests/baserow/contrib/database/api/export/test_export_views.py index 99a979373e..6666527595 100644 --- a/backend/tests/baserow/contrib/database/api/export/test_export_views.py +++ b/backend/tests/baserow/contrib/database/api/export/test_export_views.py @@ -10,6 +10,7 @@ from rest_framework.fields import DateTimeField from rest_framework.status import HTTP_200_OK, HTTP_400_BAD_REQUEST, HTTP_404_NOT_FOUND +from baserow.contrib.database.api.export.views import _validate_options from baserow.contrib.database.rows.handler import RowHandler @@ -549,3 +550,53 @@ def test_exporting_csv_with_formatted_number_field( ) with open(file_path, "r", encoding="utf-8") as written_file: assert written_file.read() == expected + + +@pytest.mark.parametrize( + "separator_name,expected_char", + [ + ("tab", "\t"), + ("record_separator", "\x1e"), + ("unit_separator", "\x1f"), + (",", ","), + ], +) +def test_validate_options_converts_display_separator_to_char( + separator_name, expected_char +): + result = _validate_options( + { + "exporter_type": "csv", + "csv_column_separator": separator_name, + } + ) + assert result["csv_column_separator"] == expected_char + + +@pytest.mark.parametrize( + "charset_display,expected_python", + [ + ("x-mac-cyrillic", "mac-cyrillic"), + ("windows-874", "cp874"), + ("utf-8", "utf-8"), + ], +) +def test_validate_options_converts_display_charset_to_python_encoding( + charset_display, expected_python +): + result = _validate_options( + { + "exporter_type": "csv", + "export_charset": charset_display, + } + ) + assert result["export_charset"] == expected_python + + +def test_validate_options_omits_group_by_when_not_provided(): + result = _validate_options( + { + "exporter_type": "csv", + } + ) + assert "group_by" not in result diff --git a/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_group_by_contract.py b/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_group_by_contract.py new file mode 100644 index 0000000000..fab354fac2 --- /dev/null +++ b/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_group_by_contract.py @@ -0,0 +1,242 @@ +"""The rows and group-tree APIs must agree on omitted, empty and ad-hoc grouping.""" + +from django.urls import reverse + +import pytest +from rest_framework.status import HTTP_200_OK + + +@pytest.fixture +def grouped_grid(data_fixture): + user, token = data_fixture.create_user_and_token() + table = data_fixture.create_database_table(user=user) + group = data_fixture.create_text_field(table=table, name="Group", primary=True) + category = data_fixture.create_text_field(table=table, name="Category") + rank = data_fixture.create_number_field(table=table, name="Rank") + view = data_fixture.create_grid_view(table=table, public=True) + data_fixture.create_view_group_by(view=view, field=group, order="ASC") + data_fixture.create_view_sort(view=view, field=rank, order="DESC") + + model = table.get_model() + rows = [ + model.objects.create( + order=index, + **{group.db_column: value, category.db_column: label, rank.db_column: n}, + ) + for index, (value, label, n) in enumerate( + [("B", "X", 2), ("A", "X", 3), ("B", "Y", 4), ("A", "X", 1)] + ) + ] + return view, token, group, category, rank, rows + + +def _options(group_mode, sort_mode, category, rank): + options = {} + if group_mode != "inherit": + options["group_by"] = "" if group_mode == "clear" else f"-{category.db_column}" + if sort_mode != "inherit": + options["order_by"] = "" if sort_mode == "clear" else rank.db_column + return options + + +def _endpoint(view, token, public, *, group_data=False): + if public: + name = "public-group-by-data" if group_data else "public_rows" + return ( + reverse(f"api:database:views:grid:{name}", kwargs={"slug": view.slug}), + {}, + ) + name = "group-by-data" if group_data else "list" + return ( + reverse(f"api:database:views:grid:{name}", kwargs={"view_id": view.id}), + {"HTTP_AUTHORIZATION": f"JWT {token}"}, + ) + + +# Explicit expected row positions avoid reproducing the implementation's sort logic. +EXPECTED_ORDER = { + ("inherit", "inherit"): [1, 3, 2, 0], + ("inherit", "clear"): [1, 3, 0, 2], + ("inherit", "replace"): [3, 1, 0, 2], + ("clear", "inherit"): [2, 1, 0, 3], + ("clear", "clear"): [0, 1, 2, 3], + ("clear", "replace"): [3, 0, 1, 2], + ("replace", "inherit"): [2, 1, 0, 3], + ("replace", "clear"): [2, 0, 1, 3], + ("replace", "replace"): [2, 3, 0, 1], +} + + +@pytest.mark.django_db +@pytest.mark.parametrize( + "public,group_mode,sort_mode", + [ + pytest.param( + public, group_mode, sort_mode, id=f"{scope}-{group_mode}-{sort_mode}" + ) + for public, scope in [(False, "private"), (True, "public")] + for group_mode in ["inherit", "clear", "replace"] + for sort_mode in ["inherit", "clear", "replace"] + # The public UI supplies an ordering option. The both-omitted public path + # has a separate baseline defect outside these PR regression cases. + if not (public and group_mode == sort_mode == "inherit") + ], +) +def test_rows_keep_grouping_and_sorting_overrides_independent( + grouped_grid, api_client, public, group_mode, sort_mode +): + view, token, _, category, rank, rows = grouped_grid + url, headers = _endpoint(view, token, public) + options = _options(group_mode, sort_mode, category, rank) + + response = api_client.get(url, options, **headers) + + assert response.status_code == HTTP_200_OK, response.json() + assert response.json()["count"] == 4 + assert [row["id"] for row in response.json()["results"]] == [ + rows[index].id for index in EXPECTED_ORDER[group_mode, sort_mode] + ] + # Fetching ad-hoc results must not rewrite the saved view configuration. + assert list(view.viewgroupby_set.values_list("field_id", "order")) == [ + (grouped_grid[2].id, "ASC") + ] + assert list(view.viewsort_set.values_list("field_id", "order")) == [ + (rank.id, "DESC") + ] + + +@pytest.mark.django_db +@pytest.mark.parametrize("public", [False, True], ids=["private", "public"]) +@pytest.mark.parametrize("group_mode", ["inherit", "clear", "replace"]) +def test_group_tree_distinguishes_omitted_empty_and_replacement_group_by( + grouped_grid, api_client, public, group_mode +): + view, token, group, category, rank, _ = grouped_grid + url, headers = _endpoint(view, token, public, group_data=True) + options = _options(group_mode, "inherit", category, rank) + + response = api_client.get(url, {**options, "limit": 10}, **headers) + + assert response.status_code == HTTP_200_OK, response.json() + pages = response.json()["pages"] + assert len(pages) == 1 + page = pages[0] + assert page["parent"] == {} + if group_mode == "clear": + assert page["groups"] == [], ( + "group_by= explicitly clears grouping; the group tree must not fall " + "back to the saved view while the rows endpoint returns ungrouped rows." + ) + assert page["group_count"] == 0 + return + + field = group if group_mode == "inherit" else category + values_and_counts = ( + [("A", 2), ("B", 2)] + if group_mode == "inherit" + else [ + ("Y", 1), + ("X", 3), + ] + ) + expected_groups = [ + { + "path": {field.db_column: value}, + "depth": 0, + "row_count": count, + "sibling_index": index, + "row_offset": 0 if index == 0 else values_and_counts[0][1], + } + for index, (value, count) in enumerate(values_and_counts) + ] + assert page == { + "parent": {}, + "groups": expected_groups, + "offset": 0, + "limit": 10, + "group_count": 2, + } + + response = api_client.get(url, {**options, "offset": 1, "limit": 1}, **headers) + + assert response.status_code == HTTP_200_OK, response.json() + assert response.json()["pages"] == [ + { + "parent": {}, + "groups": expected_groups[1:], + "offset": 1, + "limit": 1, + "group_count": 2, + } + ] + + +@pytest.mark.django_db +@pytest.mark.parametrize("group_mode", ["inherit", "clear", "replace"]) +def test_private_row_group_metadata_uses_the_effective_grouping( + grouped_grid, api_client, group_mode +): + view, token, group, category, rank, _ = grouped_grid + url, headers = _endpoint(view, token, False) + options = _options(group_mode, "inherit", category, rank) + + response = api_client.get( + url, {**options, "include": "group_by_metadata"}, **headers + ) + + assert response.status_code == HTTP_200_OK, response.json() + metadata = response.json().get("group_by_metadata", {}) + if group_mode == "clear": + assert metadata == {}, "Cleared grouping must not return saved group metadata." + return + + field = group if group_mode == "inherit" else category + expected_counts = {"A": 2, "B": 2} if group_mode == "inherit" else {"X": 3, "Y": 1} + assert set(metadata) == {field.db_column} + assert len(metadata[field.db_column]) == 2 + assert { + entry[field.db_column]: entry["count"] for entry in metadata[field.db_column] + } == expected_counts + + +@pytest.mark.django_db +def test_private_rows_saved_group_by_metadata_excludes_hidden_fields( + api_client, data_fixture +): + """ + When hidden_field_ids is non-None (enterprise restricted view), saved + group-bys on hidden fields must not leak in group_by_metadata on the + private rows endpoint. + """ + + from unittest.mock import patch + + user, token = data_fixture.create_user_and_token() + table = data_fixture.create_database_table(user=user) + visible = data_fixture.create_text_field(table=table, name="Visible") + hidden = data_fixture.create_text_field(table=table, name="Hidden") + grid = data_fixture.create_grid_view(table=table) + data_fixture.create_view_group_by(view=grid, field=visible) + data_fixture.create_view_group_by(view=grid, field=hidden) + + model = table.get_model() + model.objects.create(**{f"field_{visible.id}": "A", f"field_{hidden.id}": "secret"}) + + url = reverse("api:database:views:grid:list", kwargs={"view_id": grid.id}) + + with patch( + "baserow.contrib.database.api.views.grid.views.get_hidden_field_ids_for_view_user", + return_value={hidden.id}, + ): + response = api_client.get( + url, + {"include": "group_by_metadata"}, + HTTP_AUTHORIZATION=f"JWT {token}", + ) + + assert response.status_code == HTTP_200_OK + metadata = response.json().get("group_by_metadata", {}) + assert visible.db_column in metadata, "Visible group-by must appear in metadata." + assert hidden.db_column not in metadata, ( + "Hidden group-by must NOT appear in metadata." + ) diff --git a/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_group_by_data.py b/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_group_by_data.py index b13558015f..fe0bbc319e 100644 --- a/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_group_by_data.py +++ b/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_group_by_data.py @@ -2359,3 +2359,67 @@ def test_group_by_data_includes_multiple_select_display_values( assert group["display"][f"field_{field.id}"] == [ {"id": option.id, "value": "Red", "color": "red"} ] + + +@pytest.mark.django_db +def test_group_by_data_saved_group_bys_exclude_hidden_fields(api_client, data_fixture): + """ + When hidden_field_ids is non-None (enterprise restricted view), saved + group-bys on hidden fields must not appear in the group tree. + """ + + from unittest.mock import patch + + user, token = data_fixture.create_user_and_token() + table = data_fixture.create_database_table(user=user) + visible = data_fixture.create_text_field(table=table, name="Visible") + hidden = data_fixture.create_text_field(table=table, name="Hidden") + grid = data_fixture.create_grid_view(table=table) + data_fixture.create_view_group_by(view=grid, field=visible) + data_fixture.create_view_group_by(view=grid, field=hidden) + + model = table.get_model() + model.objects.create(**{f"field_{visible.id}": "A", f"field_{hidden.id}": "secret"}) + + url = reverse("api:database:views:grid:group-by-data", kwargs={"view_id": grid.id}) + with patch( + "baserow.contrib.database.api.views.grid.views.get_hidden_field_ids_for_view_user", + return_value={hidden.id}, + ): + response = api_client.get(url, HTTP_AUTHORIZATION=f"JWT {token}") + + assert response.status_code == HTTP_200_OK + page = _get_only_page(response) + group = page["groups"][0] + assert f"field_{visible.id}" in group["path"] + assert f"field_{hidden.id}" not in group["path"] + + +@pytest.mark.django_db +def test_public_group_by_data_saved_group_bys_exclude_hidden_fields( + api_client, data_fixture +): + """Public saved group-bys on hidden fields must not appear in the group tree.""" + + user = data_fixture.create_user() + table = data_fixture.create_database_table(user=user) + visible = data_fixture.create_text_field(table=table, name="Visible") + hidden = data_fixture.create_text_field(table=table, name="Hidden") + grid = data_fixture.create_grid_view(table=table, public=True) + data_fixture.create_view_group_by(view=grid, field=visible) + data_fixture.create_view_group_by(view=grid, field=hidden) + data_fixture.create_grid_view_field_option(grid, hidden, hidden=True) + + model = table.get_model() + model.objects.create(**{f"field_{visible.id}": "A", f"field_{hidden.id}": "secret"}) + + url = reverse( + "api:database:views:grid:public-group-by-data", kwargs={"slug": grid.slug} + ) + response = api_client.get(url) + + assert response.status_code == HTTP_200_OK + page = _get_only_page(response) + group = page["groups"][0] + assert f"field_{visible.id}" in group["path"] + assert f"field_{hidden.id}" not in group["path"] diff --git a/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_ordering_regressions.py b/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_ordering_regressions.py new file mode 100644 index 0000000000..f292eb570d --- /dev/null +++ b/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_ordering_regressions.py @@ -0,0 +1,341 @@ +from itertools import product + +from django.urls import reverse + +import pytest +from rest_framework.status import HTTP_200_OK + +from baserow.contrib.database.rows.handler import RowHandler +from baserow.contrib.database.views.handler import ViewHandler + +REQUEST_MODES = ["private_saved", "private_adhoc", "public_explicit"] +SELECTIONS = [(), (0,), (1,), (0, 1), (1, 0), (0, 2), (2, 0), (0, 1, 2)] + + +def _configure_ordering(data_fixture, view, groups, sorts, mode): + """Public grids send the displayed rules explicitly, including saved rules.""" + if mode != "private_adhoc": + for field, direction in groups: + data_fixture.create_view_group_by(view=view, field=field, order=direction) + for field, direction in sorts: + data_fixture.create_view_sort(view=view, field=field, order=direction) + + def serialize(rules): + return ",".join( + ("-" if direction == "DESC" else "") + field.db_column + for field, direction in rules + ) + + if mode == "private_saved": + return {} + return {"group_by": serialize(groups), "order_by": serialize(sorts)} + + +def _grid_urls_and_auth(view, token, mode): + if mode == "public_explicit": + return ( + reverse("api:database:views:grid:public_rows", kwargs={"slug": view.slug}), + reverse( + "api:database:views:grid:public-group-by-data", + kwargs={"slug": view.slug}, + ), + {}, + ) + return ( + reverse("api:database:views:grid:list", kwargs={"view_id": view.id}), + reverse("api:database:views:grid:group-by-data", kwargs={"view_id": view.id}), + {"HTTP_AUTHORIZATION": f"JWT {token}"}, + ) + + +def _get_json(api_client, url, params, auth): + response = api_client.get(url, params, **auth) + assert response.status_code == HTTP_200_OK, response.content + return response.json() + + +def _create_selection_matrix(data_fixture, user, table, field_kind="multiple_select"): + fields = [ + getattr(data_fixture, f"create_{field_kind}_field")( + table=table, name=f"Selections {index}" + ) + for index in range(2) + ] + if field_kind == "multiple_select": + options = [ + [ + data_fixture.create_select_option( + field=field, value=name, order=index + ).id + for index, name in enumerate("ABC") + ] + for field in fields + ] + else: + collaborators = [ + data_fixture.create_user( + workspace=table.database.workspace, first_name=name + ).id + for name in "ABC" + ] + options = [collaborators, collaborators] + model = table.get_model() + selections_by_row = {} + for selections in product(SELECTIONS, repeat=2): + # RowHandler preserves the user-selected order in the through table. A bare + # many-to-many .set() does not provide that fixture guarantee. + row = RowHandler().create_row( + user, + table, + values={ + field.db_column: [choices[index] for index in selected] + for field, choices, selected in zip(fields, options, selections) + }, + model=model, + ) + selections_by_row[row.id] = selections + return fields, model, selections_by_row + + +def _expected_selection_order(selections_by_row, groups, sorts, separator=","): + """Independent oracle: group by sets, sort by the ordered selection labels. + + Only ASCII A/B/C labels and option orders 0/1/2 are used, so the expected + ordering does not depend on locale. No production ordering hook, queryset + annotation, or previously sorted queryset supplies the expected result. + """ + result = list(selections_by_row) + streams = [("group", *rule) for rule in groups] + [ + ("sort", *rule) for rule in sorts + ] + for stream, field_index, direction in reversed(streams): + + def key(row_id): + selected = selections_by_row[row_id][field_index] + if stream == "group": + return tuple(sorted(selected)) + return separator.join("ABC"[index] for index in selected) + + # Stable passes retain row insertion order for equal keys and allow each + # stream to have its own direction. + result.sort(key=key, reverse=direction == "DESC") + return result + + +@pytest.mark.django_db +@pytest.mark.parametrize("mode", REQUEST_MODES) +@pytest.mark.parametrize("field_kind", ["multiple_select", "multiple_collaborators"]) +@pytest.mark.parametrize("direction", ["ASC", "DESC"]) +def test_m2m_group_and_sort_rows_match_group_offsets( + api_client, data_fixture, mode, field_kind, direction +): + """An unrelated M2M sort must not split the same logical group in two.""" + user, token = data_fixture.create_user_and_token() + table = data_fixture.create_database_table(user=user) + group_field = getattr(data_fixture, f"create_{field_kind}_field")(table=table) + sort_field = data_fixture.create_multiple_select_field(table=table) + included = data_fixture.create_boolean_field(table=table, name="Included") + if field_kind == "multiple_select": + group_ids = [ + data_fixture.create_select_option( + field=group_field, value=name, order=index + ).id + for index, name in enumerate("ABC") + ] + else: + group_ids = [ + data_fixture.create_user( + workspace=table.database.workspace, first_name=name + ).id + for name in "ABC" + ] + sort_ids = [ + data_fixture.create_select_option(field=sort_field, value=name, order=index).id + for index, name in enumerate("XY") + ] + view = data_fixture.create_grid_view(table=table, public=True) + data_fixture.create_view_filter( + view=view, field=included, type="boolean", value="true" + ) + params = _configure_ordering( + data_fixture, + view, + [(group_field, direction)], + [(sort_field, direction)], + mode, + ) + model = table.get_model() + rows = [ + RowHandler().create_row( + user, + table, + values={ + group_field.db_column: [group_ids[index] for index in group], + sort_field.db_column: [sort_ids[index] for index in sort], + included.db_column: True, + }, + model=model, + ) + for group, sort in [((0, 2), (0, 1)), ((0, 1), (0,)), ((2, 0), (1,))] + ] + # Both row and group endpoints must apply the saved view filter before + # calculating counts or offsets, including for ad-hoc and public requests. + excluded = RowHandler().create_row( + user, + table, + values={ + group_field.db_column: [group_ids[0]], + sort_field.db_column: sort_ids, + included.db_column: False, + }, + model=model, + ) + # AB sorts before AC. The AC and CA selections are one group, with XY + # preceding Y inside that group. Different sort cardinalities (2/1/1) expose + # join multiplication; using B instead of AB as the other group hides it. + expected_ids = [ + rows[index].id for index in ([1, 0, 2] if direction == "ASC" else [2, 0, 1]) + ] + ab, ac = [group_ids[0], group_ids[1]], [group_ids[0], group_ids[2]] + expected_groups = ( + [(ab, 1, 0), (ac, 2, 1)] if direction == "ASC" else [(ac, 2, 0), (ab, 1, 2)] + ) + rows_url, groups_url, auth = _grid_urls_and_auth(view, token, mode) + group_data = _get_json(api_client, groups_url, params, auth) + assert len(group_data["pages"]) == 1 + page = group_data["pages"][0] + assert page["group_count"] == 2 + assert [ + (group["path"][group_field.db_column], group["row_count"], group["row_offset"]) + for group in page["groups"] + ] == expected_groups + + response = _get_json( + api_client, rows_url, {**params, "include": "group_by_metadata"}, auth + ) + assert response["count"] == 3 + assert excluded.id not in [row["id"] for row in response["results"]] + assert [row["id"] for row in response["results"]] == expected_ids + assert sorted( + (entry[group_field.db_column], entry["count"]) + for entry in response["group_by_metadata"][group_field.db_column] + ) == [(ab, 1), (ac, 2)] + + # The virtual grid loads rows at these offsets. Matching group counts alone + # cannot catch rows disappearing when a group occupies nonadjacent positions. + for _, row_count, row_offset in expected_groups: + window = _get_json( + api_client, + rows_url, + {**params, "offset": row_offset, "limit": row_count}, + auth, + ) + assert window["count"] == 3 + assert [row["id"] for row in window["results"]] == expected_ids[ + row_offset : row_offset + row_count + ] + paginated_ids = [] + for offset in range(3): + window = _get_json( + api_client, rows_url, {**params, "offset": offset, "limit": 1}, auth + ) + paginated_ids.extend(row["id"] for row in window["results"]) + assert paginated_ids == expected_ids + + +@pytest.mark.django_db +@pytest.mark.parametrize("mode", REQUEST_MODES) +@pytest.mark.parametrize("direction", ["ASC", "DESC"]) +def test_grouping_same_m2m_field_preserves_selection_order_sort( + api_client, data_fixture, mode, direction +): + """Adding grouping must not reorder unchanged AB/BA and AC/CA sort keys.""" + user, token = data_fixture.create_user_and_token() + table = data_fixture.create_database_table(user=user) + fields, _, selections = _create_selection_matrix(data_fixture, user, table) + view = data_fixture.create_grid_view(table=table, public=True) + params = _configure_ordering( + data_fixture, view, [(fields[0], "ASC")], [(fields[0], direction)], mode + ) + rows_url, _, auth = _grid_urls_and_auth(view, token, mode) + + # The 64 real rows exercise empty selections, equal keys, reversed selections, + # and different cardinalities. A four-row AB/BA example can pass accidentally + # because an unordered aggregate happens to read its inputs in insertion order. + response = _get_json(api_client, rows_url, {**params, "size": 100}, auth) + assert response["count"] == 64 + assert [row["id"] for row in response["results"]] == _expected_selection_order( + selections, [(0, "ASC")], [(0, direction)] + ) + + +@pytest.mark.django_db +@pytest.mark.parametrize("persisted", [False, True], ids=["adhoc", "saved"]) +@pytest.mark.parametrize( + "groups,sorts", + [ + ([(0, "ASC"), (1, "DESC")], []), + ([], [(0, "DESC"), (1, "ASC")]), + ([(0, "ASC")], [(0, "DESC"), (1, "ASC")]), + ([(0, "DESC"), (1, "ASC")], [(0, "ASC"), (1, "DESC")]), + ], + ids=["two_groups", "two_sorts", "shared_field_and_second_sort", "both_streams"], +) +def test_multiple_m2m_ordering_streams_keep_independent_keys( + data_fixture, persisted, groups, sorts +): + user = data_fixture.create_user() + table = data_fixture.create_database_table(user=user) + fields, model, selections = _create_selection_matrix(data_fixture, user, table) + view = data_fixture.create_grid_view(table=table) + params = _configure_ordering( + data_fixture, + view, + [(fields[index], direction) for index, direction in groups], + [(fields[index], direction) for index, direction in sorts], + "private_saved" if persisted else "private_adhoc", + ) + queryset = model.objects.all() + if persisted: + queryset = ViewHandler().apply_ordering(view, queryset) + else: + queryset = queryset.order_by_fields_string( + params["order_by"], group_by_string=params["group_by"] + ) + + assert [row.id for row in queryset] == _expected_selection_order( + selections, groups, sorts + ) + + +@pytest.mark.django_db +@pytest.mark.parametrize("persisted", [False, True], ids=["adhoc", "saved"]) +@pytest.mark.parametrize("direction", ["ASC", "DESC"]) +def test_grouping_collaborators_preserves_selection_order_sort( + data_fixture, persisted, direction +): + user = data_fixture.create_user() + table = data_fixture.create_database_table(user=user) + fields, model, selections = _create_selection_matrix( + data_fixture, user, table, field_kind="multiple_collaborators" + ) + view = data_fixture.create_grid_view(table=table) + params = _configure_ordering( + data_fixture, + view, + [(fields[0], "ASC")], + [(fields[0], direction)], + "private_saved" if persisted else "private_adhoc", + ) + queryset = model.objects.all() + if persisted: + queryset = ViewHandler().apply_ordering(view, queryset) + else: + queryset = queryset.order_by_fields_string( + params["order_by"], group_by_string=params["group_by"] + ) + # Collaborator sorting concatenates names without the comma used by multiple + # select. Grouping still treats reversed selections as the same set. + assert [row.id for row in queryset] == _expected_selection_order( + selections, [(0, "ASC")], [(0, direction)], separator="" + ) diff --git a/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_views.py b/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_views.py index 82d08cae7f..ea52f8e03f 100644 --- a/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_views.py +++ b/backend/tests/baserow/contrib/database/api/views/grid/test_grid_view_views.py @@ -4115,7 +4115,7 @@ def test_list_rows_public_with_query_param_group_by(api_client, data_fixture): ) response_json = response.json() assert response.status_code == HTTP_400_BAD_REQUEST - assert response_json["error"] == "ERROR_ORDER_BY_FIELD_NOT_POSSIBLE" + assert response_json["error"] == "ERROR_VIEW_GROUP_BY_FIELD_NOT_SUPPORTED" @pytest.mark.django_db @@ -4263,7 +4263,7 @@ def test_list_rows_public_with_query_param_group_by_and_type(api_client, data_fi ) response_json = response.json() assert response.status_code == HTTP_400_BAD_REQUEST - assert response_json["error"] == "ERROR_ORDER_BY_FIELD_NOT_POSSIBLE" + assert response_json["error"] == "ERROR_VIEW_GROUP_BY_FIELD_NOT_SUPPORTED" response = api_client.get( f"{url}?group_by=field_{select_1.id}[order]", @@ -5626,3 +5626,197 @@ def count(self): assert response_json["results"][0][f"field_{text_field.id}"] == "0" assert response_json["results"][99][f"field_{text_field.id}"] == "99" assert count_calls == 0 # count is not called again + + +@pytest.mark.django_db +def test_list_rows_adhoc_order_by_with_group_by_multiple_select( + api_client, data_fixture +): + """ + When a user sends both ``order_by`` and ``group_by`` query parameters + (the adhoc sorting path used by Editors), the rows endpoint must use + ``get_group_by_sort_order`` for the group-by fields so that the row + ordering matches the group tree built by the group-by data endpoint. + + Regression test for the bug where ``order_by_fields_string`` always used + ``get_order`` (StringAgg, insertion-order dependent) instead of + ``get_group_by_sort_order`` (ArrayAgg, set-based) for multiple select + fields, causing group offsets to misalign. + """ + + user, token = data_fixture.create_user_and_token() + table = data_fixture.create_database_table(user=user) + field = FieldHandler().create_field( + user=user, + table=table, + name="Tags", + type_name="multiple_select", + ) + option_a = data_fixture.create_select_option(field=field, value="A", color="red") + option_b = data_fixture.create_select_option(field=field, value="B", color="blue") + option_c = data_fixture.create_select_option(field=field, value="C", color="green") + + # Row 1: select A then C (insertion order: A, C) + row1 = data_fixture.create_row_for_many_to_many_field( + table=table, field=field, values=[option_a.id, option_c.id], user=user + ) + # Row 2: select B only + row2 = data_fixture.create_row_for_many_to_many_field( + table=table, field=field, values=[option_b.id], user=user + ) + # Row 3: select C then A (insertion order: C, A — same set as row 1) + row3 = data_fixture.create_row_for_many_to_many_field( + table=table, field=field, values=[option_c.id, option_a.id], user=user + ) + + grid_view = data_fixture.create_grid_view(table=table, user=user) + url = reverse("api:database:views:grid:list", kwargs={"view_id": grid_view.id}) + + # Simulate the adhoc sorting path: group_by and order_by both reference the + # multiple select field. The group_by param tells the backend which fields + # are group-by fields so it can use set-based ordering. + response = api_client.get( + f"{url}?order_by=field_{field.id}&group_by=field_{field.id}", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + assert response.status_code == HTTP_200_OK + results = response.json()["results"] + assert len(results) == 3 + + # Rows 1 and 3 share the same set {A, C} and must be adjacent (contiguous + # group). With the old get_order (StringAgg), insertion order could + # separate them. + result_ids = [r["id"] for r in results] + idx1 = result_ids.index(row1.id) + idx3 = result_ids.index(row3.id) + assert abs(idx1 - idx3) == 1, ( + f"Rows with same option set {{A,C}} must be adjacent but were at " + f"positions {idx1} and {idx3}" + ) + + +@pytest.mark.django_db +def test_list_rows_public_adhoc_group_by_multiple_select_uses_set_ordering( + api_client, data_fixture +): + """ + Same as the private-view test above, but for the public rows endpoint + which has its own ``group_by`` handling in + ``get_public_rows_queryset_and_field_ids``. + """ + + user, token = data_fixture.create_user_and_token() + table = data_fixture.create_database_table(user=user) + field = FieldHandler().create_field( + user=user, + table=table, + name="Tags", + type_name="multiple_select", + ) + option_a = data_fixture.create_select_option(field=field, value="A", color="red") + option_b = data_fixture.create_select_option(field=field, value="B", color="blue") + option_c = data_fixture.create_select_option(field=field, value="C", color="green") + + row1 = data_fixture.create_row_for_many_to_many_field( + table=table, field=field, values=[option_a.id, option_c.id], user=user + ) + row2 = data_fixture.create_row_for_many_to_many_field( + table=table, field=field, values=[option_b.id], user=user + ) + row3 = data_fixture.create_row_for_many_to_many_field( + table=table, field=field, values=[option_c.id, option_a.id], user=user + ) + + grid_view = data_fixture.create_grid_view( + table=table, user=user, public=True, create_options=False + ) + data_fixture.create_grid_view_field_option(grid_view, field, hidden=False) + + url = reverse( + "api:database:views:grid:public_rows", kwargs={"slug": grid_view.slug} + ) + response = api_client.get( + f"{url}?group_by=field_{field.id}", + ) + assert response.status_code == HTTP_200_OK + results = response.json()["results"] + assert len(results) == 3 + + result_ids = [r["id"] for r in results] + idx1 = result_ids.index(row1.id) + idx3 = result_ids.index(row3.id) + assert abs(idx1 - idx3) == 1, ( + f"Rows with same option set {{A,C}} must be adjacent but were at " + f"positions {idx1} and {idx3}" + ) + + +@pytest.mark.django_db +def test_list_rows_group_by_only_without_order_by(api_client, data_fixture): + """ + After the frontend separates group_by from order_by, a request may send + only ``group_by`` without including those fields in ``order_by``. The + backend must still apply group ordering. + """ + + user, token = data_fixture.create_user_and_token() + table = data_fixture.create_database_table(user=user) + text_field = data_fixture.create_text_field(table=table, name="Label") + number_field = data_fixture.create_number_field( + table=table, name="Priority", number_decimal_places=0 + ) + + model = table.get_model() + row_b2 = model.objects.create( + **{f"field_{text_field.id}": "B", f"field_{number_field.id}": 2} + ) + row_a1 = model.objects.create( + **{f"field_{text_field.id}": "A", f"field_{number_field.id}": 1} + ) + row_a3 = model.objects.create( + **{f"field_{text_field.id}": "A", f"field_{number_field.id}": 3} + ) + row_b1 = model.objects.create( + **{f"field_{text_field.id}": "B", f"field_{number_field.id}": 1} + ) + + grid_view = data_fixture.create_grid_view(table=table, user=user) + url = reverse("api:database:views:grid:list", kwargs={"view_id": grid_view.id}) + + response = api_client.get( + f"{url}?group_by=field_{text_field.id}&order_by=field_{number_field.id}", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + assert response.status_code == HTTP_200_OK + results = response.json()["results"] + result_ids = [r["id"] for r in results] + assert result_ids == [row_a1.id, row_a3.id, row_b1.id, row_b2.id] + + +@pytest.mark.django_db +def test_list_rows_group_by_alone_applies_ordering(api_client, data_fixture): + """ + When only ``group_by`` is provided (no ``order_by``), rows must be ordered + by the group_by fields. + """ + + user, token = data_fixture.create_user_and_token() + table = data_fixture.create_database_table(user=user) + text_field = data_fixture.create_text_field(table=table, name="Category") + + model = table.get_model() + row_c = model.objects.create(**{f"field_{text_field.id}": "C"}) + row_a = model.objects.create(**{f"field_{text_field.id}": "A"}) + row_b = model.objects.create(**{f"field_{text_field.id}": "B"}) + + grid_view = data_fixture.create_grid_view(table=table, user=user) + url = reverse("api:database:views:grid:list", kwargs={"view_id": grid_view.id}) + + response = api_client.get( + f"{url}?group_by=field_{text_field.id}", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + assert response.status_code == HTTP_200_OK + results = response.json()["results"] + result_ids = [r["id"] for r in results] + assert result_ids == [row_a.id, row_b.id, row_c.id] diff --git a/backend/tests/baserow/contrib/database/field/test_created_by_field_type.py b/backend/tests/baserow/contrib/database/field/test_created_by_field_type.py index ef7e1cf006..90bf111fd8 100644 --- a/backend/tests/baserow/contrib/database/field/test_created_by_field_type.py +++ b/backend/tests/baserow/contrib/database/field/test_created_by_field_type.py @@ -490,14 +490,14 @@ def test_created_by_field_type_sorting(data_fixture): row5 = model.objects.create(created_by=None) sort = data_fixture.create_view_sort(view=grid_view, field=field, order="ASC") - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row5.id, row3.id, row2.id, row1.id, row4.id] sort.order = "DESC" sort.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row1.id, row4.id, row2.id, row3.id, row5.id] diff --git a/backend/tests/baserow/contrib/database/field/test_formula_field_type.py b/backend/tests/baserow/contrib/database/field/test_formula_field_type.py index f8e2a0adc4..de52b21f58 100644 --- a/backend/tests/baserow/contrib/database/field/test_formula_field_type.py +++ b/backend/tests/baserow/contrib/database/field/test_formula_field_type.py @@ -1631,7 +1631,7 @@ def create_primary_field(table): sort = data_fixture.create_view_sort( view=grid_view, field=formula_field, order="DESC" ) - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows ] @@ -1640,7 +1640,7 @@ def create_primary_field(table): sort.order = "ASC" sort.save() - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows ] @@ -1691,7 +1691,7 @@ def create_primary_field(table): sort = data_fixture.create_view_sort( view=grid_view, field=formula_field, order="DESC" ) - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows ] @@ -1700,7 +1700,7 @@ def create_primary_field(table): sort.order = "ASC" sort.save() - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows ] @@ -1761,7 +1761,7 @@ def create_primary_field(table): sort = data_fixture.create_view_sort( view=grid_view, field=formula_field, order="DESC" ) - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows ] @@ -1770,7 +1770,7 @@ def create_primary_field(table): sort.order = "ASC" sort.save() - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows ] @@ -1818,7 +1818,7 @@ def create_primary_field(table): sort = data_fixture.create_view_sort( view=grid_view, field=formula_field, order="DESC" ) - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows ] @@ -1827,7 +1827,7 @@ def create_primary_field(table): sort.order = "ASC" sort.save() - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows ] @@ -1922,7 +1922,7 @@ def test_formula_field_type_lookup_sorting_single_select( view=grid_view, field=formula_field, order="DESC" ) model = table.get_model() - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows @@ -1932,7 +1932,7 @@ def test_formula_field_type_lookup_sorting_single_select( sort.order = "ASC" sort.save() - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows ] @@ -2011,7 +2011,7 @@ def create_primary_field(table): sort = data_fixture.create_view_sort( view=grid_view, field=formula_field, order="DESC" ) - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows ] @@ -2020,7 +2020,7 @@ def create_primary_field(table): sort.order = "ASC" sort.save() - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows ] @@ -2099,7 +2099,7 @@ def create_primary_field(table): sort = data_fixture.create_view_sort( view=grid_view, field=formula_field, order="DESC" ) - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows ] @@ -2108,7 +2108,7 @@ def create_primary_field(table): sort.order = "ASC" sort.save() - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup = [ getattr(r, f"field_{formula_field.id}_agg_sort_array") for r in sorted_rows ] @@ -2158,21 +2158,21 @@ def test_formula_single_select_field_type_sorting(data_fixture): view=grid_view, field=formula_field, order="ASC" ) model = table.get_model() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_5.id, row_2.id, row_1.id, row_4.id, row_3.id] sort.order = "DESC" sort.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_3.id, row_1.id, row_4.id, row_2.id, row_5.id] sort.order = "ASC" sort.save() model = table.get_model() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_5.id, row_2.id, row_1.id, row_4.id, row_3.id] diff --git a/backend/tests/baserow/contrib/database/field/test_last_modified_by_field_type.py b/backend/tests/baserow/contrib/database/field/test_last_modified_by_field_type.py index 1f91ae2adb..48ff1e41fe 100644 --- a/backend/tests/baserow/contrib/database/field/test_last_modified_by_field_type.py +++ b/backend/tests/baserow/contrib/database/field/test_last_modified_by_field_type.py @@ -486,14 +486,14 @@ def test_last_modified_by_field_type_sorting(data_fixture): row5 = model.objects.create(last_modified_by=None) sort = data_fixture.create_view_sort(view=grid_view, field=field, order="ASC") - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row5.id, row3.id, row2.id, row1.id, row4.id] sort.order = "DESC" sort.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row1.id, row4.id, row2.id, row3.id, row5.id] diff --git a/backend/tests/baserow/contrib/database/field/test_lookup_field_type.py b/backend/tests/baserow/contrib/database/field/test_lookup_field_type.py index 66e5b4a2bf..19937410d5 100644 --- a/backend/tests/baserow/contrib/database/field/test_lookup_field_type.py +++ b/backend/tests/baserow/contrib/database/field/test_lookup_field_type.py @@ -2360,7 +2360,7 @@ def test_lookup_field_type_sorting_array_numbers( sort = data_fixture.create_view_sort( view=grid_view, field=lookup_field, order="DESC" ) - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup_numbers = [ getattr(r, f"field_{lookup_field.id}_agg_sort_array") for r in sorted_rows ] @@ -2369,7 +2369,7 @@ def test_lookup_field_type_sorting_array_numbers( sort.order = "ASC" sort.save() - sorted_rows = view_handler.apply_sorting(grid_view, model.objects.all()) + sorted_rows = view_handler.apply_ordering(grid_view, model.objects.all()) sorted_lookup_numbers = [ getattr(r, f"field_{lookup_field.id}_agg_sort_array") for r in sorted_rows ] diff --git a/backend/tests/baserow/contrib/database/field/test_multiple_collaborators_field_type.py b/backend/tests/baserow/contrib/database/field/test_multiple_collaborators_field_type.py index 00fef6bd2c..939113c86e 100644 --- a/backend/tests/baserow/contrib/database/field/test_multiple_collaborators_field_type.py +++ b/backend/tests/baserow/contrib/database/field/test_multiple_collaborators_field_type.py @@ -324,14 +324,14 @@ def test_multiple_collaborators_field_type_sorting( sort = data_fixture.create_view_sort(view=grid_view, field=field, order="ASC") model = table.get_model() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_5.id, row_1.id, row_4.id, row_3.id, row_2.id] sort.order = "DESC" sort.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_2.id, row_3.id, row_4.id, row_1.id, row_5.id] diff --git a/backend/tests/baserow/contrib/database/field/test_multiple_select_field_type.py b/backend/tests/baserow/contrib/database/field/test_multiple_select_field_type.py index c0e94a73e0..a3198cbd46 100644 --- a/backend/tests/baserow/contrib/database/field/test_multiple_select_field_type.py +++ b/backend/tests/baserow/contrib/database/field/test_multiple_select_field_type.py @@ -507,12 +507,12 @@ def test_multiple_select_field_type_sorting(data_fixture, django_assert_num_quer sort = data_fixture.create_view_sort(view=grid_view, field=field, order="ASC") model = table.get_model() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_4.id, row_3.id, row_2.id, row_1.id] sort.order = "DESC" sort.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_1.id, row_2.id, row_3.id, row_4.id] @@ -521,7 +521,7 @@ def test_multiple_select_field_type_sorting(data_fixture, django_assert_num_quer sort.order = "ASC" sort.save() model = table.get_model() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_4.id, row_2.id, row_1.id, row_3.id] @@ -2290,7 +2290,7 @@ def test_multiple_select_adjacent_row(data_fixture): ], ).created_rows - base_queryset = ViewHandler().apply_sorting( + base_queryset = ViewHandler().apply_ordering( grid_view, table.get_model().objects.all() ) diff --git a/backend/tests/baserow/contrib/database/field/test_single_select_field_type.py b/backend/tests/baserow/contrib/database/field/test_single_select_field_type.py index 6138706336..f8947cc5e2 100644 --- a/backend/tests/baserow/contrib/database/field/test_single_select_field_type.py +++ b/backend/tests/baserow/contrib/database/field/test_single_select_field_type.py @@ -777,13 +777,13 @@ def test_single_select_field_type_get_order(data_fixture): sort = data_fixture.create_view_sort(view=grid_view, field=field, order="ASC") model = table.get_model() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_5.id, row_2.id, row_1.id, row_4.id, row_3.id] sort.order = "DESC" sort.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_3.id, row_1.id, row_4.id, row_2.id, row_5.id] @@ -792,7 +792,7 @@ def test_single_select_field_type_get_order(data_fixture): sort.order = "ASC" sort.save() model = table.get_model() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_5.id, row_1.id, row_4.id, row_3.id, row_2.id] diff --git a/backend/tests/baserow/contrib/database/table/test_table_models.py b/backend/tests/baserow/contrib/database/table/test_table_models.py index 1d0c1b5fd6..465cddb960 100644 --- a/backend/tests/baserow/contrib/database/table/test_table_models.py +++ b/backend/tests/baserow/contrib/database/table/test_table_models.py @@ -1343,3 +1343,138 @@ def test_update_returning_ids_with_provably_empty_filter(data_fixture): **{name_field.db_column: "Falcon 1"} ) assert updated_row_ids == [] + + +@pytest.mark.django_db +def test_order_by_fields_string_without_group_by_string(data_fixture): + """ + When group_by_string is not provided (the default), order_by_fields_string + behaves identically to the pre-refactor version: all fields use get_order. + """ + + user = data_fixture.create_user() + table = data_fixture.create_database_table(user=user) + text_field = data_fixture.create_text_field(table=table, name="Name") + number_field = data_fixture.create_number_field( + table=table, name="Priority", number_decimal_places=0 + ) + + model = table.get_model() + row_b = model.objects.create( + **{f"field_{text_field.id}": "B", f"field_{number_field.id}": 2} + ) + row_a = model.objects.create( + **{f"field_{text_field.id}": "A", f"field_{number_field.id}": 1} + ) + row_c = model.objects.create( + **{f"field_{text_field.id}": "C", f"field_{number_field.id}": 3} + ) + + results = model.objects.all().order_by_fields_string(f"field_{text_field.id}") + assert [r.id for r in results] == [row_a.id, row_b.id, row_c.id] + + results = model.objects.all().order_by_fields_string( + f"field_{text_field.id}", group_by_string=None + ) + assert [r.id for r in results] == [row_a.id, row_b.id, row_c.id] + + +@pytest.mark.django_db +def test_order_by_fields_string_with_group_by_string_uses_group_by_sort_order( + data_fixture, +): + """ + When group_by_string is provided, those fields use get_group_by_sort_order + (set-based ArrayAgg for M2M) while order_string fields use get_order + (insertion-order StringAgg). This test verifies the dispatch by checking + that rows with the same set of options in a multiple_select field are + grouped adjacently when using group_by_string. + """ + + user = data_fixture.create_user() + table = data_fixture.create_database_table(user=user) + ms_field = FieldHandler().create_field( + user=user, table=table, name="Tags", type_name="multiple_select" + ) + option_a = data_fixture.create_select_option(field=ms_field, value="A", color="red") + option_b = data_fixture.create_select_option( + field=ms_field, value="B", color="blue" + ) + option_c = data_fixture.create_select_option( + field=ms_field, value="C", color="green" + ) + + # Row 1: A then C (insertion order: A, C) + row1 = data_fixture.create_row_for_many_to_many_field( + table=table, field=ms_field, values=[option_a.id, option_c.id], user=user + ) + # Row 2: B only + row2 = data_fixture.create_row_for_many_to_many_field( + table=table, field=ms_field, values=[option_b.id], user=user + ) + # Row 3: C then A (insertion order: C, A — same set as row 1) + row3 = data_fixture.create_row_for_many_to_many_field( + table=table, field=ms_field, values=[option_c.id, option_a.id], user=user + ) + + model = table.get_model() + + # With group_by_string: rows with same option set {A, C} must be adjacent + results = model.objects.all().order_by_fields_string( + "", group_by_string=f"field_{ms_field.id}" + ) + result_ids = [r.id for r in results] + assert result_ids == [row1.id, row3.id, row2.id], ( + f"Expected [{row1.id}, {row3.id}, {row2.id}] but got {result_ids}. " + f"Rows with same set {{A,C}} must precede {{B}} and tie-break by order/id." + ) + + # With group_by_string for grouping + a second M2M sort field: + # Exercises a second M2M join to verify isolated aggregates. + # Cardinalities: row1={X,Y}(2), row2={X}(1), row3={Y}(1) + ms_sort_field = FieldHandler().create_field( + user=user, table=table, name="Priority", type_name="multiple_select" + ) + option_x = data_fixture.create_select_option( + field=ms_sort_field, value="X", color="red" + ) + option_y = data_fixture.create_select_option( + field=ms_sort_field, value="Y", color="blue" + ) + + handler = RowHandler() + handler.update_row_by_id( + user, + table, + row1.id, + {f"field_{ms_sort_field.id}": [option_x.id, option_y.id]}, + ) + handler.update_row_by_id( + user, + table, + row2.id, + {f"field_{ms_sort_field.id}": [option_x.id]}, + ) + handler.update_row_by_id( + user, + table, + row3.id, + {f"field_{ms_sort_field.id}": [option_y.id]}, + ) + + model = table.get_model() + results = model.objects.all().order_by_fields_string( + f"field_{ms_sort_field.id}", + group_by_string=f"field_{ms_field.id}", + ) + result_ids = [r.id for r in results] + assert len(result_ids) == 3, ( + f"Expected 3 rows but got {len(result_ids)}: {result_ids}" + ) + + # Group {A,C} (row1, row3) before {B} (row2) in ASC. + # Within {A,C}: row1 sort={X,Y} before row3 sort={Y} in ASC. + assert result_ids == [row1.id, row3.id, row2.id], ( + f"Expected group-first then sort ordering [row1, row3, row2] " + f"but got {result_ids}" + ) diff --git a/backend/tests/baserow/contrib/database/trash/test_database_trash_types.py b/backend/tests/baserow/contrib/database/trash/test_database_trash_types.py index 489c0b6a79..91dde2975c 100644 --- a/backend/tests/baserow/contrib/database/trash/test_database_trash_types.py +++ b/backend/tests/baserow/contrib/database/trash/test_database_trash_types.py @@ -862,7 +862,7 @@ def test_trashing_a_field_with_a_sort_trashes_the_sort( ) model = customers_table.get_model() - filtered_qs = ViewHandler().apply_sorting(grid_view, model.objects.all()) + filtered_qs = ViewHandler().apply_ordering(grid_view, model.objects.all()) assert list( filtered_qs.values_list(f"field_{customers_primary_field.id}", flat=True) ) == ["1", "2"] @@ -874,7 +874,7 @@ def test_trashing_a_field_with_a_sort_trashes_the_sort( ) model = customers_table.get_model() - filtered_qs = ViewHandler().apply_sorting(grid_view, model.objects.all()) + filtered_qs = ViewHandler().apply_ordering(grid_view, model.objects.all()) assert list( filtered_qs.values_list(f"field_{customers_primary_field.id}", flat=True) ) == ["2", "1"] diff --git a/backend/tests/baserow/contrib/database/view/test_view_handler.py b/backend/tests/baserow/contrib/database/view/test_view_handler.py index 85798a080f..24e8ab6a9c 100755 --- a/backend/tests/baserow/contrib/database/view/test_view_handler.py +++ b/backend/tests/baserow/contrib/database/view/test_view_handler.py @@ -1271,7 +1271,7 @@ def test_delete_filter(send_mock, data_fixture): @pytest.mark.django_db -def test_apply_sortings(data_fixture): +def test_apply_orderings(data_fixture): user = data_fixture.create_user() table = data_fixture.create_database_table(user=user) text_field = data_fixture.create_text_field(table=table) @@ -1326,7 +1326,7 @@ def test_apply_sortings(data_fixture): ) # Without any sortings. - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_1.id, row_2.id, row_3.id, row_4.id, row_5.id, row_6.id] @@ -1334,34 +1334,34 @@ def test_apply_sortings(data_fixture): # Should raise a value error if the modal doesn't have the _field_objects property. with pytest.raises(ValueError): - view_handler.apply_sorting(grid_view, GridView.objects.all()) + view_handler.apply_ordering(grid_view, GridView.objects.all()) # Should raise a value error if the field is not included in the model. with pytest.raises(ValueError): - view_handler.apply_sorting( + view_handler.apply_ordering( grid_view, table.get_model(field_ids=[]).objects.all() ) - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_1.id, row_2.id, row_3.id, row_4.id, row_5.id, row_6.id] sort.order = "DESC" sort.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_6.id, row_5.id, row_4.id, row_1.id, row_2.id, row_3.id] sort.order = "ASC" sort.field_id = number_field.id sort.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_3.id, row_2.id, row_1.id, row_6.id, row_5.id, row_4.id] sort.field_id = boolean_field.id sort.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_3.id, row_4.id, row_5.id, row_1.id, row_2.id, row_6.id] @@ -1370,7 +1370,7 @@ def test_apply_sortings(data_fixture): sort_2 = data_fixture.create_view_sort( view=grid_view, field=number_field, order="ASC" ) - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_3.id, row_2.id, row_1.id, row_4.id, row_5.id, row_6.id] @@ -1379,7 +1379,7 @@ def test_apply_sortings(data_fixture): sort_2.field_id = boolean_field sort_2.order = "DESC" sort_2.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_1.id, row_2.id, row_3.id, row_4.id, row_5.id, row_6.id] @@ -1389,13 +1389,13 @@ def test_apply_sortings(data_fixture): sort_2.field_id = boolean_field sort_2.order = "ASC" sort_2.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_6.id, row_5.id, row_4.id, row_3.id, row_1.id, row_2.id] sort.field_id = number_field.id sort.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_4.id, row_5.id, row_6.id, row_1.id, row_2.id, row_3.id] @@ -1410,7 +1410,7 @@ def test_apply_sortings(data_fixture): sort.delete() sort_2.delete() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [ row_7.id, @@ -2282,14 +2282,14 @@ def test_cant_get_view_filter_when_view_trashed(data_fixture): @pytest.mark.django_db -def test_cant_apply_sorting_when_view_trashed(data_fixture): +def test_cant_apply_ordering_when_view_trashed(data_fixture): user = data_fixture.create_user() grid_view = data_fixture.create_grid_view(user=user) ViewHandler().delete_view(user, grid_view) with pytest.raises(ViewSortDoesNotExist): - ViewHandler().apply_sorting( + ViewHandler().apply_ordering( grid_view, grid_view.table.get_model().objects.all(), ) @@ -3660,7 +3660,7 @@ def test_delete_group_by(send_mock, data_fixture): @pytest.mark.django_db -def test_apply_sortings_sorts_by_group_bys(data_fixture): +def test_apply_orderings_sorts_by_group_bys(data_fixture): user = data_fixture.create_user() table = data_fixture.create_database_table(user=user) text_field = data_fixture.create_text_field(table=table) @@ -3715,7 +3715,7 @@ def test_apply_sortings_sorts_by_group_bys(data_fixture): ) # Without any groupbys. - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_1.id, row_2.id, row_3.id, row_4.id, row_5.id, row_6.id] @@ -3725,34 +3725,34 @@ def test_apply_sortings_sorts_by_group_bys(data_fixture): # Should raise a value error if the modal doesn't have the _field_objects property. with pytest.raises(ValueError): - view_handler.apply_sorting(grid_view, GridView.objects.all()) + view_handler.apply_ordering(grid_view, GridView.objects.all()) # Should raise a value error if the field is not included in the model. with pytest.raises(ValueError): - view_handler.apply_sorting( + view_handler.apply_ordering( grid_view, table.get_model(field_ids=[]).objects.all() ) - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_1.id, row_2.id, row_3.id, row_4.id, row_5.id, row_6.id] group_by.order = "DESC" group_by.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_6.id, row_5.id, row_4.id, row_1.id, row_2.id, row_3.id] group_by.order = "ASC" group_by.field_id = number_field.id group_by.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_3.id, row_2.id, row_1.id, row_6.id, row_5.id, row_4.id] group_by.field_id = boolean_field.id group_by.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_3.id, row_4.id, row_5.id, row_1.id, row_2.id, row_6.id] @@ -3761,7 +3761,7 @@ def test_apply_sortings_sorts_by_group_bys(data_fixture): sort_2 = data_fixture.create_view_group_by( view=grid_view, field=number_field, order="ASC" ) - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_3.id, row_2.id, row_1.id, row_4.id, row_5.id, row_6.id] @@ -3770,7 +3770,7 @@ def test_apply_sortings_sorts_by_group_bys(data_fixture): sort_2.field_id = boolean_field sort_2.order = "DESC" sort_2.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_1.id, row_2.id, row_3.id, row_4.id, row_5.id, row_6.id] @@ -3780,13 +3780,13 @@ def test_apply_sortings_sorts_by_group_bys(data_fixture): sort_2.field_id = boolean_field sort_2.order = "ASC" sort_2.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_6.id, row_5.id, row_4.id, row_3.id, row_1.id, row_2.id] group_by.field_id = number_field.id group_by.save() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_4.id, row_5.id, row_6.id, row_1.id, row_2.id, row_3.id] @@ -3801,7 +3801,7 @@ def test_apply_sortings_sorts_by_group_bys(data_fixture): group_by.delete() sort_2.delete() - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [ row_7.id, @@ -3815,7 +3815,7 @@ def test_apply_sortings_sorts_by_group_bys(data_fixture): @pytest.mark.django_db -def test_apply_sortings_applies_group_bys_first_then_view_sorts(data_fixture): +def test_apply_orderings_applies_group_bys_first_then_view_sorts(data_fixture): user = data_fixture.create_user() table = data_fixture.create_database_table(user=user) text_field = data_fixture.create_text_field(table=table) @@ -3876,7 +3876,7 @@ def test_apply_sortings_applies_group_bys_first_then_view_sorts(data_fixture): view=grid_view, field=boolean_field, order="ASC" ) - rows = view_handler.apply_sorting(grid_view, model.objects.all()) + rows = view_handler.apply_ordering(grid_view, model.objects.all()) row_ids = [row.id for row in rows] assert row_ids == [row_3.id, row_1.id, row_2.id, row_4.id, row_5.id, row_6.id] diff --git a/changelog/entries/unreleased/bug/5937_fixed_blank_rows_appearing_in_grouped_grid_views_for_editor.json b/changelog/entries/unreleased/bug/5937_fixed_blank_rows_appearing_in_grouped_grid_views_for_editor.json new file mode 100644 index 0000000000..1dcdcb29ee --- /dev/null +++ b/changelog/entries/unreleased/bug/5937_fixed_blank_rows_appearing_in_grouped_grid_views_for_editor.json @@ -0,0 +1,9 @@ +{ + "type": "bug", + "message": "Fixed blank rows appearing in grouped grid views for Editor-role users when grouping by a multiple select or multiple collaborators field. Separated group-by from order-by in frontend API calls so each uses the correct sort semantics.", + "issue_origin": "github", + "issue_number": 5937, + "domain": "database", + "bullet_points": [], + "created_at": "2026-08-20" +} diff --git a/enterprise/backend/src/baserow_enterprise/data_sync/baserow_table_data_sync.py b/enterprise/backend/src/baserow_enterprise/data_sync/baserow_table_data_sync.py index a79c36290d..0867e05c05 100644 --- a/enterprise/backend/src/baserow_enterprise/data_sync/baserow_table_data_sync.py +++ b/enterprise/backend/src/baserow_enterprise/data_sync/baserow_table_data_sync.py @@ -339,7 +339,7 @@ def get_all_rows( # filters. if view: queryset = ViewHandler().apply_filters(view, queryset) - queryset = ViewHandler().apply_sorting(view, queryset) + queryset = ViewHandler().apply_ordering(view, queryset) progress.increment(by=1) # makes the total `1` rows_queryset = queryset.values(*["id"] + enabled_property_field_ids) diff --git a/premium/backend/src/baserow_premium/api/views/views.py b/premium/backend/src/baserow_premium/api/views/views.py index 1e668ca866..ee04ca31e1 100644 --- a/premium/backend/src/baserow_premium/api/views/views.py +++ b/premium/backend/src/baserow_premium/api/views/views.py @@ -29,6 +29,7 @@ ERROR_VIEW_DOES_NOT_EXIST, ERROR_VIEW_FILTER_TYPE_DOES_NOT_EXIST, ERROR_VIEW_FILTER_TYPE_UNSUPPORTED_FIELD, + ERROR_VIEW_GROUP_BY_FIELD_NOT_SUPPORTED, ) from baserow.contrib.database.api.views.serializers import ViewSerializer from baserow.contrib.database.api.views.utils import get_public_view_authorization_token @@ -46,6 +47,7 @@ ViewDoesNotExist, ViewFilterTypeDoesNotExist, ViewFilterTypeNotAllowedForField, + ViewGroupByFieldNotSupported, ) from baserow.contrib.database.views.handler import ViewHandler from baserow.contrib.database.views.registries import view_type_registry @@ -169,6 +171,7 @@ class ExportPublicViewView(APIView): "ERROR_VIEW_FILTER_TYPE_UNSUPPORTED_FIELD", "ERROR_ORDER_BY_FIELD_NOT_FOUND", "ERROR_ORDER_BY_FIELD_NOT_POSSIBLE", + "ERROR_VIEW_GROUP_BY_FIELD_NOT_SUPPORTED", ] ), 404: get_error_schema(["ERROR_VIEW_DOES_NOT_EXIST"]), @@ -185,6 +188,7 @@ class ExportPublicViewView(APIView): ViewFilterTypeNotAllowedForField: ERROR_VIEW_FILTER_TYPE_UNSUPPORTED_FIELD, OrderByFieldNotFound: ERROR_ORDER_BY_FIELD_NOT_FOUND, OrderByFieldNotPossible: ERROR_ORDER_BY_FIELD_NOT_POSSIBLE, + ViewGroupByFieldNotSupported: ERROR_VIEW_GROUP_BY_FIELD_NOT_SUPPORTED, } ) def post(self, request, slug): @@ -202,7 +206,7 @@ def post(self, request, slug): # Delete the provided view ID because it can be identified using the slug # path parameter. - del option_data["view_id"] + option_data.pop("view_id", None) job = ExportHandler.create_and_start_new_job(None, table, view, option_data) serialized_job = ExportJobSerializer(job).data diff --git a/premium/backend/src/baserow_premium/export/exporter_types.py b/premium/backend/src/baserow_premium/export/exporter_types.py index e99a5aba33..63e95cd556 100644 --- a/premium/backend/src/baserow_premium/export/exporter_types.py +++ b/premium/backend/src/baserow_premium/export/exporter_types.py @@ -43,7 +43,7 @@ def before_job_create(self, user, table, view, export_options): class JSONQuerysetSerializer(QuerysetSerializer): can_handle_rich_value = True - def write_to_file(self, file_writer: FileWriter, export_charset="utf-8"): + def write_to_file(self, file_writer: FileWriter, export_charset="utf-8", **kwargs): """ Writes the queryset to the provided file in json format. Will generate semi-structured json based on the fields in the queryset. @@ -103,7 +103,7 @@ def file_extension(self) -> str: class XMLQuerysetSerializer(QuerysetSerializer): can_handle_rich_value = True - def write_to_file(self, file_writer: FileWriter, export_charset="utf-8"): + def write_to_file(self, file_writer: FileWriter, export_charset="utf-8", **kwargs): """ Writes the queryset to the provided file in xml format. Will generate semi-structured xml based on the fields in the queryset. Each separate row in @@ -183,6 +183,7 @@ def write_to_file( file_writer: FileWriter, export_charset: Optional[str] = None, excel_include_header: bool = False, + **kwargs, ): """ :param file_writer: The FileWriter instance to write to. @@ -257,6 +258,7 @@ def write_to_file( file_writer: FileWriter, export_charset: str = "utf-8", organize_files: bool = True, + **kwargs, ): """ Writes files from the queryset to a zip archive. Will create a directory diff --git a/premium/backend/src/baserow_premium/views/handler.py b/premium/backend/src/baserow_premium/views/handler.py index 8c8b949f07..93f3c06b6a 100644 --- a/premium/backend/src/baserow_premium/views/handler.py +++ b/premium/backend/src/baserow_premium/views/handler.py @@ -94,7 +94,7 @@ def get_rows_grouped_by_single_select_field( base_queryset = model.objects.all().enhance_by_fields() if apply_view_sorts: - base_queryset = ViewHandler().apply_sorting(view, base_queryset) + base_queryset = ViewHandler().apply_ordering(view, base_queryset) if adhoc_filters is None: adhoc_filters = AdHocFilters() diff --git a/premium/backend/tests/baserow_premium_tests/api/views/views/test_public_export_group_by_contract.py b/premium/backend/tests/baserow_premium_tests/api/views/views/test_public_export_group_by_contract.py new file mode 100644 index 0000000000..b7d77788a8 --- /dev/null +++ b/premium/backend/tests/baserow_premium_tests/api/views/views/test_public_export_group_by_contract.py @@ -0,0 +1,46 @@ +"""Public export must preserve whether an ad-hoc grouping option was supplied.""" + +from django.test.utils import override_settings +from django.urls import reverse + +import pytest +from rest_framework.status import HTTP_200_OK + +from baserow.contrib.database.export.models import ExportJob +from baserow_premium.api.views.signers import export_public_view_signer + + +@pytest.mark.django_db +@override_settings(DEBUG=True) +@pytest.mark.parametrize("group_mode", ["omitted", "clear", "replace"]) +def test_public_export_job_preserves_group_by_presence( + premium_data_fixture, api_client, group_mode +): + table = premium_data_fixture.create_database_table() + field = premium_data_fixture.create_text_field(table=table, primary=True) + view = premium_data_fixture.create_grid_view( + table=table, public=True, allow_public_export=True + ) + premium_data_fixture.create_view_group_by(view=view, field=field, order="ASC") + payload = {"exporter_type": "csv"} + if group_mode != "omitted": + payload["group_by"] = "" if group_mode == "clear" else f"-{field.db_column}" + + response = api_client.post( + reverse("api:premium:view:export_public_view", kwargs={"slug": view.slug}), + payload, + format="json", + ) + + assert response.status_code == HTTP_200_OK, response.json() + job = ExportJob.objects.get( + id=export_public_view_signer.loads(response.json()["id"]) + ) + assert job.view_id == view.id + if group_mode == "omitted": + assert "group_by" not in job.export_options, ( + "An ordinary public export must not gain group_by=None in its persisted " + f"worker payload: {job.export_options}" + ) + else: + assert job.export_options["group_by"] == payload["group_by"] diff --git a/premium/web-frontend/modules/baserow_premium/components/views/PublicViewExportMenuItem.vue b/premium/web-frontend/modules/baserow_premium/components/views/PublicViewExportMenuItem.vue index 53062b9943..2ee951257c 100644 --- a/premium/web-frontend/modules/baserow_premium/components/views/PublicViewExportMenuItem.vue +++ b/premium/web-frontend/modules/baserow_premium/components/views/PublicViewExportMenuItem.vue @@ -25,6 +25,7 @@ import PublicViewExportService from '@baserow_premium/services/publicViewExport' import { createFiltersTree, getOrderBy, + serializeGroupBys, } from '@baserow/modules/database/utils/view' export default { @@ -81,6 +82,11 @@ export default { const orderBy = getOrderBy(this.view, true) values.order_by = orderBy + const viewType = this.$registry.get('view', this.view.type) + if (viewType.canGroupBy) { + values.group_by = serializeGroupBys(this.view) + } + values.fields = this.visibleOrderedFields === null ? null diff --git a/premium/web-frontend/test/unit/premium/view/publicViewExportMenuItem.spec.js b/premium/web-frontend/test/unit/premium/view/publicViewExportMenuItem.spec.js new file mode 100644 index 0000000000..bf51fd563b --- /dev/null +++ b/premium/web-frontend/test/unit/premium/view/publicViewExportMenuItem.spec.js @@ -0,0 +1,151 @@ +import { h } from 'vue' +import flushPromises from 'flush-promises' +import PublicViewExportMenuItem from '@baserow_premium/components/views/PublicViewExportMenuItem.vue' +import { PremiumTestApp } from '@baserow_premium_test/helpers/premiumTestApp' + +const gridCases = ['saved', 'clear', 'replace'].flatMap((groupMode) => + ['clear', 'replace'].map((sortMode) => ({ groupMode, sortMode })) +) +const expectedGroup = { saved: 'field_1', clear: '', replace: '-field_3' } +const expectedSort = { clear: '', replace: '-field_4' } + +describe('Public view export preserves effective grouping', () => { + let testApp = null + let store = null + + beforeEach(() => { + testApp = new PremiumTestApp() + store = testApp.store + }) + + afterEach(async () => { + await testApp.afterEach() + }) + + const prepareView = async (type = 'grid') => { + const { view } = await store.dispatch('view/forceCreate', { + data: { + id: 1, + type, + slug: 'shared-view', + allow_public_export: true, + table: { id: 1, database_id: 1 }, + ownership_type: 'collaborative', + filter_type: 'AND', + filters: [], + filter_groups: [], + group_bys: + type === 'grid' + ? [{ id: 10, view: 1, field: 1, order: 'ASC', type: 'default' }] + : [], + sortings: [ + { id: 20, view: 1, field: 2, order: 'ASC', type: 'default' }, + ], + }, + }) + await store.dispatch('page/view/public/setIsPublic', true) + return view + } + + const exportView = async (view) => { + const client = testApp.getApp().$client + const url = `/database/view/${view.slug}/export-public-view/` + testApp.mock.onPost(url).reply(200, { id: 1 }) + const wrapper = await testApp.mount(PublicViewExportMenuItem, { + props: { + database: { id: 1 }, + table: { id: 1 }, + fields: [], + storePrefix: 'page/', + view, + }, + global: { + stubs: { + // Only the generic modal UI is replaced. The mounted menu item's + // callback, view registry, serializers, service and Axios are real. + ExportTableModal: { + props: ['startExport', 'view'], + render() { + return h( + 'button', + { + type: 'button', + onClick: () => + this.startExport({ + view: this.view, + values: { exporter_type: 'csv', view_id: this.view.id }, + client, + }), + }, + 'Export CSV' + ) + }, + }, + }, + }, + }) + await wrapper.get('button').trigger('click') + await flushPromises() + + expect(testApp.mock.history.post).toHaveLength(1) + const request = testApp.mock.history.post[0] + expect(request.url).toBe(url) + const values = JSON.parse(request.data) + expect(values.exporter_type).toBe('csv') + expect(values).not.toHaveProperty('view_id') + return values + } + + test.each(gridCases)( + 'grid export after grouping=$groupMode and sorting=$sortMode', + async ({ groupMode, sortMode }) => { + const view = await prepareView() + if (groupMode === 'clear') { + await store.dispatch('view/deleteGroupBy', { + view, + groupBy: view.group_bys[0], + readOnly: true, + }) + expect(view.group_bys).toEqual([]) + } else if (groupMode === 'replace') { + await store.dispatch('view/updateGroupBy', { + groupBy: view.group_bys[0], + values: { field: 3, order: 'DESC' }, + readOnly: true, + }) + } + + if (sortMode === 'clear') { + await store.dispatch('view/deleteSort', { + view, + sort: view.sortings[0], + readOnly: true, + }) + } else { + await store.dispatch('view/updateSort', { + sort: view.sortings[0], + values: { field: 4, order: 'DESC' }, + readOnly: true, + }) + } + + expect(testApp.mock.history.delete).toHaveLength(0) + expect(testApp.mock.history.patch).toHaveLength(0) + const values = await exportView(view) + expect(values.order_by).toBe(expectedSort[sortMode]) + // An absent key means inherit saved groups. Empty means the visitor + // removed all groups; those are different requests even for exports. + expect(values).toHaveProperty('group_by', expectedGroup[groupMode]) + } + ) + + test.each(['gallery', 'kanban', 'calendar'])( + '%s export omits group_by because the view cannot group rows', + async (type) => { + const view = await prepareView(type) + const values = await exportView(view) + expect(values.order_by).toBe('field_2') + expect(values).not.toHaveProperty('group_by') + } + ) +}) diff --git a/web-frontend/modules/database/services/view/grid.js b/web-frontend/modules/database/services/view/grid.js index 7e99946745..44462f448d 100644 --- a/web-frontend/modules/database/services/view/grid.js +++ b/web-frontend/modules/database/services/view/grid.js @@ -17,7 +17,7 @@ export default (client) => { searchMode = '', publicUrl = false, publicAuthToken = null, - groupBy = '', + groupBy = null, orderBy = null, filters = {}, includeFields = [], @@ -57,7 +57,7 @@ export default (client) => { } } - if (groupBy) { + if (groupBy || groupBy === '') { params.append('group_by', groupBy) } @@ -156,14 +156,14 @@ export default (client) => { includeDescendants = false, descendantLimit = null, descendantRowBudget = null, - groupBy = '', + groupBy = null, aggregationsOnly = false, includeTotals = false, }) { const params = new URLSearchParams() params.append('offset', offset) params.append('limit', limit) - if (groupBy) { + if (groupBy || groupBy === '') { params.append('group_by', groupBy) } if (includeDescendants) { diff --git a/web-frontend/modules/database/utils/view.js b/web-frontend/modules/database/utils/view.js index 38433166c5..43b8bd4d38 100644 --- a/web-frontend/modules/database/utils/view.js +++ b/web-frontend/modules/database/utils/view.js @@ -514,22 +514,31 @@ export function newFieldMatchesActiveSearchTerm( return false } +// Safe to call unconditionally: only GridViewType has can_group_by=True, +// so view.group_bys is always empty for gallery/kanban/calendar views. +export function serializeGroupBys(view) { + if (!view || !view.group_bys || view.group_bys.length === 0) { + return '' + } + return view.group_bys + .map((groupBy) => { + let serialized = `${groupBy.order === 'DESC' ? '-' : ''}field_${ + groupBy.field + }` + if (groupBy.type !== DEFAULT_SORT_TYPE_KEY) { + serialized += `[${groupBy.type}]` + } + return serialized + }) + .join(',') +} + export function getGroupBy(rootGetters, viewId, adhocGroupBy = false) { if (rootGetters['page/view/public/getIsPublic'] || adhocGroupBy) { const view = rootGetters['view/get'](viewId) - return view.group_bys - .map((groupBy) => { - let serialized = `${groupBy.order === 'DESC' ? '-' : ''}field_${ - groupBy.field - }` - if (groupBy.type !== DEFAULT_SORT_TYPE_KEY) { - serialized += `[${groupBy.type}]` - } - return serialized - }) - .join(',') + return serializeGroupBys(view) } else { - return '' + return null } } @@ -624,11 +633,8 @@ export function getOrderBy(view, adhocSorting) { } return serialized } - // Group bys first, then sorts to ensure that the order is correct. - const groupBys = view.group_bys ? view.group_bys.map(serializeSort) : [] const sorts = view.sortings.map(serializeSort) - - return [...groupBys, ...sorts].join(',') + return sorts.length > 0 ? sorts.join(',') : '' } else { return null } diff --git a/web-frontend/test/unit/database/services/view/gridOrdering.spec.js b/web-frontend/test/unit/database/services/view/gridOrdering.spec.js new file mode 100644 index 0000000000..3ab4d5d33c --- /dev/null +++ b/web-frontend/test/unit/database/services/view/gridOrdering.spec.js @@ -0,0 +1,182 @@ +import GridService from '@baserow/modules/database/services/view/grid' +import { getGroupBy, getOrderBy } from '@baserow/modules/database/utils/view' +import { TestApp } from '@baserow/test/helpers/testApp' + +const modes = ['inherit', 'clear', 'replace'] +const requestCases = modes.flatMap((groupMode) => + modes.map((sortMode) => ({ publicView: false, groupMode, sortMode })) +) +// Public views always send the visitor's effective configuration. +requestCases.push( + ...modes + .slice(1) + .flatMap((groupMode) => + modes + .slice(1) + .map((sortMode) => ({ publicView: true, groupMode, sortMode })) + ) +) + +const expectedGroup = { inherit: null, clear: '', replace: '-field_3' } +const expectedSort = { inherit: null, clear: '', replace: '-field_4' } + +// Parse the serialized query string, rather than inspecting helper return values. +const expectOverride = (params, key, expected) => { + expect({ [key]: params.getAll(key) }).toEqual({ + [key]: expected === null ? [] : [expected], + }) +} + +describe('Grid sorting and grouping request contract', () => { + let testApp = null + let store = null + + beforeEach(() => { + testApp = new TestApp() + store = testApp.store + }) + + afterEach(async () => { + await testApp.afterEach() + }) + + const prepareView = async ({ + publicView, + groupMode, + sortMode = 'inherit', + }) => { + const { view } = await store.dispatch('view/forceCreate', { + data: { + id: 1, + type: 'grid', + table: { id: 1, database_id: 1 }, + ownership_type: 'collaborative', + group_bys: [ + { id: 10, view: 1, field: 1, order: 'ASC', type: 'default' }, + ], + sortings: [ + { id: 20, view: 1, field: 2, order: 'ASC', type: 'default' }, + ], + }, + }) + await store.dispatch('page/view/public/setIsPublic', publicView) + + // These are the same local mutations used by an Editor or public visitor; + // removing the final item must not silently restore the saved server config. + if (groupMode === 'clear') { + await store.dispatch('view/deleteGroupBy', { + view, + groupBy: view.group_bys[0], + readOnly: true, + }) + expect(view.group_bys).toEqual([]) + } else if (groupMode === 'replace') { + await store.dispatch('view/updateGroupBy', { + groupBy: view.group_bys[0], + values: { field: 3, order: 'DESC' }, + readOnly: true, + }) + } + + if (sortMode === 'clear') { + await store.dispatch('view/deleteSort', { + view, + sort: view.sortings[0], + readOnly: true, + }) + expect(view.sortings).toEqual([]) + } else if (sortMode === 'replace') { + await store.dispatch('view/updateSort', { + sort: view.sortings[0], + values: { field: 4, order: 'DESC' }, + readOnly: true, + }) + } + + expect(testApp.mock.history.delete).toHaveLength(0) + expect(testApp.mock.history.patch).toHaveLength(0) + return view + } + + test.each(requestCases)( + 'rows: public=$publicView, grouping=$groupMode, sorting=$sortMode', + async ({ publicView, groupMode, sortMode }) => { + const view = await prepareView({ publicView, groupMode, sortMode }) + const gridId = publicView ? 'shared-grid' : view.id + const url = `/database/views/grid/${gridId}/${publicView ? 'public/rows/' : ''}` + testApp.mock.onGet(url).reply(200, { results: [], count: 0 }) + + await GridService(testApp.getApp().$client).fetchRows({ + gridId, + publicUrl: publicView, + groupBy: getGroupBy( + store.getters, + view.id, + !publicView && groupMode !== 'inherit' + ), + orderBy: getOrderBy(view, sortMode !== 'inherit'), + }) + + expect(testApp.mock.history.get).toHaveLength(1) + const request = testApp.mock.history.get[0] + expect(request.url).toBe(url) + const params = new URLSearchParams(request.params.toString()) + expectOverride(params, 'order_by', expectedSort[sortMode]) + expectOverride(params, 'group_by', expectedGroup[groupMode]) + } + ) + + test.each([ + { publicView: false, groupMode: 'inherit' }, + { publicView: false, groupMode: 'clear' }, + { publicView: false, groupMode: 'replace' }, + { publicView: true, groupMode: 'clear' }, + { publicView: true, groupMode: 'replace' }, + ])( + 'group metadata: public=$publicView, grouping=$groupMode', + async ({ publicView, groupMode }) => { + const view = await prepareView({ publicView, groupMode }) + const gridId = publicView ? 'shared-grid' : view.id + const url = `/database/views/grid/${gridId}/${publicView ? 'public/' : ''}group-by-data/` + testApp.mock.onGet(url).reply(200, { results: [] }) + + await GridService(testApp.getApp().$client).fetchGroupByData({ + gridId, + publicUrl: publicView, + groupBy: getGroupBy( + store.getters, + view.id, + !publicView && groupMode !== 'inherit' + ), + }) + + expect(testApp.mock.history.get).toHaveLength(1) + const params = new URLSearchParams( + testApp.mock.history.get[0].params.toString() + ) + expectOverride(params, 'group_by', expectedGroup[groupMode]) + } + ) + + test.each([ + { method: 'fetchRows', publicView: false }, + { method: 'fetchRows', publicView: true }, + { method: 'fetchGroupByData', publicView: false }, + { method: 'fetchGroupByData', publicView: true }, + ])( + '$method without overrides inherits server settings (public=$publicView)', + async ({ method, publicView }) => { + testApp.mock.onGet().reply(200, { results: [] }) + await GridService(testApp.getApp().$client)[method]({ + gridId: publicView ? 'shared-grid' : 1, + publicUrl: publicView, + }) + expect(testApp.mock.history.get).toHaveLength(1) + const params = new URLSearchParams( + testApp.mock.history.get[0].params.toString() + ) + expectOverride(params, 'group_by', null) + expectOverride(params, 'order_by', null) + } + ) +}) diff --git a/web-frontend/test/unit/database/utils/view.spec.js b/web-frontend/test/unit/database/utils/view.spec.js index 01e9daaeb7..0561261577 100644 --- a/web-frontend/test/unit/database/utils/view.spec.js +++ b/web-frontend/test/unit/database/utils/view.spec.js @@ -2,7 +2,9 @@ import { TreeGroupNode, canRowsBeOptimisticallyUpdatedInView, createFiltersTree, + getOrderBy, matchSearchFilters, + serializeGroupBys, } from '@baserow/modules/database/utils/view' import { TestApp } from '@baserow/test/helpers/testApp' import _ from 'lodash' @@ -326,3 +328,61 @@ describe('canRowsBeOptimisticallyUpdatedInView', () => { ).toBe(false) }) }) + +describe('getOrderBy', () => { + it('returns null when adhocSorting is false', () => { + const view = { sortings: [{ field: 1, order: 'ASC', type: 'default' }] } + expect(getOrderBy(view, false)).toBeNull() + }) + + it('returns only sortings, not group_bys', () => { + const view = { + sortings: [{ field: 2, order: 'ASC', type: 'default' }], + group_bys: [{ field: 1, order: 'DESC', type: 'default' }], + } + expect(getOrderBy(view, true)).toBe('field_2') + }) + + it('returns empty string when no sortings exist', () => { + const view = { + sortings: [], + group_bys: [{ field: 1, order: 'ASC', type: 'default' }], + } + expect(getOrderBy(view, true)).toBe('') + }) + + it('serializes DESC and non-default sort type', () => { + const view = { + sortings: [ + { field: 3, order: 'DESC', type: 'default' }, + { field: 4, order: 'ASC', type: 'numeric' }, + ], + group_bys: [], + } + expect(getOrderBy(view, true)).toBe('-field_3,field_4[numeric]') + }) +}) + +describe('serializeGroupBys', () => { + it('returns empty string when no group_bys', () => { + expect(serializeGroupBys({ group_bys: [] })).toBe('') + expect(serializeGroupBys({})).toBe('') + }) + + it('serializes group_bys correctly', () => { + const view = { + group_bys: [ + { field: 1, order: 'ASC', type: 'default' }, + { field: 2, order: 'DESC', type: 'default' }, + ], + } + expect(serializeGroupBys(view)).toBe('field_1,-field_2') + }) + + it('includes non-default sort type', () => { + const view = { + group_bys: [{ field: 5, order: 'ASC', type: 'numeric' }], + } + expect(serializeGroupBys(view)).toBe('field_5[numeric]') + }) +})