check that wendigo query param equals 'true'

[kevinwilde]

req.query.XXX is a string, so right now when a request is made with ?wendigo=0 or ?wendigo=false, req.query.wendigo is truthy and we still get wendigo errors.

This change would make it so that you only get wendigo errors with ?wendigo=true

[kevinwilde]

Not sure if you'd prefer checking that req.query.wendigo === 'false'. The reason I didn't is that the guzzle http client in the php sdk sends boolean query params as either '0' or '1', so if we were checking that req.query.wendigo === 'false', you'd have to know to pass ['wendigo' => 'false'] rather than ['wendigo' => false] when using the sdk.

[connorjclark]

Let's be a bit more explicit and strict about this. Current changes make it somewhat difficult to reason about the accepted values / behavior.

something like this?

let wendigo;
if (req.query.wendigo === 'true' || req.query.wendigo === '1') wendigo = true;
else if (req.query.wendigo === 'false' || req.query.wendigo === '0') wendigo = false;
else throw new Error('unacceptable value for wendigo. expected one of: false, true, 0, 1')

If parsing query params is need elsewhere at some point, I could see this being being extracted to a module.

[kevinwilde]

Maybe this is just personal preference, but I don't think query params should be required, and I don't think unexpected values for query params should cause an error. In both cases, I think a default value should be used.

[connorjclark]

Oh, definitely should not require it, that was a mistake. last else should check if req.wendigo is not undefined.

I mainly wish to prevent unexpected input from producing unexpected behavior (which is what motivated you to open this PR). For example, if "true" and "1" is allowed for the above parameter, one might think "yes" could work too, but attempting to use that could (with a loose implementation) result in a value of false unexpectedly being assigned. Also, strictly defining the accepted input domain helps with changing the interface in the future without potentially causing breaking changes. I think that information is important to surface.

Surfacing that information doesn't have to be done through an error - perhaps being that strict is only good for libraries, not services that communicate over the web (where dealing with breaking changes are more of a hassle to coordinate). Ideally, would be good to still continue normal execution.

If the response body wasn't the HTML payload, we could just tack on an "errors"/"warnings" field. So, we could add it to the response header, or just log a warning without alerting the requester in any way. WDYT?

This doesn't matter much for this one use case here, but I want to ensure a good process exists in case future plugins come about that make use of query parameters.

[kevinwilde]

Ok here's my thinking right now. If the wendigo plugin is being used, we should default to wendigo being true. (If someone didn't want to use wendigo at all they would just not use the plugin.) If wendigo is undefined or an unexpected value, continue normal execution. In the case of an unexpected value, log a warning.

Something like?

let wendigo = true
if (req.query.wendigo) {
  if (req.query.wendigo === 'false' || req.query.wendigo === '0') {
    wendigo = false
  } else if (req.query.wendigo !== 'true' && req.query.wendigo !== '1') {
    core.log(`theia:wendigo ${componentLibrary}`, "WARNING: Unexpected value for wendigo. expected one of: 'false', 'true', '0', '1'")
  }
}

This also puts the default value for wendigo in the wendigo plugin, as opposed to in the sdk here. That seems like an improvement. The sdk shouldn't have something that is so specific to the wendigo plugin, which is optional to use.

[connorjclark]

Looks good.

This also puts the default value for wendigo in the wendigo plugin, as opposed to in the sdk

I agree, good idea.