simplify and avoid repetition in ParserHelper methods

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

changed milestone to %metrics-lib 1.8.0

added component::metrics/library milestone::metrics-lib 1.8.0 owner::karsten priority::medium resolution::implemented severity::normal status::closed type::enhancement labels

Good candidate for 1.8.0.

Trac:
Milestone: N/A to metrics-lib 1.8.0

Trac:
Keywords: metrics-help deleted, N/A added

I'll give this a try.

Trac:
Owner: metrics-team to karsten
Status: new to assigned

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

Trac:
Status: assigned to needs_review

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

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

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] 
...

Trac:
Status: needs_review to needs_revision

Oops. Please look again now.

Trac:
Status: needs_revision to needs_review

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?

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?

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.

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

Trac:
Status: needs_review to closed
Resolution: N/A to implemented

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.

Trac:
Resolution: implemented to N/A
Status: closed to reopened

Trac:
Status: reopened to needs_review

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

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

Trac:
Status: needs_review to merge_ready