Enable format validation and fix bugs. #3
Conversation
|
|
||
| yield return new TestData<DateTimeOffset>( | ||
| Value: new(new DateTime(2021, 1, 1), TimeSpan.Zero), | ||
| AdditionalValues: [DateTimeOffset.MinValue, DateTimeOffset.MaxValue], | ||
| ExpectedJsonSchema: """{"type":"string","format": "date-time"}"""); | ||
| ExpectedJsonSchema: """{"type":"string","pattern":"^\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}(?:\\.\\d\u002B)?(?:Z|[\u002B-]\\d{2}:\\d{2})?$"}"""); |
There was a problem hiding this comment.
One thing to contemplate here is that this change is trading off human readability for precision. Could this be something that could create problems with downstream consumers?
There was a problem hiding this comment.
(Note: I'm not even a JSON schema novice)
Is it possible to define custom "formats" at all? Could we make a "dotnet-date-time" format where the pattern could be 100% accurate, but in all the places it is used, users just see "format": "dotnet-date-time", so it is more readable?
I agree readability is important. But it feels like precision is necessary because getting false positive schema errors for valid data is pretty annoying.
There was a problem hiding this comment.
Is it possible to define custom "formats" at all?
OpenAPI v3 is permissive about allowing you to define custom formats (e.g. we defined one for unsigned integers) although I dunno that dotnet-datet-time would be one that we'd pursue.
Is {type: "string", format: "date-time", pattern: "the-regex-here" } a permissible schema here?
There was a problem hiding this comment.
Is
{type: "string", format: "date-time", pattern: "the-regex-here" }a permissible schema here?
It would result in false negatives in implementations that do validate for the format keyword.
There was a problem hiding this comment.
Is it possible to define custom "formats" at all? Could we make a "dotnet-date-time" format...
Yes, custom formats are permissible, however you have the same problem that format isn't validated by default.
With custom formats, you now have the added problem that your format probably wouldn't be supported/understood by the validator, so even if it wanted to validate, it wouldn't know how (without custom code).
https://docs.json-everything.net/schema/basics/#schema-format
Here's what my validator supports.
There was a problem hiding this comment.
FWIW I reverted some of the changes because it turns out this was only needed for DateTime, but not DateTimeOffset. This is because the built-in converter for DateTime omits TZ offsets in the representation if the instance is of DateTimeKind.Unspecified.
There was a problem hiding this comment.
The test failures are being triggered because I'm feeding my tests with DateTime.MaxValue which has an undefined TZ. I wonder if supporting DateTimeKind.Unspecified is the right trade-off over using format : date-time.
|
Superseded by #5 |
Addressing partial feedback from dotnet/runtime#103096
cc @gregsdennis @captainsafia