search

electric-sql / electric

Sync little subsets of your Postgres data into local apps and services.
https://electric-sql.com
Apache License 2.0
6.48k stars 156 forks source link

Fix infinite loop when the typescript client requests a non-existing table #1876

Closed KyleAMathews closed 1 month ago

KyleAMathews commented 1 month ago

The server returns a 400 and this line then restarts the fetching https://github.com/electric-sql/electric/blob/371dc560083f131d10477b7c101cc30a1adcaa08/packages/typescript-client/src/client.ts#L235

It seems 404 is the proper response code for the server and would fix this issue.

thruflo commented 1 month ago

Are there any other cases where a 400 could be returned. Do we also want to fix infinite loop for 400 generally?

KyleAMathews commented 1 month ago

I'm not sure why we special-case the 400 there — 409 is the code we use to tell the client to refetch. In general 4xx codes are a user error so the client should just throw ann error and quit.

But yeah, to your point, we need to do two things — return 404 from the server for non-existing tables and don't special case the 400 status code.

balegas commented 1 month ago

We used a 400 initially on the server to not specialize the reasons. I guess things have evolved.

balegas commented 1 month ago

One related problem is that the constructor makes the first fetch call, so non-actionable errors can't be caught. We should change the client to separate resource creation (i.e. constructor) from connection initialisation (start) to make it easier to catch non-actionable errors.

msfstef commented 1 month ago

We should change the client to separate resource creation (i.e. constructor) from connection initialisation (start) to make it easier to catch non-actionable errors.

I agree, I think we should open a new issue about this in particular.

I've opened https://github.com/electric-sql/electric/pull/1880 that should fix this immediate issue of infinite loops.

As an aside, I don't think we should be returning 404 errors since as far as we're concerned any request that defines a shape that is not valid (inexistent or invalid root table, inexistent or invalid column names, mismatched shape id and definition) are badly formed requests, and thus 400s.

If we want to split out errors (e.g. missing table from PG is 404, invalid table name is 400), it would be perhaps more explicit and informative but the handling from our side would be exactly the same, so it would just be a matter of improving API semantics.