fix: Handle 400 errors as unrecoverable #1880
Conversation
| @@ -236,7 +241,7 @@ defmodule Electric.Plug.ServeShapePlug do | |||
| # thus the shape ID does not match the shape definition | |||
| # and we return a 400 bad request status code | |||
| conn | |||
| |> send_resp(400, @must_refetch) | |||
| |> send_resp(400, @shape_definition_mismatch) | |||
There was a problem hiding this comment.
Why isn't this a 409? If the shape definition is valid perhaps they just have a really old shape_id.
There was a problem hiding this comment.
Where people were hitting this problem (myself included) is when we'd request an invalid shape — e.g. the table didn't exist.
There was a problem hiding this comment.
This is the case where the shape_id is valid and exists (so there is an active shape that matches it), but the provided shape definition does not match that. That is a bad request and cannot be resolved with a must-refetch.
You need to:
- remove the
shape_idso we start serving you the shape that matches the shape definition provided, OR - provide the correct shape definition that matches this active
shape_idthat you are providing
There was a problem hiding this comment.
Hmm no — when a shape is deleted on the server (for whatever reason), the next request for that shape would re-create the shape with a new shape_handle — then the next person to request the shape would have a mis-match between their shape_handle and the shape definition because their handle is pointing to a shape log that no longer exists. Which isn't a problem as a new shape now exists so they just need to refetch it.
This is the main use case for must-refetch.
400 is for a bad shape definition e.g. we can't parse the where clause or whatever.
There was a problem hiding this comment.
(re: shape_handle #1771)
There was a problem hiding this comment.
Oh ok — yeah this shouldn't happen unless the client has a bug.
Did you check the "request a shape where the table doesn't exist"?
There was a problem hiding this comment.
So nice seeing all those green checkboxes now!
There was a problem hiding this comment.
(see the integration test I added to the client)
when there is a mismatch, there are two cases:
- the
shape_idno longer exists, so we return a 409 with the new shape handle for the given definition - the
shape_idexists and matches a shape definition that is not the provided one in the URL, so we return a 400
There was a problem hiding this comment.
Did you check the "request a shape where the table doesn't exist"?
This case will return a 400 at the shape validation stage - I didn't add an integration test for this but as far as the client is concerned right now any 400 will terminate the stream so we should be fine
Fixes #1876 The issue was that we were sending `must-refetch` messages with some of our 400s, although they were unrecoverable and re-fetching would trigger an infinite loop. I've made the server send 400 errors for the cases where the `shape_id` and `shape_definition` do not match, with an explanatory message, and I've made the client terminate when receiving such an error. As a temporary measure, the error is not thrown into the ether but stored in the `ShapeStream` (accessible via `stream.error`). We have had some discussions with @balegas about making errors more "handleable" by changing our approach to the `ShapeStream` instantiation. I think this warrants opening a new issue and corresponding PR to discuss more thoroughly.
Fixes #1876
The issue was that we were sending
must-refetchmessages with some of our 400s, although they were unrecoverable and re-fetching would trigger an infinite loop.I've made the server send 400 errors for the cases where the
shape_idandshape_definitiondo not match, with an explanatory message, and I've made the client terminate when receiving such an error.As a temporary measure, the error is not thrown into the ether but stored in the
ShapeStream(accessible viastream.error).We have had some discussions with @balegas about making errors more "handleable" by changing our approach to the
ShapeStreaminstantiation. I think this warrants opening a new issue and corresponding PR to discuss more thoroughly.