Copilot commented on code in PR #836:
URL:
https://github.com/apache/rocketmq-dashboard/pull/836#discussion_r3702771830
##########
server/src/main/java/org/apache/rocketmq/studio/ops/audit/InMemoryAuditRepository.java:
##########
@@ -46,12 +47,11 @@ public List<AuditRecordVO> findAll(String search, String
operationType,
.filter(r -> startDate == null || r.getTimestamp() != null &&
!r.getTimestamp().isBefore(startDate))
.filter(r -> endDate == null || r.getTimestamp() != null &&
!r.getTimestamp().isAfter(endDate))
.filter(r -> result == null || result.isEmpty() ||
result.equals(r.getResult()))
- .sorted((a, b) -> {
- if (a.getTimestamp() == null || b.getTimestamp() == null) {
- return 0;
- }
- return b.getTimestamp().compareTo(a.getTimestamp());
- })
+ .sorted(Comparator
+ .comparing(AuditRecordVO::getTimestamp,
+
Comparator.nullsLast(Comparator.reverseOrder()))
+ .thenComparing(AuditRecordVO::getId,
+
Comparator.nullsLast(Comparator.naturalOrder())))
Review Comment:
This comparator is now non-trivial and may be reused or need adjustment
later. Consider extracting it into a `private static final
Comparator<AuditRecordVO>` (or a small named method) to make the ordering rules
easier to read, test, and modify independently.
##########
server/src/test/java/org/apache/rocketmq/studio/ops/audit/InMemoryAuditRepositoryTest.java:
##########
@@ -69,6 +69,33 @@ void findAllShouldTreatBlankSearchAsNoSearchFilter() {
.containsExactly("record-blank-search");
}
+ @Test
+ void findAllShouldOrderTimestampedRecordsBeforeMissingTimestamps() {
+ AuditRecordVO noTimestamp = AuditRecordVO.builder()
+ .operationType("CREATE")
+ .result("SUCCESS")
+ .build();
+ noTimestamp.setId("record-no-timestamp");
+ AuditRecordVO newest = AuditRecordVO.builder()
+ .timestamp(LocalDateTime.of(2026, 8, 1, 10, 0))
+ .operationType("UPDATE")
+ .result("SUCCESS")
+ .build();
+ newest.setId("record-newest");
+ AuditRecordVO older = AuditRecordVO.builder()
+ .timestamp(LocalDateTime.of(2026, 8, 1, 9, 0))
+ .operationType("DELETE")
+ .result("SUCCESS")
+ .build();
+ older.setId("record-older");
+
+ putRecords(noTimestamp, older, newest);
+
+ assertThat(repository.findAll(null, null, null, null, null))
+ .extracting(AuditRecordVO::getId)
+ .containsExactly("record-newest", "record-older",
"record-no-timestamp");
Review Comment:
The PR description mentions adding an `id` tie-breaker for deterministic
ordering, but this test only covers mixed null/non-null timestamps. Please add
a test case where two records share the same timestamp (and possibly another
where both timestamps are null) to assert the `id` ordering is applied as
intended.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
