Opened 2 years ago

Closed 2 years ago

#22279 closed enhancement (implemented)

simplify and avoid repetition in ParserHelper methods

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

Description

Simplify and avoid repetition in ParserHelper methods and tests in ExtraInfoDescriptorTest accordingly. Cf. #22217 comments 6 an 7. This includes a minor code clean-up and raising exceptions when duplicate keys are encountered.

Child Tickets

Change History (19)

comment:1 Changed 2 years ago by iwakeh

Milestone: metrics-lib 1.8.0

Good candidate for 1.8.0.

comment:2 Changed 2 years ago by iwakeh

Keywords: metrics-help removed

comment:3 Changed 2 years ago by karsten

Owner: changed from metrics-team to karsten
Status: newassigned

I'll give this a try.

comment:4 Changed 2 years ago by karsten

Status: assignedneeds_review

Please review my task-22279 branch for a first try in this direction.

comment:5 Changed 2 years ago by iwakeh

I'm reviewing and changing this and would prefer to base all that on the changes in #19607.

comment:6 Changed 2 years ago by karsten

Please find my branch task-22279-2 which is the same patch as before but rebased to current master.

comment:7 Changed 2 years ago by iwakeh

I only added a RuntimeException in a code part that according to its comment shouldn't be reached:

--- a/src/main/java/org/torproject/descriptor/impl/ParseHelper.java
+++ b/src/main/java/org/torproject/descriptor/impl/ParseHelper.java
@@ -333,6 +333,8 @@ public class ParseHelper {
           new String[]{ validatedString }, 0, 0, ",", Integer.class, result);
     } catch (DescriptorParseException e) {
-      /* Should have caught in earlier validation step. */
+      throw new RuntimeException("Should have been caught in earlier validation "
+          + "step, but wasn't. ", e);
     }
     return result;
   }

and made the tests fail:

...
   [junit] Testcase: testDirreqStatsValid took 0.009 sec
    [junit] 	Caused an ERROR
    [junit] Should have caught in earlier validation step, but wasn't. 
    [junit] java.lang.RuntimeException: Should have been caught in earlier validation step, but wasn't. 
    [junit] 	at org.torproject.descriptor.impl.ParseHelper.convertCommaSeparatedKeyIntegerValueList(ParseHelper.java:336)
    [junit] 	at org.torproject.descriptor.impl.ExtraInfoDescriptorImpl.getDirreqV2Ips(ExtraInfoDescriptorImpl.java:1005)
    [junit] 	at org.torproject.descriptor.impl.ExtraInfoDescriptorImplTest.testDirreqStatsValid(ExtraInfoDescriptorImplTest.java:1307)
    [junit] Caused by: org.torproject.descriptor.DescriptorParseException: Line '' contains an illegal key in list element ''.
    [junit] 	at org.torproject.descriptor.impl.ParseHelper.parseStringKeyTValueList(ParseHelper.java:436)
    [junit] 	at org.torproject.descriptor.impl.ParseHelper.convertCommaSeparatedKeyIntegerValueList(ParseHelper.java:332)
    [junit] 
...

comment:8 Changed 2 years ago by iwakeh

Status: needs_reviewneeds_revision

comment:9 Changed 2 years ago by karsten

Status: needs_revisionneeds_review

Oops. Please look again now.

comment:10 Changed 2 years ago by iwakeh

Hmm, when looking at the commit I have the following question:
Could/should a valid validatedString really be empty or is this just a testing tweak?

comment:11 Changed 2 years ago by karsten

We'll have to represent the case of an empty key-value list somehow. For example "dirreq-v2-reqs " contains zero key-value elements, which we want to represent somehow. And it's different from an omitted dirreq-v2-reqs line. I mean, we could imagine doing this differently in the future, but I tried to leave the current behavior unchanged for now. What did you have in mind here?

comment:12 in reply to:  11 Changed 2 years ago by iwakeh

Replying to karsten:

We'll have to represent the case of an empty key-value list somehow. For example "dirreq-v2-reqs " contains zero key-value elements, which we want to represent somehow. And it's different from an omitted dirreq-v2-reqs line. I mean, we could imagine doing this differently in the future, but I tried to leave the current behavior unchanged for now. What did you have in mind here?

Ah, that makes sense. I just wanted to know a real data example for this code change. Thanks for providing it!

Please find some more tweaks (two commits) for review. The first commit is also intending to remove (intended) side-effects like adding to maps given as method arguments and make the code a little more explicit.
Thus, easier to comprehend, I hope.
The second commit removes a parameter that is not used anymore.

comment:13 Changed 2 years ago by karsten

Resolution: implemented
Status: needs_reviewclosed

Looks great! Squashed all four commits together and pushed to master. Closing. Thanks! :)

comment:14 Changed 2 years ago by karsten

Resolution: implemented
Status: closedreopened

Looks like we overlooked the case when a descriptor does not contain a line, e.g., dirreq-v3-reqs. In this case, ExtraInfoDescriptorImpl#dirreqV3Reqs will stay null, and ParseHelper#convertCommaSeparatedKeyIntegerValueList() will not get a validated string from us but null. Please find this commit in my task-22279-3 branch with a test and a fix.

comment:15 Changed 2 years ago by karsten

Status: reopenedneeds_review

comment:16 Changed 2 years ago by iwakeh

<deleted a comment that was meant for a different ticket>

comment:17 Changed 2 years ago by karsten

Still needs review, and needs to go into 1.8.0 which we're planning to release tomorrow.

comment:18 Changed 2 years ago by iwakeh

Status: needs_reviewmerge_ready

Test and fix look fine; all tests and checks pass. => ready for merge&release.

comment:19 Changed 2 years ago by karsten

Resolution: implemented
Status: merge_readyclosed

Great, thanks for checking! Merged and re-closing.

Note: See TracTickets for help on using tickets.