Commits on Mar 10, 2020

1. [SIEM] Fix and consolidate handling of error responses in the client (e… …

   …lastic#59438)
   
   * Convert our manual throwing of TypeError to a custom Error
   
   Throwing a TypeError meant that our manual errors were indistinguishable
   from, say, trying to invoke a method on undefined. This adds a custom
   error, BadRequestError, that disambiguates that situation.
   
   * Present API Error messages to the user
   
   With Core's new HTTP client, an unsuccessful API call will raise an
   error containing the body of the response it received. In the case of
   SIEM endpoints, this will include a useful error message with
   potentially more specificity than e.g. 'Internal Server Error'.
   
   This adds a type predicate to check for such errors, and adds a handling
   case in our errorToToaster handler.
   
   If the error does not contain our SIEM-specific message, it will fall
   through as normal and the general error.message will be displayed in the
   toaster.
   
   * Remove unnecessary use of throwIfNotOk in our client API calls
   
   The new HTTP client raises an error on a 4xx or 5xx response, so there
   should not be a case where throwIfNotOk is actually going to throw an
   error.
   
   The established pattern on the frontend is to catch errors at the call
   site and handle them appropriately, so I'm mainly just verifying that
   these are caught where they're used, now.
   
   * Move errorToToaster and ToasterError to general location
   
   These were living in ML since that's where they originated. However, we
   have need of it (and already use it) elsewhere.
   
   The basic pattern for error handling on the frontend is:
   1) API call throws error
   2) caller catches error and dispatches a toast
   
   throwIfNotOk is meant to convert the error into a useful message in 1).
   We currently use both errorToToaster and displayErrorToast to display
   that in a toaster in 2)
   
   Now that errorToToaster handles a few different types of errors, and
   throwIfNotOk is going to be bypassed due to the new client behavior of
   throwing on error, we're going to start consolidating on:
   
   1) Api call throws error
   2) caller catches error and passes it to errorToToaster
   
   * Refactor Rules API functions to not use throwIfNotOk
   
   * Ensures that all callers of these methods properly catch errors
   * Updates error toasterification to use errorToToaster
   * Simplifies tests now that we mainly just invoke the http client and
   return the result.
   
   throwIfNotOk is not being used in the majority of cases, as the client
   raises an error and bypasses that call.
   
   The few cases this might break are where we return a 200 but have errors
   within the response. Whether throwIfNotOk handled this or not, I'll need
   a simpler helper to accomplish the same behavior.
   
   * Define a type for our BulkRule responses
   
   These can be an array of errors OR rules; typing it as such forces
   downstream to deal with both. enableRules was being handled correctly
   with the bucketing helper, and TS has confirmed the rest are as well.
   
   This obviates the need to raise from our API calls, as bulk errors are
   recoverable and we want to both a) continue on with any successful rules
   and b) handle the errors as necessary. This is highly dependent on the
   caller and so we can't/shouldn't handle it here.
   
   * Address case where bulk rules errors were not handled
   
   I'm not sure that we're ever using this non-dispatch version, but it was
   throwing a type error. Will bring it up in review.
   
   * Remove more throwIfNotOk uses from API calls
   
   These are unneeded as an error response will already throw an error to
   be handled at the call site.
   
   * Display an error toaster on newsfeed fetch failure
   
   * Remove dead code
   
   This was left over as a result of elastic#56261
   
   * Remove throwIfNotOk from case API calls
   
   Again, not needed because the client already throws.
   
   * Update use_get_tags for NP
   
   * Gets rid of throwIfNotOK usage
   * uses core http fetch
   
   * Remove throwIfNotOk from signals API
   
   * Remove throwIfNotOk
   
   This served the same purpose as errorToToaster, but in a less robust
   way. All usages have been replaced, so now we say goodbye.
   
   * Remove custom errors in favor of KibanaApiError and isApiError type predicate
   
   There was no functional difference between these two code paths, and
   removing these custom errors allowed us to delete a bunch of associated
   code as well..
   
   * Fix test failures
   
   These were mainly related to my swapping any remaining fetch calls with
   the core router as good kibana denizens should :salute:
   
   * Replace use of core mocks with our simpler local ones
   
   This is enough to get our tests to pass. We can't use the core mocks for
   now since there are circular dependencies there, which breaks our build.
   
   * add signal api unit tests
   
   * privilege unit test api
   
   * Add unit tests on the signals container
   
   * Refactor signals API tests to use core mocks
   
   * Simplifies our mocking verbosity by leveraging core mocks
   * Simplifies test setup by isolating a reference to our fetch mock
   * Abstracts response structure to pure helper functions
   
   The try/catch tests had some false positives in that nothing would be
   asserted if the code did not throw an error. These proved to be masking
   a gap in coverage for our get/create signal index requests, which do not
   leverage `throwIfNotOk` but instead rely on the fetch to throw an error;
   once that behavior is verified we can update those tests to have our
   fetchMock throw errors, and we should be all set.
   
   * Simplify signals API tests now that the subjects do less
   
   We no longer re-throw errors, or parse the response, we just return the
   result of the client call. Simple!
   
   * Simplify API functions to use implict returns
   
   When possible. Also adds missing error-throwing documentation where
   necessary.
   
   * Revert "Display an error toaster on newsfeed fetch failure"
   
   This reverts commit 6421322.
   
   * Error property is readonly
   
   * Pull uuid generation into default argument value
   
   * Fix type predicate isApiError
   
   Uses has to properly inspect our errorish object. Turns out we have a
   'message' property, not an 'error' property.
   
   * Fix test setup following modification of type predicate
   
   We need a message (via new Error), a body.message, and a
   body.status_code to satisfy isApiError.
   
   Co-authored-by: Xavier Mouligneau <189600+XavierM@users.noreply.github.com>
   Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

   

   3 people committed Mar 10, 2020
   Configuration menu

   • View commit details
   • Copy full SHA for 51b93fb
   • Browse repository at this point

   Copy the full SHA

   51b93fb View commit details

   Browse the repository at this point in the history