search

spring-projects / spring-data-envers

Envers extension of the Spring Data JPA module
http://projects.spring.io/spring-data-envers
Apache License 2.0
264 stars 101 forks source link

Missing Pageable Sort interpretation #379

Closed NiklasLoechte closed 1 year ago

NiklasLoechte commented 1 year ago

Currently you cannot sort by property only the sort direction is interpretated as the revision number.

https://github.com/spring-projects/spring-data-envers/blob/2196536b4b4aa1927656903bbd9633a30e8df4c2/src/main/java/org/springframework/data/envers/repository/support/EnversRevisionRepositoryImpl.java#L147

schauder commented 1 year ago

While it doesn't mean it has to stay this way, this seems to be intentional and somewhat in accordance to the relevant JavaDoc on RevisionRepository

Returns a Page of revisions for the entity with the given id. Note, that it's not guaranteed that implementations have to support sorting by all properties.

I guess the original idea was that sorting revisions by anything but timeline is of limited use.

Could you explain your use case of sorting by something else but the revision number?

NiklasLoechte commented 1 year ago

Thank you very much for your answer. We have a distributed service that performs update operations for entities and updates a modified timestamp each time. We have encountered a problem when we use a cached sequence with an increment of 50 (to increase performance for large amounts of data). When multiple instances make updates to an entity, the temporal order is not guaranteed when using such a cache, so sorting by the modified timestamp of the revisioned entities seemed to be straight forward. For now, overriding the current implementation of the repository with something like that did the job.

    var orderMapped = pageable.getSort().stream().findFirst().map(order ->
            order.getDirection().isAscending() ?
                AuditEntity.property(order.getProperty()).asc() :
                AuditEntity.property(order.getProperty()).desc())
        .orElse(AuditEntity.revisionNumber().asc());

But perhapse this can be adapted to enhance this great project. If you think that this would be a benefit let me know and I could provide a pull request. Thank you in advance.

schauder commented 1 year ago

Yes, that does make sense and sounds like an improvement. A PR would be appreciated.