Abp: Can sort by fields which are not defined in DTO

[Trojaner]

You can order by fields in CrudAppService even if they are not defined in the DTO. I am not sure if this is meant to be like that.

Related code:

https://github.com/abpframework/abp/blob/0a1568781df03fe9c86dc38cb7d47b95389cdf0b/framework/src/Volo.Abp.Ddd.Application/Volo/Abp/Application/Services/CrudAppService.cs#L188

[acjh]

Why not?

[Trojaner]

I don't think you should be able to do operations with non exposed members of the entity.

Firstly it could be abused to check if columns exist in the database table, since it returns 500 if that's not the case.

Also there may be other potential unwanted situations where you may not want to expose how entities are ordered by a specific column.

In short it seems like this breaks encapsulation.

It's debatable if any of these are relevant at all but I think reporting this and discussing it would be a good idea.

[olicooper]

I have also experienced 500 errors when the column doesn't exist or it is a computed column etc. To fix the issue I overrode the ApplySorting method as you have and applied custom filtering for columns. I agree this isn't ideal though.

A partial solution could be a property attribute applied to DTOs such as [Sortable]. Then ignore any filters that aren't in the 'sortable' list.
This solution still wouldn't be flexible enough in my case as I also need to ensure sorted computed columns and enums work too.

Its not an easy fix I think.

[hikalkan]

Normally, exception occurs only if someone tries to hack/abuse the system by sorting, because UI will normally not send a non-existing column name. However, it doesn't hurt the application or database even they send another column name. I will think on that but that may not be an easy problem to overcome. Maybe we add a list of "allowed sort columns" to allow you to restrict the sortable columns, however it will be tedious to write and maintain the list for you.

[gdlcf88]

It is dangerous when a vistor is lucky enough to try a exsiting sensitive column name likes IsStatic for sorting. But, sensitive columns are usually less than common columns, so how about to add [Unsortable]?

[hikalkan]

Unsortable to DTO? How repository layer know about DTO properties? If you have an idea, can you write your full implementation suggestion?

[gdlcf88]

Maybe [Unsortable] to entity, but domain layer should not care about sorting, this is really a problem.

[gdlcf88]

namespace Volo.Abp.Application.Dtos
{
  public interface ISortedResultRequest : IValidatableObject
  {
    IEnumerable<string> UnsortableColumns { get; }

    string Sorting { get; set; }
  }
}

namespace Volo.Abp.Application.Dtos
{
    [Serializable]
    public class PagedAndSortedResultRequestDto : PagedResultRequestDto, IPagedAndSortedResultRequest
    {
        protected virtual IEnumerable<string> UnsortableColumns { get; }

        public virtual string Sorting { get; set; }

        public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
        {
            if (Sorting == null)
            {
                yield break;
            }

            var columns = Sorting.Trim().Split(',').ToList();

            foreach (var columnName in from column in columns
                let i = column.IndexOf(' ')
                select i == -1 ? column : column.Substring(0, i).TrimStart()
                into columnName
                from unsortableColumn in UnsortableColumns
                where unsortableColumn.Equals(columnName, StringComparison.OrdinalIgnoreCase)
                select columnName)
            {
                yield return new ValidationResult("Unsortable column inputted.", new[] {columnName});
            }
        }
    }
}

How about this idea? @hikalkan

[hikalkan]

Creating such an UnsortableColumns property is not a practical solution. Most of the developers will not even implement it or will find it tedious (what about if you later add a sensitive column to the database and forget to add this list). Also, this couples the DTO to db field names.

One solution could be relating ProductDto to the ProductListRequestDto, so we ca know ProductListRequestDto is used to request ProductDto objects. Then we can check ProductDto properties and their attributes to validate the sorting. In that way, you can use [Sortable] or [Unsortable] attributes over the result DTO class.

Relating ProductDto to the ProductListRequestDto can be automatic. Validation interceptor already knows the input and return types of the validating method. Getting this directly from the method also has an advantage that you don't couple ProductListRequestDto to ProductDto, so you can reuse it any other method.

[gdlcf88]

That's a good idea. Usually I will not add a sensitive property to ProductDto, so [Sortable] looks more reasonable. But this solution brings a breaking change because developers should add this attribute manually now for sorting.

[hikalkan]

We can make it backward compatible. For example, we can add a [ValidateSorting] to the ProductListRequestDto. If you don't add it, everything works as before. If you add it, it indicates that only the properties of this DTO are allowed for sorting. If you want, you can add [Unsortable] to any property to exclude from sorting.
Even more, [ValidateSorting] can have a parameter to set the default behaviour used like [ValidateSorting(SortingValidationBehaviour.AllowAll)] (then you add Unsortable to desired properties) and [ValidateSorting(SortingValidationBehaviour.RestrictAll)] (then you add Sortable to desired properties).

[gdlcf88]

then you add Unsortable to desired properties

What if the property does not in ProductDto?

[hikalkan]

What if the property does not in ProductDto?

The original problem was to restrict sorting on fields not in the return DTO 😄

Actually, I don't see the point of sorting by a field not included in the result.

[gdlcf88]

If you add it, it indicates that only the properties of this DTO are allowed for sorting.

I understand this premise now. 😃

[hikalkan]

We can add more properties to the SortingValidationBehaviour enum to cover other scenarios too.

[hikalkan]

I am taking this to backlog and will prioritize later. Any PR welcome :)