-
Notifications
You must be signed in to change notification settings - Fork 496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[release/8.0] Fix ConfigurationSchemaGenerator to use correct TimeSpan format #3342
Conversation
'duration' isn't an acceptable description of how to represent a TimeSpan. Instead use a custom regular expression to represent TimeSpan formats.
@@ -69,12 +69,12 @@ | |||
"properties": { | |||
"Delay": { | |||
"type": "string", | |||
"format": "duration", | |||
"pattern": "^-?(\\d{1,7}|((\\d{1,7}[\\.:])?(([01]?\\d|2[0-3]):[0-5]?\\d|([01]?\\d|2[0-3]):[0-5]?\\d:[0-5]?\\d)(\\.\\d{1,7})?))$", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you author this based on the spec (appears to be BNF) or is it copied from someplace?
and assuming https://datatracker.ietf.org/doc/html/rfc3339#appendix-A is the right place to look at, what does this correspond to -- "Duration" here?
Mentally translating that BNF into pattern is a bit taxing on my brain. I see you have a bunch of tests -- are there any other canonical tests for this patter we can borrow from somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see it's documented in the code below. But still not sure which part of the spec it matches to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and assuming https://datatracker.ietf.org/doc/html/rfc3339#appendix-A is the right place to look at, what does this correspond to -- "Duration" here?
That is precisely the wrong place to look. We used to say format: duration
which says:
A duration as defined by the ISO 8601 ABNF for "duration". For example, P3D expresses a duration of 3 days.
But this doesn't work with TimeSpan.Parse
which is what the code is using.
did you author this based on the spec (appears to be BNF) or is it copied from someplace?
I authored it following the behavior of TimeSpan.Parse
, which is what the code that is parsing the string uses. See:
@@ -401,12 +401,21 @@ private void AppendTypeNodes(JsonObject propertyNode, TypeSpec propertyTypeSpec) | |||
} | |||
} | |||
|
|||
const string NegativeOption = @"-?"; | |||
const string DaysAlone = @"\d{1,7}"; | |||
const string DaysPrefixOption = @$"({DaysAlone}[\.:])?"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the parentheses in all these pieces intended to capture? (I don't know where this regex is executed)
if not, they should ideally be noncapturing (?: .... )
or RegexOptions.ExplicitCapture passed. Although, if it's just doing IsMatch, we're probably smart enough to realize this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't intended to do a capture. They are intended to be groupings for alternations and loops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can put the ?:
into the parens, but it really hurts the readability of the Regex. (not that it is currently all that readable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I don't know where this regex is executed)
It is executed whenever someone wants to validate the JSON document against the schema. For example in JsonSchema.Net: https://github.com/gregsdennis/json-everything/blob/fd3ce0b98f1ee6fa7c0f4965281151efa5df7e57/JsonSchema/PatternKeyword.cs#L87 it just calls Regex.IsMatch.
Regex.IsMatch is smart enough to not do captures because they won't be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving from bar POV
Backport of #3320 to release/8.0
/cc @eerhardt
Customer Impact
When setting TimeSpan values in appsettings.json, we are not writing the correct format in our Configuration JSON schema. We were using
format: duration
, which sounds like it is correct, but this format is:Which doesn't work with
TimeSpan
.Testing
Existing tests pass, added new tests for the TimeSpan format Regex.
Risk
Very low. Changing the valid format for TimeSpan values won't affect anything else.
Microsoft Reviewers: Open in CodeFlow