-
-
Notifications
You must be signed in to change notification settings - Fork 308
Add discussion of runtime errors and refusal-to-process vs validation success/failure #1316
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
Closed
Closed
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm not sure that these need to be distinct. I think the outcome of a "refusal to process" is still a runtime error.
In .Net, a "runtime error" means that an exception would be thrown. This is the outcome in both cases.
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.
They're distinct because the expected responses are distinct (and this might actually mean I have these definitions a bit wrong in the current text).
A "refusal to process" is a correct outcome. There is nothing you can or should do to produce a different outcome. If you do something like remove an unsupported vocabulary that was preventing evaluation, you're really just trying something else entirely. It might be represented by a validation status of
null
orNone
or whatever, instead oftrue
orfalse
. It could be an exception, but it's not really something that is handled.A "runtime error" is an incorrect outcome. It indicates either an error in the schema (referencing a non-existent internal location, which can be determined immediately) or an error in configuration (failing to supply the necessary external schema for resolving references). These are correctable errors- exceptions you can potentially handle.
For example, if you get an exception that says that a reference to
https://example.com/external-schema#/$defs/whatever
cannot be resolved, you might be able to download that resource, load it, and re-try the original evaluation.Does this correct-but-no vs correctable-error distinction help? I am definitely open to re-framing this, "runtime error" and "refusal to process" were just what came to mind, and the definitions were off the top of my head.
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.
Hm... That's subtle, and I don't think I'd go through the effort of making the distinction except maybe throwing different exception types. But I'd still throw an exception for both cases.
If the spec is going to identify "refusal to process" as a non-erroring result, then accommodation needs to be made for it in the output. Currently there is no such accommodation.
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.
We should also identify the specific scenarios where "refusal to process" is the desired result (as we have with
$vocabulary
)."Runtime error" should be the catch-all result for anything not specified as one of the other result states.
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.
@gregsdennis
I like this idea. I think I got too caught up in when things can happen (schema load or evaluation) which produced a weird ambiguity/overlap in these categories.
An unresolvable reference should always be a "runtime error" even if it is detected at schema load time.
I don't think implementation details such as what is or is not an exception (some languages don't even have exceptions, some implementations don't use them) are relevant, though. I wouldn't put refusal-to-process in the output format, but I'm OK with deferring that decision until later (I wouldn't fight it tooth and nail either- it's a weak preference).
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'm okay with that. Sounds like the paragraph at line 642 needs to be updated, then, as it attempts to make the load/evaluation distinction.
Honestly, I'm not sure we need it at all unless you want to CREF it.
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.
@gregsdennis I removed it and rewrote the previous paragraph- please see if this seems clear and reasonable.
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 agree that the distinction between "runtime error" and "refuse to process" is hazy, especially since I think that encountering an unknown $vocabulary fits into both categories. You say that "refuse to process" means that nothing you can do would provide a better outcome, whereas an unresolvable $ref is fixable (e.g. by loading the missing resource). But isn't that the same as a missing $vocabulary? I could resolve that error by informing the evaluator about that vocabulary (that is, perform some extra bit of code to load that vocabulary logic into the evaluator).
Perhaps you're seeing the distinction as between "we can detect this condition by statically analyzing the schema", or "we don't know if the evaluation can be run to completion until we try to evaluate it against an instance"? We can certainly detect a missing $schema or $vocabulary in static analysis; whether or not you choose to detect a missing $ref at runtime is more tricky (certainly if you allow for loading resources from disk/network at runtime, you could delay this determination; also, even if all schemas need to be preloaded, circular references mean that one cannot immediately error on loading a schema if it contains an unknown $ref, as that resource may be added sometime in the future but still in time for a successful evaluation).