Skip to content

fix: DurationParser allowing invalid input#772

Merged
jpenilla merged 8 commits into
Incendo:masterfrom
emilyy-dev:fix/duration-parser-allows-invalid-input
Jul 19, 2026
Merged

fix: DurationParser allowing invalid input#772
jpenilla merged 8 commits into
Incendo:masterfrom
emilyy-dev:fix/duration-parser-allows-invalid-input

Conversation

@emilyy-dev

Copy link
Copy Markdown
Contributor

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.

@github-actions

github-actions Bot commented Sep 11, 2024

Copy link
Copy Markdown

Test Results

 88 files  ±0   88 suites  ±0   14s ⏱️ -2s
436 tests +6  436 ✅ +6  0 💤 ±0  0 ❌ ±0 
481 runs  +6  481 ✅ +6  0 💤 ±0  0 ❌ ±0 

Results for commit f49d197. ± Comparison against base commit ad10573.

This pull request removes 1 and adds 7 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()
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_unicode_digits()

♻️ This comment has been updated with latest results.

@jpenilla

Copy link
Copy Markdown
Member

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.

@emilyy-dev

Copy link
Copy Markdown
Contributor Author

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

jpenilla
jpenilla previously approved these changes Sep 11, 2024
@Citymonstret

Copy link
Copy Markdown
Member

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
Citymonstret previously approved these changes Sep 12, 2024
Citymonstret
Citymonstret previously approved these changes Sep 16, 2024
Citymonstret and others added 2 commits October 18, 2025 09:32
The previous regex only accepted ASCII digits, while Character.isDigit
also recognizes Unicode decimal digits. Avoid accidentally expanding the
accepted duration grammar, and add a regression test for full-width
digits.
@jpenilla jpenilla changed the title Fix DurationParser allowing invalid input fix: DurationParser allowing invalid input Jul 19, 2026
@jpenilla
jpenilla merged commit e8f9532 into Incendo:master Jul 19, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants