Add support for date, time, and date-time validators by jnewc · Pull Request #118 · kylef/JSONSchema.swift

jnewc · 2021-04-14T08:24:31Z

Picks up on the work done in #90.

jnewc · 2021-04-14T08:37:46Z

@kylef can you review please?

jnewc · 2021-04-20T14:27:15Z

@kylef hey again, any updates on this?

kylef

Thanks for taking a go at this @jnewc.

It seems like there's still a few gaps in the rfc3339 support which I've commented below. Could you please add this patch to enable the JSON Schema tests suite (which is not very extensive but at-least covers some positive and negative cases):

diff --git a/Tests/JSONSchemaTests/JSONSchemaCases.swift b/Tests/JSONSchemaTests/JSONSchemaCases.swift
index e4ac246..67e1dc8 100644
--- a/Tests/JSONSchemaTests/JSONSchemaCases.swift
+++ b/Tests/JSONSchemaTests/JSONSchemaCases.swift
@@ -117,7 +117,6 @@ class JSONSchemaCases: XCTestCase {
       "infinite-loop-detection.json",
 
       // optional formats
-      "date-time.json",
       "email.json",
       "hostname.json",
       "uri-reference.json",
@@ -137,8 +136,6 @@ class JSONSchemaCases: XCTestCase {
       "infinite-loop-detection.json",
 
       // optional, format
-      "date-time.json",
-      "date.json",
       "email.json",
       "hostname.json",
       "idn-email.json",
@@ -146,7 +143,6 @@ class JSONSchemaCases: XCTestCase {
       "iri-reference.json",
       "iri.json",
       "relative-json-pointer.json",
-      "time.json",
       "uri-reference.json",
       "uri-template.json",
     ] + additionalExclusions)
@@ -173,8 +169,6 @@ class JSONSchemaCases: XCTestCase {
 
       // optional, format
       "format.json",
-      "date-time.json",
-      "date.json",
       "duration.json",
       "email.json",
       "hostname.json",
@@ -183,7 +177,6 @@ class JSONSchemaCases: XCTestCase {
       "iri-reference.json",
       "iri.json",
       "relative-json-pointer.json",
-      "time.json",
       "uri-reference.json",
       "uri-template.json",
     ] + additionalExclusions)
@@ -212,8 +205,6 @@ class JSONSchemaCases: XCTestCase {
 
       // optional, format
       "format.json",
-      "date-time.json",
-      "date.json",
       "duration.json",
       "email.json",
       "hostname.json",
@@ -222,7 +213,6 @@ class JSONSchemaCases: XCTestCase {
       "iri-reference.json",
       "iri.json",
       "relative-json-pointer.json",
-      "time.json",
       "uri-reference.json",
       "uri-template.json",
     ] + additionalExclusions)
@@ -245,7 +235,7 @@ class JSONSchemaCases: XCTestCase {
         return cases.filter {
           if let schema = $0.schema as? [String: Any] {
             let format = schema["format"] as! String
-            return !["date-time", "email", "hostname"].contains(format)
+            return !["email", "hostname"].contains(format)
           }
 
           return true

kylef · 2021-04-20T17:19:22Z

@@ -0,0 +1,46 @@
+import Foundation
+
+func validateDateTime(_ context: Context, _ value: Any) -> AnySequence<ValidationError> {


While testing some of the examples from rfc3339 (https://tools.ietf.org/html/rfc3339#section-5.8), appears this isn't behaving as expected with leap seconds, the test below seems to fail.

import XCTest import JSONSchema class DateTimeFormatTests: XCTestCase { func testDateTimeWithLeapSecond() throws { let schema: [String: Any] = [ "$schema": "https://json-schema.org/draft/2020-12/schema", "format": "date-time", ] let result = try validate("1990-12-31T23:59:60Z", schema: schema) XCTAssertTrue(result.valid) } }

@kylef I'm a bit confused about this one as it's called out as invalid in the jsonschema.org tests repo - see https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/6bc53e60c3de5d2e2ab15a84bcd3a79ca12c66a9/tests/draft2020-12/optional/format/date-time.json#L28

This seems to be at odds with the spec though:

1990-12-31T15:59:60-08:00 This represents the same leap second in Pacific Standard Time, 8 hours behind UTC.

Interesting, unsure if that's test is trying to disallow a leap second or instead there being 31 days in February. We can try to clarify that in the tests suite.

Started with json-schema-org/JSON-Schema-Test-Suite#481

1990-02-31T15:59:60.123-08:00 this particular date would be invalid even for a leap second, because leap seconds wouldn't be allowed in that date or time, but it should be possible in other times. There's further explanation at https://tools.ietf.org/html/rfc3339#appendix-D.

The leap second would need to be at the end of the day.

kylef · 2021-04-20T17:26:22Z

+func validateTime(_ context: Context, _ value: Any) -> AnySequence<ValidationError> {
+  if let date = value as? String {
+    let rfc3339DateTimeFormatter = DateFormatter()
+    rfc3339DateTimeFormatter.dateFormat = "HH:mm:ss.SSSZZZZZ"


From what I can tell from rfc3339, the time-secfrag component of partial time is optional:

partial-time = time-hour ":" time-minute ":" time-second [time-secfrac]

The following test can be used to assert this is implemented:

class TimeFormatTests: XCTestCase { func testTimeWithoutSecondFraction() throws { let schema: [String: Any] = [ "$schema": "https://json-schema.org/draft/2020-12/schema", "format": "time", ] let result = try validate("23:59:50Z", schema: schema) XCTAssertTrue(result.valid) } }

jnewc · 2021-04-20T17:50:32Z

Thanks @kylef - I'll ping you again when I've applied all the necessary updates 👍

…update time validator

jnewc · 2021-04-21T07:47:38Z

@kylef added some commits + a comment re: leap seconds

yspreen · 2021-04-21T11:48:45Z

This is actually not complete yet. While you added a date format, it fails during execution:

'format' validation of 'date' is not yet supported.

yspreen · 2021-04-21T12:08:37Z

See #121

jnewc · 2021-04-21T13:08:34Z

This is actually not complete yet. While you added a date format, it fails during execution:
'format' validation of 'date' is not yet supported.

This appears to be working fine for our team's use case - example validation error:

  - 0 : "\'2021-01-01T00:00:0A\' is not a valid RFC 3339 formatted date-time."

yspreen · 2021-04-21T13:19:37Z

Yeah, the error was something unrelated! Our id string in the given spec used HTTP instead of HTTPS. Still, some changes were necessary, so I based my PR on yours

jnewc · 2021-04-22T12:20:37Z

@kylef in case you missed it, this is ready for another review - thanks again :-)

kylef

@jnewc Overall I'm +1 on these changes, thanks for taking the time to prepare them and looking forward to merging this. I'd like to spend a bit of time verifying each of the RFC3339 behaviours, and hoping to do that in JSON-Schema-Test-Suite itself. So to start, I've prepared json-schema-org/JSON-Schema-Test-Suite#481.

I'm debating if we should make the date-time format use the underlying date and time validators internally so that we can be sure that they are absolutely identical in regards to behaviour (and would possibly simplify having multiple date formatters to validate each part). What do you think?

kylef · 2021-04-22T21:31:58Z

I'm debating if we should make the date-time format use the underlying date and time validators internally so that we can be sure that they are absolutely identical in regards to behaviour (and would possibly simplify having multiple date formatters to validate each part). What do you think?

If going down that route we could have internal functions such as isValidTime, isValidDate etc which internally the validators would use. Would possibly do the same for other validators such as the email format, because it depends on similar logic from the host/ipv4 and ipv6 formats.

jnewc · 2021-04-23T07:59:01Z

If going down that route we could have internal functions such as isValidTime, isValidDate etc which internally the validators would use. Would possibly do the same for other validators such as the email format, because it depends on similar logic from the host/ipv4 and ipv6 formats.

@kylef This seems like a good idea in principle - for clarity, do you mean that the validators would i.e. split the date and time on 'T' and then validate separately, in all cases for date-time? or something else?

EDIT: Thinking about this, it could be taken further by splitting dates/times down into their component parts, validating the correct number of components and such along the way, which would in turn avoid using things like the regex I added to ensure two-digit padding is honoured.

The other benefit here would be avoiding using Swift's DateFormatter entirely, which is clearly too flexible for + lacks the configurability needed to meet rfc3339 requirements.

kylef · 2021-04-23T11:20:12Z

After giving more thought to this, in date-time we will need to parse both the date and time, not just validate the date and time separately, but also enforce further restrictions that the time is valid for the given date (respecting leap seconds etc). That's one constraint I didn't consider yesterday.

If your keen, we can try tackle the time format on its own, and validating against json-schema-org/JSON-Schema-Test-Suite#481 (we can use the branch for those tests in Tests/Cases submodule to validate it works correctly) and iterate on date and then date-time. If I have a chance tonight I'll try prepare an equivalent to those time tests for date, looking at date.json, it doesn't cover many cases (and only one positive case).

jnewc · 2021-04-23T15:20:11Z

After giving more thought to this, in date-time we will need to parse both the date and time, not just validate the date and time separately, but also enforce further restrictions that the time is valid for the given date (respecting leap seconds etc). That's one constraint I didn't consider yesterday.

This is a very good point that I hadn't considered.

If your keen, we can try tackle the time format on its own, and validating against json-schema-org/JSON-Schema-Test-Suite#481 (we can use the branch for those tests in Tests/Cases submodule to validate it works correctly) and iterate on date and then date-time. If I have a chance tonight I'll try prepare an equivalent to those time tests for date, looking at date.json, it doesn't cover many cases (and only one positive case).

Well I am certainly keen to scratch this itch! However, I'm not sure I have the time free right now to get too immersed in the leap-seconds problem due to other commitments. That will change in a few weeks though and I'd definitely be keen to pick it up then!

kylef · 2021-04-23T22:05:31Z

Ran out of time for now, but made an attempt at regex time validation, missing the leap second checks but there as comments in 12bbf6d in the time branch. Not entirely sure this is the best approach so just exploring options.

jnewc · 2021-04-26T07:26:26Z

@kylef LGTM, as a first pass.

In the interim, you could add 23.59.60+00:00 as a literal check.

For longer term solutions that verify the time and offset in pair (as mentioned in your code comment), I'd lean towards this not being possible and/or viable with regex ... maybe with some clever use of backrefs, but even then it will be a maintenance nightmare and so a procedural solution might be more desirable.

yspreen · 2021-04-30T10:44:49Z

Okay guys, I haven't followed the discussion too closely, so sorry for jumping in. But I want to add two quick thoughts:

We are using jnewc's branch for an EU-wide project at the moment, and it works perfectly. Merging this PR would fix the current issue with master not allowing date formats: 'format' validation of 'date' is not yet supported. It would be good to get rid of a fork in our dependency list.
To get back to the schema discussion in Add support for date, time, and date-time validators, and support a greater range of schema ids. #121: That PR is completely disfunct, and I agree with closing it. However, I still think making schema validation more robust would be a valuable asset to this project. If you agree, @kylef, I'll make a new one with a) more robustness, and b) avoiding the mistakes I did in Add support for date, time, and date-time validators, and support a greater range of schema ids. #121. Let me know. No need in opening a PR when I know it will not get merged :)

kylef · 2021-04-30T14:48:34Z

We are using jnewc's branch for an EU-wide project at the moment, and it works perfectly. Merging this PR would fix the current issue with master not allowing date formats: 'format' validation of 'date' is not yet supported. It would be good to get rid of a fork in our dependency list.

One thing to mention so your aware, there are dates which I think this branch may errenously validate as incorrect when they are indeed valid dates. I think this will mostly be around the second fraction decimal places and leap seconds. There may be risk in the opposite direction too, I don't recall the details at the moment. So bear that in mind when using this in any production environment. Working on getting those things fully tested and implemented.

To get back to the schema discussion in Add support for date, time, and date-time validators, and support a greater range of schema ids. #121: That PR is completely disfunct, and I agree with closing it. However, I still think making schema validation more robust would be a valuable asset to this project. If you agree, @kylef, I'll make a new one with a) more robustness, and b) avoiding the mistakes I did in Add support for date, time, and date-time validators, and support a greater range of schema ids. #121. Let me know. No need in opening a PR when I know it will not get merged :)

I'll reiterate what my thoughts for clarity:

I'm completely for allowing https when the spec states http for the scheme. Forcing the use of http when the spec is defined as https is not going to be accepted, but I will happily accept the https when spec states http.
There's two places in which meta schema id/$id are matched from (both from $schema, and from $ref). The initial detection of a schema type from $schema I think you alredy found what does that. The other place for $ref is within the RefResolver when a schema uses $ref against a metaschema. The schemas are registered with the resolver upon load. Perhaps we should register metaschemas twice in that case for https upgrades) in the respective validator (for example Draft7Validator). I would like to ensure the behaviour is consistent with matching core specs whether it be via $id or $ref.
I'm hesistant to allow http when the spec states https and json-schema.org explicitly calls out that https should be used. When it comes to interopability with other libraries, that can incur an insecure HTTP request to fetch an untrusted metaschema. Disallowing that entirely reduce risk as it forces consumers to ensure that a trusted URL is used which uses encryption and certificate verification or embedded in libraries.

kylef · 2021-04-30T17:26:44Z

@jnewc I've merged in the date parser (pretty must as-is) via 9b9dc51, I've extended the test suite in json-schema-org/JSON-Schema-Test-Suite#483 to contain all the month-day limit validations which should test regex vs date formatter (i.e, they would fail if date formatter check is removed).

I've merged in a slightly different time validation to count for leap seconds in bd34ab5.

Next step is to bridge them together and do the date-time parsing. 🎉

cwagdev · 2021-05-03T17:22:42Z

Looking forward to the date-time addition!

jnewc · 2021-05-04T08:00:33Z

@kylef great news!

CI is failing because there are now two files named time.swift, which the Swift compiler is unhappy with - do you have a preference of which filename you'd like to change, and to what name?

kylef · 2021-05-04T18:19:03Z

Based on #118 (comment) and #118 (comment), I'd suggest that the file should exist in the Sources/Format directory.

jnewc · 2021-05-05T10:28:27Z

Sorry @kylef got my wires crossed - offending file has been removed.

yspreen · 2021-05-06T18:17:23Z

I think we're good to merge? @jnewc

yspreen · 2021-05-10T17:20:46Z

This is literally the only thing holding us from releasing our library, spm doesn't let you release a package that relies on non-merged dependencies 😄

What can I do to help get this PR to the finish line?

ismailmustafa and others added 13 commits August 15, 2020 12:27

Add carthage support

fe9f0c9

Add date-time, date, and time validators

4802423

Enable tests for new validators

0aec188

Merge branch 'master' of https://github.com/kylef/JSONSchema.swift

696b2dd

Merge remote-tracking branch 'source/master'

67ef451

Re-add date + time validators

c01e38c

Add swiftpm generated folder to gitignore

a7f6c82

Update validators + tests

991b653

Move validators to correct folder

a47ad74

Fix gitignore

5d4edf7

Remove xcodeproj

4ddf0a4

Cleanup

9ff0b2a

Fix indentation

c21d826

kylef reviewed Apr 20, 2021

View reviewed changes

Jack Newcombe added 3 commits April 21, 2021 08:27

Apply patch from @kylef

0fb8929

Move date validator to format.swift

29e2b15

Add test case for time with timezone + without fraction seconds, and …

4637ac1

…update time validator

yspreen mentioned this pull request Apr 21, 2021

Implement "format" validation #3

Open

6 tasks

yspreen mentioned this pull request Apr 21, 2021

Add support for date, time, and date-time validators, and support a greater range of schema ids. #121

Closed

kylef approved these changes Apr 22, 2021

View reviewed changes

Merge remote-tracking branch 'source/master'

cb1085f

Remove duplicate formats keys

e31ef51

Remove time.swift from Validation folder

cdb44c4

yspreen mentioned this pull request May 16, 2021

Replace our fork of JSON Schema with the base, once changes are implemented eu-digital-green-certificates/dgca-app-core-ios#32

Open

3 tasks

Merge remote-tracking branch 'source/master'

8f962f8

jnewc closed this Sep 1, 2023

		@@ -0,0 +1,46 @@
		import Foundation

		func validateDateTime(_ context: Context, _ value: Any) -> AnySequence<ValidationError> {

Conversation

jnewc commented Apr 14, 2021

Uh oh!

jnewc commented Apr 14, 2021

Uh oh!

jnewc commented Apr 20, 2021

Uh oh!

kylef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kylef Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

jnewc Apr 21, 2021

Choose a reason for hiding this comment

Uh oh!

kylef Apr 22, 2021

Choose a reason for hiding this comment

Uh oh!

kylef Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylef Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewc commented Apr 20, 2021

Uh oh!

jnewc commented Apr 21, 2021

Uh oh!

yspreen commented Apr 21, 2021

Uh oh!

yspreen commented Apr 21, 2021

Uh oh!

jnewc commented Apr 21, 2021

Uh oh!

yspreen commented Apr 21, 2021

Uh oh!

jnewc commented Apr 22, 2021

Uh oh!

kylef left a comment

Choose a reason for hiding this comment

Uh oh!

kylef commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewc commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kylef commented Apr 23, 2021

Uh oh!

jnewc commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kylef commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewc commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yspreen commented Apr 30, 2021

Uh oh!

kylef commented Apr 30, 2021

Uh oh!

kylef commented Apr 30, 2021

Uh oh!

cwagdev commented May 3, 2021

Uh oh!

jnewc commented May 4, 2021

Uh oh!

kylef commented May 4, 2021

Uh oh!

jnewc commented May 5, 2021

Uh oh!

yspreen commented May 6, 2021

Uh oh!

yspreen commented May 10, 2021

kylef Apr 22, 2021 •

edited

Loading

kylef commented Apr 22, 2021 •

edited

Loading

jnewc commented Apr 23, 2021 •

edited

Loading

jnewc commented Apr 23, 2021 •

edited

Loading

kylef commented Apr 23, 2021 •

edited

Loading

jnewc commented Apr 26, 2021 •

edited

Loading