Rewrite the [DynamicJson] badge; affects [gitlab dynamic-xml dynamic-yaml]

[paulmelnikow]

This starts the rewrite of the dynamic badges. I've pulled into BaseService an initial version of the query param validation from #2325.

I've extended from BaseJsonService to avoid duplicating the deserialization logic, though it means there is a bit of duplicated code among the three dynamic services. The way to unravel this would be to move the logic from _requestJson and friends from the base classes into functions so DynamicJson can inherit from BaseDynamic. Would that be worth it?

Marked blocker since it affects other code that may validate query parameters (e.g. #2377), the other dynamic badges, and further cleanup of server.js.

This introduces a regression of #1446 for this badge.

Ref: #2345 (partial fix)

[shields-ci]

	Warnings
⚠️	This PR modified service code for gitlab but not its test code. That's okay so long as it's refactoring existing code.

	Messages
📖	✨ Thanks for your contribution to Shields, @paulmelnikow!

Generated by 🚫 dangerJS

[paulmelnikow]

The way to unravel this would be to move the logic from _requestJson and friends from the base classes into functions so DynamicJson can inherit from BaseDynamic. Would that be worth it?

I've found another case where the coupling of the request logic to the base class is causing problems. #1721 is about using github auth on a particular request, but only when static auth is present (i.e. for self-hosting). So I want the logic from BaseGithubAuthService if I'm using static auth, and otherwise what's in BaseJsonService.

If these functions were extracted to separate modules, it might leave open more options open for handling this unusual case.

[chris48s]

I think the previous value was right. This PR changes the default to

- https://shields-staging-pr-2399.herokuapp.com/badge/dynamic/json.svg?url=https%3A%2F%2Fraw.githubusercontent.com%2Fbadges%2Fshields%2Fmaster%2Fpackage.json&label=package&query=$.name

but I think we still want it to be

- https://img.shields.io/badge/dynamic/json.svg?url=https%3A%2F%2Fraw.githubusercontent.com%2Fbadges%2Fshields%2Fmaster%2Fpackage.json&label=package&query=$.name

[paulmelnikow]

Oops! Thanks for catching that.

[chris48s]

This introduces a regression of #1446 for this badge.

so that is what causes:

- https://shields-staging-pr-2399.herokuapp.com/badge/dynamic/json.svg?url=https%3A%2F%2Fraw.githubusercontent.com%2Fbadges%2Fshields%2Fmaster%2Fpackage.jso&label=package&query=$.name&colorB=blue

instead of

- https://shields-staging-pr-2399.herokuapp.com/badge/dynamic/json.svg?url=https%3A%2F%2Fraw.githubusercontent.com%2Fbadges%2Fshields%2Fmaster%2Fpackage.jso&label=package&query=$.name

right? Have you have any thoughts on how we might solve that? (if its large, it doesn't necessarily have to go into this PR)

[paulmelnikow]

Yes, exactly right.

Have you have any thoughts on how we might solve that? (if its large, it doesn't necessarily have to go into this PR)

I suppose we'll need to add an 'isError' field to serviceData. It can be set automatically in _handleError(), and manually in handle() for rare case like Crates. Then add logic in _makeBadgeData() to handle it.

shields/services/base.js

Lines 392 to 437 in 8070f0b

	static _makeBadgeData(overrides, serviceData) {
	const {
	style,
	label: overrideLabel,
	logo: overrideLogo,
	logoColor: overrideLogoColor,
	logoWidth: overrideLogoWidth,
	link: overrideLink,
	colorA: overrideColorA,
	colorB: overrideColorB,
	} = overrides
	const {
	label: serviceLabel,
	message: serviceMessage,
	color: serviceColor,
	link: serviceLink,
	} = serviceData
	const {
	color: defaultColor,
	logo: defaultLogo,
	label: defaultLabel,
	} = this.defaultBadgeData
	const badgeData = {
	text: [
	// Use `coalesce()` to support empty labels and messages, as in the
	// static badge.
	coalesce(overrideLabel, serviceLabel, defaultLabel, this.category),
	coalesce(serviceMessage, 'n/a'),
	],
	template: style,
	logo: makeLogo(style === 'social' ? defaultLogo : undefined, {
	logo: overrideLogo,
	logoColor: overrideLogoColor,
	}),
	logoWidth: +overrideLogoWidth,
	links: toArray(overrideLink || serviceLink),
	colorA: makeColor(overrideColorA),
	}
	const color = overrideColorB || serviceColor || defaultColor || 'lightgrey'
	setBadgeColor(badgeData, color)
	return badgeData
	}

I'd like to fix this, though it's a bit fiddly. There are possible merge conflicts to contend with in base.js in #2441. It's a minor enough regression that I think we can do it in a follow on. If this bug were important it wouldn't have been open almost a year. 😉

The test I disabled here really belongs in base.spec.js, and should get moved as part of #1446.

[paulmelnikow]

Gitlab failure seems transient.

[chris48s]

for rare case like Crates

Hrm. On reflection, I think with crates what I did in PR #2462 was not the best way to do it. Perhaps that case would be better handled by throwing a custom ShieldsRuntimeError more like this situation:

shields/services/cdnjs/cdnjs.service.js

Lines 29 to 33 in b2d5d3e

	if (Object.keys(json).length === 0) {
	/* Note the 'not found' response from cdnjs is:
	status code = 200, body = {} */
	throw new NotFound()
	}

If we say we should always be throwing an exception for cases like this, would that allow us to handle this issue one layer further out (where we catch the exceptions and wrap them in a badge)?

There are possible merge conflicts to contend with
It's a minor enough regression that I think we can do it in a follow on

Agreed - its just useful to think about how to do that, given its a bit fiddly

[paulmelnikow]

If we say we should always be throwing an exception for cases like this, would that allow us to handle this issue one layer further out (where we catch the exceptions and wrap them in a badge)?

We apply overrides further out than handling errors, which probably makes sense… since on error we should still respect e.g. ?style=flat-square. Probably there are places even further up, like cache handling, where we may want to consider whether or not it was an error.

Custom ShieldsRuntimeError seems nice; it could let us attach the error to what gets bubbled up to handleRequest, which could let us adjust the cache settings appropriately for probably recoverable vs probably permanent errors.

Probably we could catch ShieldsRuntimeError here…

shields/services/base.js

Lines 330 to 332 in b2d5d3e

	error instanceof InvalidResponse ||
	error instanceof Inaccessible ||
	error instanceof Deprecated

[shields-deployment]

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch: