Opened 18 months ago

Closed 17 months ago

Last modified 17 months ago

#24580 closed enhancement (fixed)

Let ExoneraTor only serve completed dates

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

Description

While investigating ExoneraTor downtimes (#24534) we noticed that a large number of requests are being made for the current date. It seems that somebody is using ExoneraTor as some sort of exit list interface.

That's not what ExoneraTor is designed for, and it's not what it's good at. We also cannot guarantee that ExoneraTor provides correct results, because the import may well be behind a few hours, which typically doesn't matter.

We should reject requests for the current day and for the day before.

I'll attach a possible patch shortly.

Regarding translations, let's finalize the text in English and then wait a few weeks for Transifex translators to provide translations. It should be sufficient to verify them using Google Translate.

Child Tickets

Change History (11)

comment:1 Changed 18 months ago by karsten

Status: newneeds_review

comment:2 Changed 18 months ago by iwakeh

Reviewer: iwakeh

Added to review list.

comment:3 Changed 18 months ago by iwakeh

As there are only dates used in the servlet the calculations should also only use date time objects. In addition, the calculation shouldn't rely on string comparison when actually comparing dates.

For example:

  • src/main/java/org/torproject/metrics/exonerator/ExoneraTorServlet.java

    diff --git a/src/main/java/org/torproject/metrics/exonerator/ExoneraTorServlet.java b/src/main/java/org/torproject/metrics/exonerator/ExoneraTorServlet.java
    index c26d08e..beea0d2 100644
    a b import java.io.StringWriter; 
    1515import java.net.URL;
    1616import java.text.ParseException;
    1717import java.text.SimpleDateFormat;
     18import java.time.LocalDate;
    1819import java.time.ZoneOffset;
    19 import java.time.ZonedDateTime;
     20import java.time.format.DateTimeFormatter;
    2021import java.util.ArrayList;
    2122import java.util.Arrays;
    2223import java.util.List;
    public class ExoneraTorServlet extends HttpServlet { 
    321322   * it matches the day before the current system date (in UTC) or is even
    322323   * younger. */
    323324  static boolean checkTimestampTooRecent(String timestampParameter) {
    324     return timestampParameter.compareTo(ZonedDateTime.now(ZoneOffset.UTC)
    325         .toLocalDate().minusDays(1).toString()) >= 0;
     325    return LocalDate.parse(timestampParameter, DateTimeFormatter.ISO_LOCAL_DATE)
     326        .isAfter(LocalDate.now(ZoneOffset.UTC).minusDays(2));
    326327  }
    327328
    328329  /* Helper method for fetching a query response via URL. */

Actually, this would be a perfect time to introduce java 8 date time classes throughout this servlet; avoiding the mixture of parsing methods and most likely reducing the number of lines (' checkTimestampTooRecent' could actually be avoided with the use of isAfter etc.), and also making the code also more readable.

comment:4 Changed 18 months ago by karsten

Sounds good to me. I haven't had the time to read up on all Java 8 date/time things yet, so I might not pick the best classes. Happy to switch everything else in this class (ideally in a separate commit).

I'd say whoever start working on this leaves a quick note here. :)

comment:5 Changed 17 months ago by iwakeh

Owner: changed from metrics-team to iwakeh
Status: needs_reviewaccepted

I grab this. It will be good to create an example here for applying Java 8 features (related to date and time) to existing code bases.

comment:6 Changed 17 months ago by iwakeh

Status: acceptedneeds_revision

Please review four commits on this branch.

I tried to encapsulate all date related requests in a special ExoneraTorDate class and also separated the tests and extended them a little. The goal is to make the servlets decision making obvious from the code and have the decision making about the date parameter in one place. Please check that the existing logic didn't get changed in the process. (Maybe, the tests could be altered to catch the things that you find?)

The final commit tweaks the error message a little to tell users when to return.

(The very same approach of encapsulation could also be used for the ip parameter later.)

comment:7 Changed 17 months ago by iwakeh

Status: needs_revisionneeds_review

comment:8 Changed 17 months ago by karsten

Status: needs_reviewneeds_revision

Looks good. Please find my updated task-24580 branch with fixup commits. Are these good to go in, squashed into yours?

There's also a placeholder commit pointing out a minor (related) issue I found while testing. I'm not entirely sure what the best fix would be there. Do you want to give that a try?

comment:9 Changed 17 months ago by iwakeh

Status: needs_revisionneeds_review

The other changes are fine.
Please find an additional commit on this branch for review. I think this does what the TODO described.

comment:10 Changed 17 months ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Looks good! Merged to master. Deploying soon. Thanks! Closing.

comment:11 Changed 17 months ago by karsten

(Deployed!)

Note: See TracTickets for help on using tickets.