Fix sync breaking when an invalid filterId is in localStorage #228
Conversation
pik
force-pushed
the
bug-invalid-filter
branch
2 times, most recently
from
6ff09df to
988e32a
Compare
| @@ -2396,6 +2396,16 @@ MatrixClient.prototype.getOrCreateFilter = function(filterName, filter) { | |||
| // debuglog("Existing filter ID %s: %s; new filter: %s", | |||
| // filterId, JSON.stringify(oldDef), JSON.stringify(newDef)); | |||
| return; | |||
| }, function(error) { | |||
| // {errcode: "M_UNKNOWN", name: "M_UNKNOWN", message: "No row found", data: Object, httpStatus: 404} | |||
| if (error.httpStatus === 404 && (error.errcode === "M_UNKNOWN" || error.errcode === "M_NOT_FOUND")) { | |||
There was a problem hiding this comment.
Updated as discussed to use errcode instead of message.
pik
force-pushed
the
bug-invalid-filter
branch
from
988e32a to
c8f5c4e
Compare
| // {errcode: "M_UNKNOWN", name: "M_UNKNOWN", message: "No row found", data: Object, httpStatus: 404} | ||
| if (error.httpStatus === 404 && (error.errcode === "M_UNKNOWN" || error.errcode === "M_NOT_FOUND")) { | ||
| // Clear existing filterId if the localStorage value is out of sync with the server | ||
| self.store.setFilterIdByName(filterName, null); |
There was a problem hiding this comment.
I don't think this is necessary, since we're about to set it to something else below.
(if it were necessary, the 'out of sync' case would need updating too.)
There was a problem hiding this comment.
Yeah but the createFilter could fail and we might retry the whole thing, imho it makes sense to undefined it out if it's definitely been deleted.
| // Clear existing filterId if the localStorage value is out of sync with the server | ||
| self.store.setFilterIdByName(filterName, null); | ||
| // Return a null value for existingId further down the promise chain | ||
| return null; |
There was a problem hiding this comment.
should probably return undefined, for consistency with the 'out of sync' case and the 'unknown filter name' case.
(though I don't know why the out of sync case has an explicit return rather than just letting flow reach the end of the function.)
pik
force-pushed
the
bug-invalid-filter
branch
from
c8f5c4e to
877a0ec
Compare
* if getFilter fails for a filterId, null out the localStorage id and redirect to the createFilter path * add spec * fix unit/matrix-client.spec.js http response not matching synapse
pik
force-pushed
the
bug-invalid-filter
branch
from
877a0ec to
828c7ba
Compare
|
Thanks! |
Currently it keeps retrying at
keepAlivewithout invalidating: https://github.com/matrix-org/matrix-js-sdk/blob/master/lib/sync.js#L386 (Probably this should actually check whether it is an error that can be recovered through retry or not, rather than looping endlessly?)Here is what a response looks like (note that httpStatus was missing from the mock response in the beforeEach() setup of the spec):