Fix a regression in RB.apiCall and add crucial docs and testing.
Review Request #12473 — Created July 14, 2022 and submitted
A recent change (29fa166) to
RB.apiCall()
altered the order in which
we registered built-in and caller-supplied callbacks, in an effort to
fix a bug where acomplete
handler was getting overridden. Theerror
andcomplete
handlers` were changed to always take precedence over any
caller-supplied ones.This code is old and complicated, and most API calls go through it. To
aim for a lack of regressions, our code was audited to ensure we knew
exactly when and how we were providing these handlers.This change ultimately ended up being the right move for the
complete
handler, but not forerror
. The reason is that ourerror
handler
tries to be "smart".For instance, an HTTP 500 or a 404 would be an error if the caller was
expecting a 200. Normally, an error is anything unexpected that leads to
a failure. However, in a REST API, error codes don't always indicate
failures. They sometimes indicate status.An example is when we request a review request draft. If missing, we get
a 404. This tells us a draft needs to be created. It's not, however, an
"error" in the way that jQuery, Backbone, etc. think of it. It's an
expected result.
RB.apiCall()
was built around this idea. It tried to determine what's
an HTTP error and what's an API error. If it saw an API error payload,
it was considered not to be an API error in the same way, and would hand
the payload to the caller viasuccess()
in order to further analyze
it.This default error handler also took care of showing the "Server Error"
banner (which, notably, does not appear if overridingerror
).This was all the default behavior for all AJAX calls before we moved
to Backbone, and still applies to any place where we manually call
RB.apiCall()
.With Backbone, calls go through
Backbone.sync
->Backbone.ajax
->
RB.apiCall
. As part of any call, it will always pass insuccess
and
error
handlers. Models (such as resource classes) operating this way
handled success and error results separately, instead of API errors
going into the success handler.That brings us to the regression.
The recent fix made our
error
handler run unconditionally. It would
see an API error payload and turn it into asuccess
call. While fine
for manual callers ofRB.apiCall()
, any Backbone models would find
theirsuccess
handlers being called with API error payloads, breaking
all sorts of state.This is all due for a major rethink (for instance, Backbone models can
never trigger the useful "Server Error" banner).However, for now, the fix is pretty clear: Undo the part of the change
where the built-inerror
handler was promoted to being the main one.
We can still keepcomplete
as the primary, since it doesn't try to
interpret and alter state.Since this is complicated, and we've actually had attempts at fixing
this in the past, a big part of this change is documenting all the
current behavior and adding comprehensive unit tests to ensure we don't
inadvertently regress it with future fixes or cleanups.
Ran through the UI and attempted to trigger API calls. Didn't hit any
issues anywhere.All unit tests pass.
Summary | ID |
---|---|
7b4b18e0383850fade206e16bc071f34e732ec35 |
Description | From | Last Updated |
---|---|---|
Missing semicolon. Column: 6 Error code: W033 |
reviewbot | |
'data' was used before it was defined. Column: 19 Error code: W003 |
reviewbot | |
Missing semicolon. Column: 23 Error code: W033 |
reviewbot | |
Missing semicolon. Column: 23 Error code: W033 |
reviewbot | |
Typo "default" -> "defaults" |
maubin | |
We don't need the first {} anymore. |
david | |
"a HTTP" -> "an HTTP" |
david | |
Can we move this to be before the { so it's at the same level as the comment for options? |
david |
- Change Summary:
-
- Pre-defined a variable to avoid a JSHint warning.
- Added missing semicolons.
- Commits:
-
Summary ID cb06e187843ab7314d96d7da0fced74310b02410 775f1a3386a3200068352911dafaa332035ebd70
Checks run (2 succeeded)
- Change Summary:
-
Fixed a typo in a docstring:
default
->defaults
- Commits:
-
Summary ID 775f1a3386a3200068352911dafaa332035ebd70 46ef2ea71f23ae04b7a61055ebcbcea353e19da4
Checks run (2 succeeded)
- Change Summary:
-
Updated the option-building code to be a lot more readable and maintainable, and to avoid some weirdness with indentation levels and comment placement.
Unit tests and original repro case have been re-tested.
- Commits:
-
Summary ID 46ef2ea71f23ae04b7a61055ebcbcea353e19da4 7b4b18e0383850fade206e16bc071f34e732ec35