[download-microservice] Improve Error Handling

[confused-Techie]

This PR addresses the two issues we are seeing in the download microservice:

• The releases object not being iterable. This object is defined by what GitHub returns so we have minimal control over it. While there is an error check for this step in the form of a try...catch handler, we can add a check here to validate we are working with an array, and if not return that we got improper data from GitHub. As well as log what we received, so that we can investigate the issue better.
• When we work with the parameters provided by users, we previously would call just .split() on the value, not checking if we had received a string. When this occurs it was uncaught, causing the entire microservice to crash. Now we can validate we have a string or if not bail early.

[DeeDeeG]

I'm late reviewing this since I was away for a couple of days, but thanks very much for the fix and this looks great to me!!

We can get maybe a less mysterious handling of this scenario for end-users of this microservice with this change, which makes it clearer that the issue isn't our fault if it happens to be e.g. GitHub's fault.

👍

[DeeDeeG]

I am unsure if raw can possibly not be a string, since it starts with e.g. https://... and then the user-provided query parameters afterward.

That said, being conservative around that possibility feels more worthwhile than YOLOing if there is any remaining doubt -- And maybe there's a little doubt, so ultimately 👍 .

[confused-Techie]

This change was actually in a reaction to us calling split() later on the raw value and crashing the microservice because the method wasn't available.

If I had to assume, the fact that it can be something not a string depends on how the HTTP handler built into NodeJS decodes this data, since I'm pretty sure we are accessing the parameters, rather than the full raw URL string

[DeeDeeG]

Ah, okay.

I think I got a bit confused by var names and code comments nearby.

Maybe I can think of a way to clarify the argument names, such as req here, and the path var name in the call site:

package-frontend/microservices/download/index.js

Line 12 in 6bdf643

os: utils.query_os(path[1]),

As well as making this comment a bit less ambiguous, since I mistakenly read it as implying the argument would take the form https://...?param=val&param2=val2...:

let raw = req; // The URL string containing any number of query params.

[confused-Techie]

@DeeDeeG So just double checked, and what you thought does have a lot of merit.

In index.js we take the path via req.url and split this on ?. Meaning we did at one point have the full path, as you envisioned it. But then we just pass the second part of that array to these query parameter handlers. Which, in the event there was nothing to actually split, and the resulting array has a single item, then we would be passing null meaning it's not a string.

So absolutely some better comments here and variables names aren't a bad idea.

[DeeDeeG]

Suggested change

	let raw = req; // The URL string containing any number of query params.
	let raw = req; // The query string containing any number of query params.

This would clarify it a bit for me, personally.

https://en.wikipedia.org/wiki/Query_string

[confused-Techie]

Not a bad suggestion, I'll probably try to clean up this code just a little bit in the near future