cloud icon indicating copy to clipboard operation
cloud copied to clipboard

Fix `DurationParser` allowing invalid input

Open emilyy-dev opened this issue 1 year ago • 4 comments

DurationParser using that regex and Matcher#find() only finds the pattern in the given input string, while the pattern is right for individual segments denoting value-unit sequences, it does not prevent the input to have non-matching content. This patch fixes that by manually parsing the input.

Additionally it conforms to the recommended way of parsing an argument: peek input, pop input on successful parsing.

emilyy-dev avatar Sep 11 '24 18:09 emilyy-dev

Test Results

 88 files  ±0   88 suites  ±0   11s ⏱️ -1s 435 tests +5  435 ✅ +5  0 💤 ±0  0 ❌ ±0  480 runs  +5  480 ✅ +5  0 💤 ±0  0 ❌ ±0 

Results for commit 325d896c. ± Comparison against base commit 66a01439.

This pull request removes 1 and adds 6 tests. Note that renamed tests count towards both.
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_failing()
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_garbage()
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_invalid_unit()
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_leading_garbage()
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_no_time_unit()
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_no_time_value()
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_trailing_garbage()

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Sep 11 '24 18:09 github-actions[bot]

Additionally it conforms to the recommended way of parsing an argument: peek input, pop input on successful parsing.

This isn't the case in 2.x, the tree will restore the cursor on a failed parse. You only need to do this if your parsing logic calls for it.

jpenilla avatar Sep 11 '24 19:09 jpenilla

Additionally it conforms to the recommended way of parsing an argument: peek input, pop input on successful parsing.

This isn't the case in 2.x, the tree will restore the cursor on a failed parse. You only need to do this if your parsing logic calls for it.

In that case the docs need updating 👍🏻 I'll revert that change here for now

emilyy-dev avatar Sep 11 '24 19:09 emilyy-dev

Additionally it conforms to the recommended way of parsing an argument: peek input, pop input on successful parsing.

This isn't the case in 2.x, the tree will restore the cursor on a failed parse. You only need to do this if your parsing logic calls for it.

In that case the docs need updating 👍🏻 I'll revert that change here for now

Yes, the docs should be updated. When we switched over to the CommandInput there wasn't exactly a plan to no longer require peeking. That was introduced later on to increase the comfort of using the new system. I haven't done a thorough sweep of the documentation to reflect this (although it is at the very least pointed out in the external docs now)

Citymonstret avatar Sep 12 '24 16:09 Citymonstret