Should our list based results provide an entry of "error" data sets

[MikeNeilson]

Or perhaps a separate list.

In reviewing #865 it turns out the very first element of the NAB rating templates causes the error, thus zero results are returned.

This would make it rather difficult to fix anything. If the user was aware and had permissions they could correct the element. But all they get in that case is "not valid because X" but which rating template isn't identified at all.

This is possibly true for any other data set, while that shouldn't be able to happen, we do have a lot of historical data from before certain constraints or defaults were appropriately applied.

[MikeNeilson]

FYI, for awareness of the thought. @rma-psmorris @adamkorynta @RyanM-RMA @rma-rripken @rma-bryson @DanielTOsborne @krowvin

[adamkorynta]

Ratings are one of the few API's that do validation of the data returned by the database before sending to the client, and this is due to the fact that it uses the hec-cwms-ratings libraries instead of basic DTO's. These API's are an outlier to me unless we decide to add more business logic to CDA.

The normal DTO required field validation is only called on POST requests to short circuit logic before hitting the database and give more meaningful responses to the client. If we add that to list based retrieval that would add overhead for each element in the list (which may be worth it?).

Last bit of context, there is some precedence for this issue in the CWMS schema for ratings. The cwms_rating.store_ratings_xml method does return a clob with descriptions of ratings that failed to store.

[MikeNeilson]

In the particular case where this came up, the error was well before it would've gotten to a DTO. It was nested down in the DAO.

[MikeNeilson]

Oh wait I see what your saying. I still don't think that's quite the issue. But likely part of it. In the case of the data in #865 it was the first rating in the result set that was incorrect and thus the user gets no data.

I'm pretty sure that data is somehow wrong in the database, but if not, the validation is wrong.

[adamkorynta]

Right, that's because we are going through the hec-cwms-ratings API, where the validation takes place, which is atypical for all of our other endpoints. So it seems like this is a good idea for this particular endpoint, but we'd need to start adding more validation to other GET endpoints to make it worth adding to the others.

[MikeNeilson]

The particular error throw is actually in the CDA code: https://github.com/USACE/cwms-data-api/blob/fbce50d6abbdb74e15caac28a5020b4e0c24c3b7/cwms-data-api/src/main/java/cwms/cda/data/dao/RatingTemplateDao.java#L212

[adamkorynta]

Oh I see! That still seems like an outlier to me, searching across the DAO classes for throw new that seems to be one of the few validations done on a GET request. Of course that doesn't count general non-thrown RuntimeExceptions but those are just CDA bugs that shouldn't be returned to the client and should be analyzed through an enterprise logging solution (or manual log analysis) where we could keep tabs on unexpected exceptions. (as a side note, I think we should lower the log level from WARNING for 404 errors so that it doesn't get reported at the same level as real bugs).

I think I'd like to see 2 or 3 other places that we would use an errors field given the current codebase before we add to a base class. Either that or if we agree to start adding more CDA validation on GET requests (we'd probably want some performance metrics on that).

[MikeNeilson]

Yeah, definitely an edge case. The other question is would does that particular validation even matter in the grand scheme of things.

But it general if only a single element of returned data has an error, we probably shouldn't bail on everything, but the user needs to know something is wrong; or at least an administrative user does.

[adamkorynta]

That's what all our API's currently do since we generally just return the data as-is from the database, then the client decides how to return individual row failures. Maybe some of the package calls that return UDT's have validation and internal schema failures would report for all the rows.

[MikeNeilson]

Okay, then #865 is definitely an outlier as it isn't doing that.