Fix negative duration parsing #552
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Jefftree The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/S
does this actually fix all the issues raised in 523? that looks like it had a lot of other things mixed in, like decimal support, long-unit support, etc |
| } | ||
|
|
||
| durationMatcher = regexp.MustCompile(`((\d+)\s*([A-Za-zµ]+))`) | ||
| durationMatcher = regexp.MustCompile(`(((?:-\s?)?\d+)\s*([A-Za-zµ]+))`) |
There was a problem hiding this comment.
can you clarify what additional values this allows? it's not clear to me that accepting new internal whitespaces between - and the number is a good idea here
Ah thanks for the catch, it does not. The primary concern was around decimal & long-unit support which is not addressed, so I don't see much value in getting just the negative duration part in. /hold |
k8s-ci-robot
added
the
do-not-merge/hold
| durationMatcher = regexp.MustCompile(`(((?:-\s?)?\d+)\s*([A-Za-zµ]+))`) | ||
| ) | ||
|
|
||
| // IsDuration returns true if the provided string is a valid duration |
There was a problem hiding this comment.
ParseDuration uses durationMatcher.FindAllStringSubmatch with an unpinned regex, which will skip and ignore any random junk before / between / after matches of the regex.
See https://go.dev/play/p/fCur5WS4zK5 for the type of nonsense IsDuration would accept currently.
To actually address the problem of decimal junk being accepted but ignored, we need the regex to fully define what we allow, and be pinned to the beginning / end of the string. Something like this
singleDurationMatcher = `-?(\d+)\s*([A-Za-zµ]+)`
// I'm not sure what separators we *intend* to allow between the items, whitespace? comma? something else?
multiDuration = `^(`+singleDurationMatcher+`)(\s*`+singleDurationMatcher+`)*$`
durationMatcher = regexp.MustCompile(multiDuration)
...
4 participants
Fixes #523.
Patch was applied from go-openapi/strfmt. go-openapi/strfmt#144
/assign @jpbetz