How do we validate input data?

[chris48s]

See longer conversation in #5597 (comment)

What exactly is the purpose of the schema validation?

How loose/strict they should be?

Some suggested principles for input validation:

• Our definition of "valid" should be no stricter than the upstream service's definition of "valid". Perhaps to phrase this in RFC-like language: "our validation MAY be less strict than the upstream service but MUST NOT be more strict than the upstream service". Exactly where in that grey are we fall is going to be determined by other badge/service-specific criteria/requirements.

• If theory (docs) and practice (real-world API responses) conflict, real-world outputs take precedence over documented behaviour. e.g: if the docs say version is a semver but we learn that there are real-world packages where the version number is 0.3b or 1.2.1.27 then we should accept those values in preference to enforcing the documented API behaviour.

• Shields is descriptive ("the registry says your package version is 0.7, therefore your package version is 0.7") rather than prescriptive ("you're doing versioning wrong - lol"). We reflect the established norms of the communities we serve. If it's good enough for the upstream, it's good enough for us.

Some other thoughts on principles we can apply. Maybe these ones are more about how we identify when validation that already exists is incorrect rather than how we write schemas from scratch:

• If we know of a real-world example of a package/repo/etc that causes us to render an error value on a badge (e.g: etc), our input validation is broken and we should fix it.

• If we know of a real-world example of a package/repo/etc that causes us to thrown an unhandled runtime exception, our input validation is broken and we should fix it.

• We should not fail to render a badge because of a validation failure on a field that isn't necessary to render the badge. (for example, a version badge should not fail to render because the license field is missing). It is not inherently wrong to define a single schema which is applied to multiple badges. For example, its fine in principle to define a schema that says

  const schema = Joi.object({
    version: Joi.string().required(),
    license: Joi.string().required(),
  }).required()

  for a shared helper and have both the license and version badges validate the response against that schema. However, if we become aware of a real-world example of a package/repo/etc that has a version key but not a license key then we should split the schema (or make version optional and handle the error in code). Maybe we know that at schema creation time (e.g: because its documented), or maybe we don't find out until some later point in time (because we have a real-world example of it happening via a support request).

Would welcome input from other maintainers...

[calebcartwright]

Cross posting the gist of my last comment from the linked issue for reference

Still digesting and thinking on this one, but wanted to note that I look forward to the discussion and hopefully being able to decide on a direction. There's been different and yes some times conflicting prescriptions on the "how strict/loose" topic, and it was one of the more confusing, and occasionally frustrating, things for me when I first started contributing to Shields.

Ultimately I don't care all that much about what the decided outcome is, but really want to avoid contributors receiving different messages depending on which maintainer is reviewing (and what may be on that maintainer's mind that day), or which docs/existing badges are used as a reference.

[calebcartwright]

@chris48s - my takeaway theme from the guiding points you've articulated would be:

define all schemas as loose as they can possibly be without causing potential runtime type errors, and retroactively tighten them if/when needed.

I think in general I can get behind that strategy, although I do still think there's merit to having stricter schemas that surface incorrect assumptions by failing more loudly/obviously. I don't have a good example offhand, but my sense is that we've had our fair share of bugs that were more easily debugged due to the invalid response data error reported by the user.

The most important outcome of this discussion from my perspective is that we document the agreed upon strategy, and use/enforce that going forward (as well as adjusting existing non-compliant schemas over time). Perhaps I'm wrong, but I'd posit there's a mix of both ends of the schema strictness scale today and that there are plenty that are overly strict in the context of this proposal, and that they did not get there iteratively but were instead born very tight/strict.

[PyvesB]

Will have a think as well, but reading through your principles makes sense to me so far @chris48s !

[chris48s]

my takeaway theme from the guiding points you've articulated would be:

define all schemas as loose as they can possibly be without causing potential runtime type errors, and retroactively tighten them if/when needed.

I wouldn't consider that a correct summary, or certainly not a complete one.

Rendering a badge that says doesn't throw a runtime exception. We should still not render it. Part of what we're trying to is prevent unexpected or undesirable outputs/behaviours.

Also, if you think about a schema like

license: Joi.string().required()

we could make that

license: Joi.alternatives().try(Joi.string().required(), Joi.number().required())

That is more lenient (the second constraint will accept more inputs than the first and its not going to cause us to throw any more errors than the first one), but it would also make no sense and it does not fit in with out understanding of what a license is. The schema should be informed by (and explain) our understanding of the input data, not just be "as loose as they can possibly be".

Perhaps another good purpose to think of a schema as having is as well as preventing us from accepting invalid data, its also partly about explaining/documenting to a human reading this code what we expect the input data to look like.

The way I would characterise it more is that any particular service/API is going to have a "window of sensible-ness" within which our schema should sit and I've attempted to write the (non-exhaustive) start of some principles that help us to determine
a) where each end of that window is and
b) under what circumstance we need to re-evaluate the window
but as long as our schema is somewhere inside that window, other characteristics of the schema (like simplicity, or readability, or providing better documentation of our expectations about the input data) might also inform exactly exactly where we choose to sit within that window in a particular case.

although I do still think there's merit to having stricter schemas that surface incorrect assumptions by failing more loudly/obviously

Another principle I've not made explicit but I'm going to suggest we encode is that the schema/validation we choose is informed by the assumptions we're making about the data. This can include basic type checks. For example: If we need to multiply it by something, we check its a number. If we're going to call .split() on it, we make sure its a string. If we're going to address foo[0], foo must be an array. But this can include more nuanced stuff e.g: If we're going to sort a version on the assumption it is a semver, check its a semver. I agree we do want to know that an upstream API is returning non-semver compliant versions if we're passing them to semver.compare() (whether that will throw an exception or just produce an unexpected result) and I'm definitely not suggesting we should loosen a schema that asserts a characteristic we are actively relying on. That's a case where the format of the version does matter. Equally though, if we're just going to bung foo.latest_version on a badge, we can dispense with a check of that nature. Check the stuff that matters, don't check the stuff that doesn't matter.

Does that cover that point?

The most important outcome of this discussion from my perspective is that we document the agreed upon strategy, and use/enforce that going forward

The most important thing for me is that this doesn't just make a rod for our own backs, or turn into somehow an reason to not fix a bug when its clearly a bug. If this just turns into telling a user " is correct because we wrote some stuff in a markdown document", this process is a waste of our time and everyone else's. Any decision or rule we write down should be amendable in light of better evidence. Documentation (like code) is not a sacred cow. It is just some stuff we made up, and our future selves can change it. They will be smarter than us.

[calebcartwright]

The most important thing for me is that this doesn't just make a rod for our own backs, or turn into somehow an reason to not fix a bug when its clearly a bug. If this just turns into telling a user " is correct because we wrote some stuff in a markdown document", this process is a waste of our time and everyone else's. Any decision or rule we write down should be amendable in light of better evidence. Documentation (like code) is not a sacred cow. It is just some stuff we made up, and our future selves can change it. They will be smarter than us.

I'm sensing quite a bit of anger/frustration in this, and hopefully that's not due to anything I've said or the way I've said it (my apologies if so). All I'm asking for is that we have some guidance we can point contributors to around what to do when it comes to strictness of schemas. I don't think that's unreasonable, nor do I think it's a waste of time.

I mentioned previously that I felt there was a lack of clarity on this topic when I was first contributing to Shields, and that's still a sense I have today. I don't think that experience/perception should be dismissed out of hand, even if others feel differently.

I'm not trying to define strategy nor documentation for the sake of doing it, nor do I strive to produce bad badges/invalid response data badges. I just wanted to have a discussion and agree to something instead of continuing the ad-hoc and often conflicting approaches we've taken to this recurring question.

[calebcartwright]

I wouldn't consider that a correct summary, or certainly not a complete one.

Yeah that's fair. I got overly strict in my theme-defining schema 😉

Does that cover that point?

Yes, I was just trying to distill it down to a one liner but lost some important nuance. I guess it' could be more of a.. no stricter than necessary to render a good badge, but completely agree that what meets that qualification of good/sensible-ness will absolutely be context/API/service specific.

[chris48s]

I'm sensing quite a bit of anger/frustration in this

I thought the tone of this draft was fairly light-hearted tbh. Maybe it needed more emoji

All I'm asking for is that we have some guidance we can point contributors to around what to do when it comes to strictness of schemas

Guidance is good. Writing down what we do and why we do it is helpful. I fully agree with this - hence having bothered to spend time on this exercise so far. I suspect I will spend a fair chunk more time on it before we're done with this.

Being too rigid about enforcing that guidance, not so much. I guarantee at least one of the things I've written here sounds reasonable now but will turn out to be wrong in the face of a sensible example. Feel free to be the person that provides it.

FWIW, I think this process should be mostly about describing the good practices we've learned from writing hundreds of these things and learning from our mistakes rather than defining a swathe of changes we're going to apply to make everything "compliant". Obviously I've not read every single one of the hundreds of schemas we've already defined and I'm sure there's some weird stuff in there, but I reckon the vast majority are already somewhere inside the "window of sensible-ness".

I mentioned previously that I felt there was a lack of clarity on this topic when I was first contributing to Shields, and that's still a sense I have today

Anything in this thread helping with that, or not really?

often conflicting approaches

Got any particular examples of that? If you can't bring any to mind, no worries, but if we've got some examples of the kind of situation we're trying to clarify or inconsistency we're trying to prevent then that is going to be a big help in writing the docs that are supposed to clarify it.

[chris48s]

default to the isBuildStatus validator, and generally don't go outside that unless you really need to

Yeah I'm happy to not be too specific, Maybe we're just starting from different points and discovering that isBuildStatus is the most prevalent validator so we can say "default to isBuildStatus if you can" is enough of a clarification at this point. To me it seemed like the main question to resolve with build status badges is more like when do we jam another edge case into isBuildStatus and when to just not use it, and I'm not sure I have more of an answer to that now.

[calebcartwright]

Maybe we're just starting from different points and discovering that isBuildStatus is the most prevalent validator so we can say "default to isBuildStatus if you can" is enough of a clarification at this point.

Yeah good point. I think it's really more of an illustrative example. If you're using the renderBuildStatus helper, then you need some kind of schema strict enough to ensure that the helper can produce the right message value & color, and isBuildStatus is a good way to achieve that?

[PyvesB]

From a user perspective, I'm not convinced there's much value in making build status validation too strict. For example, if a CI returns "in progress", I would much rather have than . For us as maintainers, if we notice it we would immediately jump to the conclusion "we need to tweak the validator schema", but for some random person browsing someone else's repo, joining all the dots leading them to report a bug on the Shields.io project is a very long shot - most likely this transitional state will never be flagged up but will keep on popping up now and again and simply give the viewer the feeling that something has not been quite polished somewhere. That doesn't prevent us from having stricter validation on the test side though, so that we can notice more easily when a color or build message needs tweaking.

Generally speaking, I think that if we can render a badge that provides valuable information to the user, we should go ahead and render it. If the color is not perfect and defaults to the standard blue, it's not the end of the world in my opinion. Of course, I'm in agreement that for data that we can tell is blatantly wrong and doesn't convey useful information, e.g. , there's no value in trying to render the badge.

[calebcartwright]

I guess I'm unclear on the degrees of "wrongness" for a badge.

IIRC, an unknown status will result in a lightgrey/inactive message color and IMO a build badge like (I don't recall if pass is something already covered, just using it as an example of an unrecognized status with the helper)

is just as blatantly wrong as the coverage badge

I also believe that one of the benefits of having some kind of minimal check within the schema beyond the presence of the key is that it can force the author/developer to do some up front thinking and review of the potential responses from the upstream service.

If we were to transition to just using Joi.string() for all build status badges and accepting that for all new badges I think we'd definitely have more incorrect badges.

[calebcartwright]

but for some random person browsing someone else's repo, joining all the dots leading them to report a bug on the Shields.io project is a very long shot

I do agree with you on this point though. I can never seem to find it when I need it, but there's been previous discussions about letting service classes override the value of this message and I suppose one option on the opposite end of the spectrum would be to change this to something very explicit like (but not literally 😄) Shields.io schema validation bug - please report at https://github.com/badges/shields/issues/new/choose

[calebcartwright]

I would say nexus is probably a good example of one where we are checking for something we don't really need to check at input time. It looks like nothing we are doing with latestRelease or latestSnapshot really requires them to be in a specific format.

Completely agreed, and that's why I picked it. We've had a few previous reports about invalid response data and had to adjust the regex accordingly. Huge benefit of relaxing this one is no longer having to battle regexes.

I think we do have consensus that not knowing what colour to render the badge is a condition we want to prevent and we should validate to guard against that

Yup, i think color would indeed fall under the "correct" umbrella we've been discussing here in addition to the message. So while yes, badges in the build status category would relatively have stricter schemas, the reason is because it's needed to ensure proper coloring of the badge.

That being said, there's obviously a coloring aspect. to version badges too. Many (most, all?) use the version helpers which also have some assumptions about the contents. I guess the difference here is that a version would have to be pretty wild (starting with something bonkers like xa..) to cause issues, and that doesn't seem like anywhere near as realistic a possibility as a build tool changing/adding a status variant

shields/services/color-formatters.js

Lines 9 to 23 in ff9273a

	function version(version) {
	if (typeof version !== 'string' && typeof version !== 'number') {
	throw new Error(`Can't generate a version color for ${version}`)
	}
	version = `${version}`
	let first = version[0]
	if (first === 'v') {
	first = version[1]
	}
	if (first === '0' || /alpha|beta|snapshot|dev|pre/i.test(version)) {
	return 'orange'
	} else {
	return 'blue'
	}
	}