Handle [ClearlyDefined] empty response

[PyvesB]

The ClearlyDefined API would previously return a 500 error when querying an invalid provider or type was provided, Now there's a 200 with an empty response: https://api.clearlydefined.io/definitions/abc/xyz/-/jquery/3.4.1

I've fixed the handling to cover this change, and also strengthened our test coverage on this service.

[github-actions[bot]]

	Messages
:book:	:sparkles: Thanks for your contribution to Shields, @PyvesB!

Generated by :no_entry_sign: dangerJS against e2c4f2b7314d85bd4bb4bac7eb1f326ccea7689a

[PyvesB]

The problem here is that it's not an empty JSON response, i.e. {}, it's a completely empty response. I don't think any changes to the schema will work, the fundamental issue is that we're expecting the API to return JSON and it is not in this edge-case, which causes us to fail parsing before even validating the schema.

The only other clean-ish alternative I can think of is to make this badge depend on BaseService instead of BaseJsonService, copy-paste the logic from _requestJson, and insert an extra check between the calls to _request and _parseJson. The approach from this PR essentially uses an exception as control flow, which clearly isn't great, the BaseService one would be trading that against some code duplication, which isn't ideal either. I'm happy to give it a go if you think that would be better :)

[jNullj]

Made a suggestion at https://github.com/PyvesB/shields/pull/1690 Let me know what you think.

[PyvesB]

Thanks for putting the idea together! How would we go about keeping the unknown type, provider, or upstream issue message? Would we catch the EmptyResponse error and rethrow a new one with the non-default message?

[jNullj]

That's a bit similar to what you mentioned before, if we catch the exception this could work, but isn't best as a control flow. The main benefit I take from this is that we can now make sense of any empty response in the future for all services quickly.

We might be able to avoid raising exceptions for _request errors all together by introducing the exceptions inside the httpErrors variable passed down to the checkErrorResponse function. This might be the best of both world, have both exceptions with rendered error messages for unhandled exceptions while also be able to avoid raising exceptions by having checkErrorResponse make a new InvalidResponse with the crafted prettyMessage similar to how we already do for http error codes (like 404).

i would probably implement this similar to this add if (httpErrors[exceptionName] !== undefined) { props.prettyMessage = httpErrors[exceptionName] then use an else to use the logic i added before error = new EmptyResponse()

Do you think its worth pushing towards this?

[jNullj]

Sorry its a bit of an iterative process, i could probably notice this from the previous comment and put a bit more thinking into the solution i made before

[PyvesB]

Do you think its worth pushing towards this?

I think it's potentially good solution, but the main drawback is that the change is no longer localised to this badge, we're adding complexity in critical parts of the codebase. If other badges could benefit from the same pattern, it would be worth it, but if it's just for the present edge-case, I don't think we should go down that route. I've not checked other existing badges to assess that, I'm on my phone at the moment.

Alternatively, I'm more and more warming up to the idea of giving BaseService a spin. We'll avoid exceptions as control flow, keep the change localised, and probably not end up with that much duplication. :)

[PyvesB]

@jNullj I've given the BaseService approach a go, it looks pretty clean to me. Let me know what you think!

As a side note, there were less than 10 requests for this badge in the past 24 hours. I think it's not worth spending much more time on this, and have a change as localised as possible.