Fix freeform guardrails

[ansoncfit]

We currently limit the number of destinations and OD pairs in regional analyses with time/path results. This PR fixes a couple issues with the related guardrails:

• Fixes a conditional so that when oneToOne = true, the broker checks the number of OD pairs (which equals the number of origins) against the correct limit, rather than just allowing the analysis to proceed.
• Checks limit on number of destinations for path analyses in broker. Workers were applying a stricter limit (5000 destinations for path analyses) than the broker, so the broker could potentially enqueue millions of tasks that would trigger an immediate error once they reached the workers.

Small refactor for brevity/legibility: task = job.templateTask

[abyrd]

The underlying problem where tasks continue to be distributed to workers after an error was reported should be solved by #921. With that fixed, mismatches in validation rules between different components (UI, backend, workers) should be much less problematic and just result in a few error messages.

That said, it's also a good idea to make these checks more specific as in this PR.

[ansoncfit]

Is throw AnalysisServerException.badRequest(... preferable here?

Is it straightforward to apply filterStackTrace (moved to ExceptionUtils in #921) to the message eventually passed along to the UI?

[abyrd]

On your second question: yes, I think so. It looks like we could apply filterStackTrace to the message parameter in each call to HttpApi#respondToException (there are five of them just above the method definition).

[abyrd]

On your first question: it looks like calling badRequest would yield an identical AnalysisServerException instance but with type BAD_REQUEST instead of UNKNOWN. It looks like the only effect would be the string BAD_REQUEST instead of UNKNOWN in a field in the JSON returned to the UI. Assuming this does not change UI behavior in any problematic way, yes badRequest does seem to better represent the source of the exception.

[ansoncfit]

Thanks for pointing out that method. I think we want to apply it to the stackTrace, not the message. I committed that below for testing on staging, but feel free to revert.

[abyrd]

Oh right. It was late when I was reading that and I mistakenly thought the callers were passing e.getStackTrace() as the message. This was probably driven by some kind of wishful thinking because I realized filterStackTrace is so narrowly suited to output from one method (throwable.printStackTrace via ExceptionUtils.stackTraceString) that it probably shouldn't be public and applicable to the more general String type. This is a minor detail but it's located in error handling code that should be foolproof (things could rapidly get annoying to debug if the signal path for the debugging messages themselves is failing). It looks like it is failsafe and would at worst return "Unknown stack frame" but there could be some edge case we're not thinking of.
Of course in current usage, both places the method is used are filterStackTrace(errorEvent.stackTrace) so we know it's always being applied to a final field that's the output of ExceptionUtils.filterStackTrace(). This should work fine but I'm still thinking about ways to clean up the method signature.
Option A: have the public method be public static String filterStackTrace (Throwable throwable) then call that in the ErrorEvent constructor. This would cause all console log traces to be filtered. All others (Slack) are already filtered.
Option B: move filterStackTrace back to a private method on ErrorEvent and always use it via a public wrapper method that only reads from ErrorEvent.stackTrace. (Won't work because we need it in RegionalWorkResult)
Option C: Add a field to ErrorEvent so it has both the full and filtered stack trace
Option D: keep the entire Throwable in a private field on the ErrorEvent and generate the stack traces on demand

[abyrd]

I did option C as it ensures the trace string always came from the right method but still allows for verbose stack traces in console logs. Seems to work in local testing on some missing regional results.

D looks cleaner, but it defers processing of the stack trace to the moment the log or slack message is written, thus deferring some potential failures to those background event handling threads. I'd rather front-load the stack trace processing to the point where the ErrorEvent is created, containing any delays or problems closer to the source of the original error.

[abyrd]

I am approving this as-is in case you want to merge it now. Feel free to add the exception changes discussed in the PR comments if you like in which case I can merge immediately after re-review.