Refactor exceptions phase 3

[saneshark]

This PR will make a lot of sweeping changes to Common::Exceptions, particularly with BackendServiceExceptions.

1. All backends should try to use the raise_error middleware in some form, it will make errors consistent.
2. The error returned will be a very generic 'Operation Failed' exception unless a unique mapping is provided in the common.exceptions.yml file
3. All other exceptions will eventually be modified such that title, detail, and other attributes cannot be overridden with options. The unique minor code will map to that error unless it is a model validation error or the exception accepts arguments that are used to build the detail message.

The goal is to bring continuity across all errors rendered as part of vets.gov and have unique minor codes dilineating these errors.

More info and documentation to follow.

[saneshark]

@patrickvinograd The changes I've made to BackendServiceException will make it so that the DSL you're currently relying on in multi_client will no longer work. Can we work together to come up with a good alternative?

@austin-martinez @markolson @carlmjohnson I'd like to get your feedback on this as well. Particularly, to learn what, if any, errors you return as part of a response. I would characterize anything that is not a 200 level response as an error. How are you rendering these and how can we change it so that it starts using Common::Exceptions.

[austin-martinez]

For claim status/EVSS, some errors are returned to vets-api as a 200 response with errors in the JSON body. We are currently subclassing StandardError and raising the error in lib/evss/error_middleware. This would be a good candidate to switching to a Common::Exception. Timeouts/breakers exceptions are rescued by the client and data is pulled from the DB if present, the error is not re-raised. All other connection/application errors are not caught specifically by any of our controllers/clients and are handled in the ApplicationController

[saneshark]

@austin-martinez I'm looking at your error middleware you guys use, and I see what you mean now. Unless the payload contains a success it is an error, I wonder if maybe we can have a middleware that coerces the env body and env headers -- essentially normalizes errors and success to be more http spec like, and then use the raise_error middleware I've done here.

I can think of some alternatives but I like this approach the best. Curious to hear your thoughts on making this change. In either case it can be done as a separate PR.

[patrickvinograd]

The ArcGIS REST API has the same thing - sometimes returns a 200 with an "error" JSON object. I don't know that you're going to be able to catch all the variants in one middleware so part of each backend service integration is dealing with those - and then we just need some contract about how to raise them up the chain - either coerce the response env body/headers or just raising a certain type of exception that is caught/coerced into a BackendServiceException.

[saneshark]

@patrickvinograd that's true, but i think we could have an intermediary middleware, one that occurs before the raise_error middleware that coerces it into a more "standardized" format that raise_error can work with.

I've tried to make raise_error be dynamic enough to handle anything we throw at it, i can easily add something to it where it also checks for status[200] and success == false

The main thing though is, raise_error is designed for errors we present to an api consumer. If the errors are caught and other errors raised that would be more or equally representative to a user as to what happens that's fine... but so long as they conform to JSONAPI and have unique code for each distinct message returned.

[saneshark]

@aub there is more of this to come, but i think this touches enough places to warrant its own individual code review and I'll continue doing more of the same for the internal error messages.

Based on the comment thread, I think eventually, I will look to replace the functionality I have now for the raise_error middleware such that it need only accept argument for the Code prefix, and everything else would be handled in a normalization middleware that is used before this one.

[aub]

Ok, here's a first pass. Overall looks really solid. I'm with you on the concern over how to get it deployed and am happy to discuss sometime.

[aub]

Oh man do I hate this rubocop rule. You cannot convince me that this change makes the code look better. How about roll this back and add the removal of the rubocop rule to the commit? Here's the .rubocop.yml code I have in provider screening:

# This makes you do things like this:
# variable = if test
#   'abc-123'
# else
#   'def-456'
# end
#
# I think this is harder to read than assigning the variable within the conditional.
Style/ConditionalAssignment:
  Enabled: false

[kreek]

I'd argue the former is more readable, depends what you're used to 😄 I think it's more Ruby-ish (Lisp/Smalltalk influences) and avoids duplication.

[aub]

The thing that bugs me the most is the crazy indentation, which is also required because of another rubocop rule. The actual assignment of the variable is less of a problem, but I'd argue that assigning the variable outside of the if like this makes it jarring to read because we're used to the if beginning on the left side of the line. Ultimately, I just can't think of a good reason for rubocop to enforce this style.

[aub]

I wonder if we'll catch this happening... almost seems like it should raise a different error type under these circumstances so that we know there's an error type we're not handling. Looks like as it is, it'll default to 'backend_service_exception', which is fine. But I bet we want some way to ensure we're seeing these.

[saneshark]

Is there a way to use Raven when you don't have an actual exception? Just a warning.

[aub]

It looks like you can pass it an exception without raising it, so I think you could do something like this:

exc = UnhandledBackendServiceExceptionException(blah, blah)
Raven.capture_exception(exc)

[aub]

I'm not following how this error message lines up with the if test. If the status code isn't in the 400-999 range, does it imply that this was invoked from the wrong place?

[saneshark]

Yea, this is kind of a dumb heuristic. It would be a violation of OOP to reference the caller, don't want to do that. Maybe I can make the error message be something else. The idea here is though, that one shouldn't mess with raising this exception outside of the middleware, but I can't think of a good way of enforcing that. Maybe just being deligent in code review. Not sure it's even necessary as long as its done correctly -- maybe i'll remove this check entirely.

[aub]

Yeah, I think I'd put back the comment about not invoking it directly and remove the test.

[aub]

Why the underscored method names? Doesn't it suffice that these are private?

[saneshark]

Yea, I have seen the underscore pattern used before when there is a semantic difference in meaning to what the class is designed to do with something having the same name. I could just remove the underscore and keep them private. My syntax highlighting in atom also makes it think as though "source" is a reserved word. Which is mildly annoying.

[aub]

I bet this is fine for now, but these lines using `env[:body][{source_key,detail_key,code_key}]``` do make an assumption that the response body contains these things as easily accessible attributes. Is this always the case? I think something was mentioned about some services that return a 200 and then have the error status buried in a JSON response. Might be worth a thought as to how we'd handle that case here. Presumably you'd have to either subclass the middleware or pass it procs that can pull out the keys based on the env.

[saneshark]

No, and you're right. I think a better strategy would be to have preliminary middleware before the raise_error that normalizes these and then the raise_error middleware just accepts argument for the prefix, 'Rx' or 'SM' I thought about introspecting somehow to get this as well, but couldn't think of a good way to do it.

[austin-martinez]

Could this use capture_message instead?
https://docs.sentry.io/clients/ruby/usage/#reporting-messages

[saneshark]

Ideally this PR will be merged to dev and staging + production will cherry pick all the other PRs that have been merged with the exception of this one. I will make sure to squash and merge to make this easier.

That way I can test it out on dev to make sure there are not any issues before we push to prod. Thoughts? CC: @patrickvinograd @jkassemi @aub

I'll write up a test plan for this as well prior to merging.

[aub]

Yikes, that seems like it would be pretty difficult to manage. So what I'd need to do at deploy time is create a new branch, find the list of commits in master but not in production, then cherry-pick all of those commits into the new branch, except for this one?

And aside from that complexity, I'm concerned about the fact that the code in dev won't be consistent with what goes to production, which could cause problems.

I think we should take a step back and think about what our goal is here. If this code needs to sit in a non-production, deployed environment for a while, how long would that last? What would we be testing by doing so? Is there a simpler way to achieve the same?

[saneshark]

Hmmm, good point.

I think there will be situations that will arise where we need Staging or Dev just to test critical patches to production that need to go out. And that everything else would happen during an EOW iteration of some sort. I think this would be something that would be a good candidate for EOW. Perhaps it would get pushed early in the week to dev or staging and by the EOW if everything checks out then we're good to merge. But I think having a dev or staging setup just for the daily patches then is ideal.

In Agile parlance, I would characterize these changes as part of an Epic, and each of the individual parts as User Stories. I think a collection of User Stories deserve their own iteration / release schedule and QA process independent of daily critical patches we make to production.

It seems right now, we're just merging stuff and then addressing issues that arise with critical patches after the fact because we don't really have a formal QA process or iteration -> deployment strategy in place.

Realistically though, I should be able to spend a few hours playing around with this locally testing, and maybe we can just merge straight to production. Worst case scenario I'm around to push a quick patch or fix, or we can roll it back. Thoughts?

[aub]

Ok, I think this is good to get merged. Plan is to drop it into dev/staging today, then do some extensive testing tonight and tomorrow before the deploy.