Skip to content
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.

Sign up for GitHub

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

test a malformed pattern #386

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

test a malformed pattern #386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions tests/draft2019-09/pattern.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,27 @@
"description": "pattern is not anchored",
"schema": {"pattern": "a+"},
"tests": [
{
"description": "doesn't match a substring",
"data": "xxyy",
"valid": false
},
{
"description": "matches a substring",
"data": "xxaayy",
"valid": true
}
]
},
{
"description": "pattern is malformed",
"schema": {"pattern": "hel{1,"},
"tests": [
{
"description": "invalid regex does not match",
"data": "hello",
"valid": false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For drafts pre-2019-09 this is (potentially) incorrect I think -- it depends whether the metaschema it's run under is validating formats or treating them as annotations or whatever. So for those this'd need to go in optional. For 2019-09 I don't remember what changed -- do you enable format via a vocabulary and then it has to be respected, or what?

(Also this is invalid JSON but probably you'll notice that part)

Copy link
Member Author

@karenetheridge karenetheridge Jun 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not assuming/expecting the metaschema to use a format validation. Even if no validation is done on the schema at all, it should still return a non-valid result at runtime.

Copy link
Member Author

@karenetheridge karenetheridge Jun 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invalid json fixed -- and the test suite is now happy, so it looks like the json was just parsing badly before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may not be the case. An implementation is free to blow up entirely in this case if you give it an invalid schema and it isn't validating it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i.e. it's valid for the result here to be "my validator blows up" rather than returning invalid)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As well, assuming that “valid=false” for failed schemas changes the meaning of “valid” for the tests in that suite. It’s also bad practice to have meanings of fields change depending on other variables.

Copy link
Member

@ChALkeR ChALkeR Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps there should be another flag to indicate invalid schemas (apart from valid: false on tests), perhaps in a separate directory/subdir, where the impl is free to blow up on schema compilation (like it should imo)?

That would be different from valid: false, as the impl can either follow this and return false, because none strings match invalid pattern, or it can as well reject the schema alltogehter which should also be a valid behavior. Per schema, pattern should be a regex, and this is not a regex.

Copy link
Member Author

@karenetheridge karenetheridge Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we've been discussing this across a few different PRs and in slack. There hasn't been any consensus yet as to how to declare this in tests (with an extra flag? a new directory under optional/? both of these?) or how to document how implementations should deal with it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to merge the outstanding PRs on tests in the "normal" category before addressing this. I.e. #354 and the PRs being split off of it. Let's get those merged, then we can hit cases that may require a structural change. Could definitely use more eyes reviewing those PRs so we can get them off the queue.

Copy link
Member

@gregsdennis gregsdennis Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, my implementation gives an (arguably incorrect) error, but doesn't blow up:

The string value was not a match for the indicated regular expression

(Interestingly, .Net doesn't think this is an invalid pattern.)

I think this is one of the things I had to account for in some other regex test where .Net didn't support something. I switch from deserializing the regex directly to just deserializing a string and lazily parsing the regex.

}
]
}
]
16 changes: 16 additions & 0 deletions tests/draft3/pattern.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,27 @@
"description": "pattern is not anchored",
"schema": {"pattern": "a+"},
"tests": [
{
"description": "doesn't match a substring",
"data": "xxyy",
"valid": false
},
{
"description": "matches a substring",
"data": "xxaayy",
"valid": true
}
]
},
{
"description": "pattern is malformed",
"schema": {"pattern": "hel{1,"},
"tests": [
{
"description": "invalid regex does not match",
"data": "hello",
"valid": false
}
]
}
]
16 changes: 16 additions & 0 deletions tests/draft4/pattern.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,27 @@
"description": "pattern is not anchored",
"schema": {"pattern": "a+"},
"tests": [
{
"description": "doesn't match a substring",
"data": "xxyy",
"valid": false
},
{
"description": "matches a substring",
"data": "xxaayy",
"valid": true
}
]
},
{
"description": "pattern is malformed",
"schema": {"pattern": "hel{1,"},
"tests": [
{
"description": "invalid regex does not match",
"data": "hello",
"valid": false
}
]
}
]
16 changes: 16 additions & 0 deletions tests/draft6/pattern.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,27 @@
"description": "pattern is not anchored",
"schema": {"pattern": "a+"},
"tests": [
{
"description": "doesn't match a substring",
"data": "xxyy",
"valid": false
},
{
"description": "matches a substring",
"data": "xxaayy",
"valid": true
}
]
},
{
"description": "pattern is malformed",
"schema": {"pattern": "hel{1,"},
"tests": [
{
"description": "invalid regex does not match",
"data": "hello",
"valid": false
}
]
}
]
16 changes: 16 additions & 0 deletions tests/draft7/pattern.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,27 @@
"description": "pattern is not anchored",
"schema": {"pattern": "a+"},
"tests": [
{
"description": "doesn't match a substring",
"data": "xxyy",
"valid": false
},
{
"description": "matches a substring",
"data": "xxaayy",
"valid": true
}
]
},
{
"description": "pattern is malformed",
"schema": {"pattern": "hel{1,"},
"tests": [
{
"description": "invalid regex does not match",
"data": "hello",
"valid": false
}
]
}
]