-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
As a user, if my posted declaration fails validation, I want to receive a 4xx error and I want the validation issues returned #336
Comments
A standalone is reporting an "uncaught exception" as well, so I don't think this is in the tasker:
Hank |
No, this is another one of those instances where the underlying parser (snakeyaml) can throw an exception that is either not advertised in the api or has not been properly handled on our end (core wres). We have had this several times before with various exception types and may need to resort to catching |
This is what we currently have on the embedded server. Then when we attempt to get the results we catch the "unknown" exception in a 500 INTERNAL SERVER ERROR. Prior when this had come up we had said that we should have that checking on the embedded server and to simply add the unadvertised exceptions to the core as they come up and wrap them into UserInputException, but I am not opposed to adding a catch all for these exceptions to the formatting stuff so we don't have this known timebomb wres/src/wres/server/EvaluationService.java Lines 738 to 761 in 4b4cdd1
|
We should address in the core app because the api advertises typed exceptions. I wouldn't really describe as a time bomb, though. Nothing breaks. It just produces a wrongly typed exception and resulting http error code. Not brilliant, but not a big deal. Will fix this and add a unit test later today after finishing some HEFS work. |
Incidentally, I think we're just mitigating a bug in a dependent api because these various exception types are not advertised, from what I recall, or aggregated, so it's a whac-a-mole as they arise. Still, better not to catch a generic |
Another question this potentially arises is: Should we create a job id before validation? Currently since this validation exception isn't handled we are redirected to a generic 500 page. From what Hank shows the interal logging is correct, but since an job ID is never generated we can't actually propogate that information to the user in any sort of way. Also if we were to generate the job id first we could properly log validation failures in the database. I don't know how much effort that would be, just thinking outloud |
Probably. Afterall, any web service api call is a job in some sense, at least a web service job. Not all jobs are evaluation jobs and not all web service jobs generate core app jobs (specifically, this validation pathway) but they are all jobs. This is reflected in the core app api, for example, where any call to |
I think we discussed adding some "validation workers" at some point, i.e., core app instances that are dedicated to validation tasks (or non-evaluation tasks). There is probably a connected ticket to which we need to close the loop. This would eliminate the slightly yucky special snowflake pathway for pre-validation via the |
Although, tbh, it is debatable whether that extra complexity is worthwhile. We could just send all validation jobs via the same pathway as evaluation jobs, using evaluation workers. Again, they should complete very quickly indeed and users engaged in validation checks are not different users than those engaged in evaluations, so it isn't as though we have thousands of validation endpoint users competing with evaluation end point users. |
Anyway, this is total déjà vu and we should dig out the earlier discussion/ticket and close the loop. |
Fixing the superficial issue with the core app. |
Going to create a separate issue to have the validation go through the service front door and/or discuss why this hasn't been done, as I cannot immediately find the earlier conversation. |
We created a special pathway for validation, because we did not want to have validation waiting on a worker to be free. As a user, it would be annoying if I have to wait an hour for my evaluation to get processed just to see that it failed validation. However, maybe I'm missing a key point made above, so let me review the comments more closely, Hank |
So I think I see the opposite case, more or less, mentioned above where we want to, "avoid hogging an evaluation worker with declaration validation tasks". However, that wasn't the primary concern if it was a concern at all. The primary concern was a COWRES that was busy handling 5 evaluations (which is rare, but has happened) and then someone posting another job (perhaps via the WRES GUI) and having to wait a while just to see that it failed validation. We want quick feedback on evaluations that don't pass validation. At least, that's what I recall, Hank |
Please add your comments to #339, but I think they are already addressed there. |
Catch and rethrow an exception with an advertised type, #336.
The declaration I attempted to post to the COWRES is below; note that
probability_threhsolds
is declared twice. When posted,curl
returns the following:The same thing happens if I paste the declaration to the API front-end and click
Check
to validate it: I'm forwarded to a page that reflects the above HTML.In the tasker logs, I can see the validation error:
That message is embedded in an exception that is very long and looks like this (snipped):
I'll try to get a complete copy of the exception, but its pages long which makes it hard for me to grab through an ssh terminal. I'll have to download the log and open it up locally to grab the entire stack trace.
More information in a bit,
Hank
The text was updated successfully, but these errors were encountered: