Datapusher fixes

[gcsantos-gpa]

Required By

GridProtectionAlliance/SystemCenter#506
✅ GridProtectionAlliance/gpa-gemstone#317

SC-161
SC-162
SC-163
SC-199

[StephenCWills]

Do we need to continue to support the legacy UserAccount authentication? Seems to me like what we should do is fully switch over to APIQuery and then maybe adjust authorization rules to disallow DataPusher requests from regular users.

[gcsantos-gpa]

Do we need to continue to support the legacy UserAccount authentication? Seems to me like what we should do is fully switch over to APIQuery and then maybe adjust authorization rules to disallow DataPusher requests from regular users.

The intention, as I understand it, was to not disrupt legacy behavior or create a breaking change.

[StephenCWills]

Can we go ahead and create a breaking change then? IMO, we can fix the errors at GTC/GPC easily enough, but we have no process for removing the legacy behavior later.

[StephenCWills]

Really excellent cleanups in this PR. I like where this is headed. I just noticed a couple problematic patterns that were repeated several times. They should be easy enough to clear up.

[StephenCWills]

I know this pattern wasn't introduced this PR, but now seems like a good time to switch to response.EnsureSuccessStatusCode().

Suggested change

	if (!response.IsSuccessStatusCode)
	throw new InvalidOperationException($"Server returned status code {response.StatusCode}: {response.ReasonPhrase}");
	response.EnsureSuccessStatusCode();

[gcsantos-gpa]

I did so, out of curiosity and ignorance, why is this a bad pattern?

[StephenCWills]

response.EnsureSuccessStatusCode() will throw an HttpResponseException, I believe. Exception handlers will be more likely to expect it. For example, and if I'm not mistaken, the HTTP status code would be passed through to the client automatically by ApiController handlers.

It also reduces nesting, which typically makes the code more readable.