#24327 closed defect (fixed)

Sort results under technical details by timestamp and, if necessary, by fingerprint

Reported by: karsten Owned by: metrics-team
Priority: Medium Milestone:
Component: Metrics/ExoneraTor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

A while ago, in 92cbc07, we took out ORDER BY statements, because we didn't need them anymore in order to produce correct output. However, this also made the technical details part less readable with entries appearing in random order. Let's fix this by sorting results first by timestamp and then by fingerprint.

Child Tickets

Change History (8)

comment:1 Changed 21 months ago by karsten

Status: newneeds_review

Please review one commit in my task-24327 branch.

comment:2 Changed 21 months ago by iwakeh

Status: needs_reviewneeds_revision

As the ordering is simply for sorting the matches for returning them, it should rather be given as special comparator argument to the Collections.sort method than defining it as the Comparator of the Match class. Defining it as the Comparator with the Match class somewhat indicates that this is a natural ordering, which ought be consistent with equals, which is not the case and not intended here. (Of course, from the data point of view there shouldn't be matches not differing in the two fields used for comparison and differing in other fields of the Match class, but that is not always guaranteed.)

Better to limit the changes to the one place where the sorting is done:

--- a/src/main/java/org/torproject/exonerator/QueryServlet.java
+++ b/src/main/java/org/torproject/exonerator/QueryServlet.java
@@ -18,6 +18,7 @@ import java.text.ParseException;
 import java.text.SimpleDateFormat;
 import java.util.ArrayList;
 import java.util.Calendar;
+import java.util.Collections;
 import java.util.List;
 import java.util.SortedSet;
 import java.util.TimeZone;
@@ -338,6 +339,15 @@ public class QueryServlet extends HttpServlet {
         }
       }
       if (!matches.isEmpty()) {
+        Collections.sort(matches,
+            (m1, m2) -> {
+            if (m1 == m2) {
+              return 0;
+            } else if (!m1.timestamp.equals(m2.timestamp)) {
+              return m1.timestamp.compareTo(m2.timestamp);
+            } else {
+              return m1.fingerprint.compareTo(m2.fingerprint);
+            }});
         response.matches = matches.toArray(new QueryResponse.Match[0]);

comment:3 Changed 21 months ago by iwakeh

Is there a particular reason to implement sorting in Java now instead of having the db query sort the results?

comment:4 Changed 21 months ago by iwakeh

And, a some tests for this would be great :-)

comment:5 Changed 21 months ago by karsten

Status: needs_revisionneeds_review

Moving the sorting code to QueryServlet sounds good to me. Good point about also having to implement equals() and so on, that's not really what we want here. Changed in a subsequent commit in the same branch.

Regarding your question about the reason for moving sorting from the database to Java, I can't give you a definite answer. We changed a few things when combining multiple database queries into one. The new code did not require ordered query results in order to produce correct output, and it still does not. We took out the ORDER BY statements, because that was easier than replacing numbers with names. We just overlooked that this would affect the order of entries in the technical results. Now, we could put the sorting back to the database, but then we'd have to find a way to use column names rather than numbers, and I didn't find an easy way to do that with all the UNIONs. I'd say it's simply easier to sort things in the servlet.

I also agree that tests would be great. I even spent some time thinking about testing this in QueryResponseTest until realizing that it should rather be tested in a new QueryServletTest. But writing useful tests for QueryServlet is not a trivial task and shouldn't delay merging this bugfix.

Please take another look at task-24327 branch. If this looks okay to you, I'll squash and merge tomorrow.

comment:6 Changed 21 months ago by iwakeh

Status: needs_reviewmerge_ready

Looks ok, tests and checks pass, except that checkstyle wants the following change in order to not complain:

--- a/src/main/java/org/torproject/metrics/exonerator/QueryServlet.java
+++ b/src/main/java/org/torproject/metrics/exonerator/QueryServlet.java
@@ -347,7 +347,8 @@ public class QueryServlet extends HttpServlet {
                 return m1.timestamp.compareTo(m2.timestamp);
               } else {
                 return m1.fingerprint.compareTo(m2.fingerprint);
-              }});
+              }
+            });

Ready to merge w/o the checkstyle complaint.

comment:7 in reply to:  5 ; Changed 21 months ago by iwakeh

Replying to karsten:

Moving the sorting code to QueryServlet sounds good to me. Good point about also having to implement equals() and so on, that's not really what we want here. Changed in a subsequent commit in the same branch.

Fine.

Regarding your question about the reason for moving sorting from the database to Java, I can't give you a definite answer. We changed a few things when combining multiple database queries into one. The new code did not require ordered query results in order to produce correct output, and it still does not. We took out the ORDER BY statements, because that was easier than replacing numbers with names. We just overlooked that this would affect the order of entries in the technical results. Now, we could put the sorting back to the database, but then we'd have to find a way to use column names rather than numbers, and I didn't find an easy way to do that with all the UNIONs. I'd say it's simply easier to sort things in the servlet.

Ah, ok. Thanks for pointing this all out.

I also agree that tests would be great. I even spent some time thinking about testing this in QueryResponseTest until realizing that it should rather be tested in a new QueryServletTest. But writing useful tests for QueryServlet is not a trivial task and shouldn't delay merging this bugfix.

I think this situation should be changed. Creating new ticket #24365 for this task.

comment:8 in reply to:  7 Changed 21 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

Merged and deployed! Thanks for creating #24365. Closing.

Note: See TracTickets for help on using tickets.