Open sirosen opened 7 months ago
In general this sounds fine to me -- the main two things I think I'd want to be sure of are:
ValidationError
into n
per-keyword exception classes, e.g. exceptions.failure.MinItems
, exceptions.failure.Required
, exceptions.failure.AllOf
, etc. In that world, a bunch of the error properties are going to be deprecated (e.g. ValidationError.context
which only applies to applicator keywords, so will only be an attribute on AllOf
and not Required
exceptions). And hopefully _Error
will disappear entirely, but we'll see. I did some work to start on this but didn't finish it, mostly again because to do it right requires assembling a test suite for error messages that's better than the ad hoc one here (TestValidationErrorMessages
). It's not an insurmountable amount of work, just work, and I intend(ed) to get back to it sometime this year now that referencing is maturing (though there's an important piece of work I want to do there too which is higher priority). Anyways -- all this is relevant only inasmuch as I don't want to make that work any harder -- but I think the attribute you're proposing will be applicable to all classes, so that should be fine.context
and cause
-- extremely general sounding names which we use in sort of specific ways. So I want to avoid more of that, especially again to be sure the names stick when we do the Great Exception Split of 2024(hopefully). I thinkshort_message
is a bit clearer for that, but maybe there are even more ideas we can bikeshed if need be. We'll regardless have to document what you say (that it's guaranteed to not have the instance in it).But other than the above, and a request to add tests of this property for all keywords, I'm good with it!
I have a mix of thoughts, not all well-organized, so I'll try to summarize what I see right now.
I've found some APIs somewhat-frustrating to use when they make names for related things hard to distinguish. e.g. reason
, cause
, details
, and description
all being different strings. It's hard to know which one is which. reason
is vulnerable to this criticism.
I'm not sure that short_message
is what I would choose though, since it's possible, at least in one of my examples for maxLength
, for the message to be longer than a message
.
I really wanted to call it cause
, but I saw that's already in use. I'll need to think more about naming. Never easy! :grinning:
This is going to take me some time to digest and make sure I understand. I'm not worried too much about it, but I haven't digested it enough to think through the implications.
message
(like keyword values)I think that both of these are features which would be nice to have. If I were to separate those, maybe there are two new fields?
# pseudocode for the new attributes
class ValidationError:
...
# a pair of (keyword, keyword-value), e.g. `("maxLength", 3)`
keyword: tuple[str, Any]
# a string unique to the keyword validation failure, e.g. "too long" for maxLength
error_category_message: str
I'm just spitballing a little bit here.
info currently missing from message (like keyword values)
The only known examples of this should be #119, #992 and #993 and in general it's considered a bug, but yeah the solution I'd like is what I mentioned, of course happy to elaborate or brainstorm further on it
I just wanted to drop a small note, since it's been ~1 month and I haven't done anything:
I still want to do this, but I'm trying to negotiate the scheduling at work so that I can do this during working hours. (I've had really limited hobby development time lately.)
Cool, thanks for the update -- I have little doubt you won't completely disappear :D -- definitely understood on the hobby side!
This is a feature which I'd be very happy to work on, if it gets the ol' :+1: of approval as an idea! (It's a case in which I could probably do FOSS work as part of my paid work allotment, rather than on personal time.)
Motivation
For context, we have a use-case at $WORK in which
jsonschema
is being used to validate data which is sourced from a mixture of locations. Some of the data is user-supplied, and some of it is sourced from config or application-owned secrets. As a result, the user who is triggering this interaction is not guaranteed to have the rights/permission to see the full body of data being passed tojsonschema
. The application therefore cannot showValidationError.message
data, since it may contain secrets.I think the feature idea here has broader applicability -- and I don't think that the case I have at $WORK represents a great application design -- but there's my particular user story.
More generally, having some kind of message string which doesn't include the
instance
would allow applications to inspect the messages more reliably and potentially choose special behaviors for known errors. Today, this is only possible with some detailed knowledge of the error formats, and checks such aserr.message.startswith(...)
.Proposal:
ValidationError.reason
I'm proposing a new string property on error classes, which is primarily intended for
ValidationError
but may be useful in other cases (needs some evaluation). The purpose of this property is to name the error case sans the input instance. In concert with the JSON Path to the failing instance, this is nearly equivalent to the current content inmessage
, but it doesn't producestr(instance)
as part of the error message.reason
is either provided explicitly or falls back toNone
. I considered having it default tomessage
but that makes things messier for a client, which then has fewer guarantees about what might be in thereason
field.Where possible,
reason
may be used to add details which are not present in themessage
and would now be a breaking change to introduce.reason
would be documented as a concise description of the error cause which does not to contain details from the instance being evaluated.Example Changes
additionalProperties
keyword validation produces messages of the form"Additional properties are not allowed ({fields} {was|were} unexpected)"
. Thereason
field on such an error would be"Additional properties are not allowed."
.maxLength
validation produces messages of the forms"{instance} is expected to be empty"
and"{instance} is too long"
. Thereason
field on such an error would be"The value is too long. The maximum length is {maxLength}."
Proposed Implementation
I would make this change by first updating the core
_Error
base class to supportreason
as astr | None
field. I then "just" need to walk all of the keywords and add explicitreason
strings for each validator, plus a test case for each.I don't think this is very complex as a change, although it is a bit tedious and laborious to add.