Validate input for CrudEntry.fromRow explicitly

[LucDeCaf]

As suggested in #137.

[LucDeCaf]

@simolus3 Here's the new branch - sorry for the move, I'm a bit of a stickler for accurate branch names

[rkistner]

I think this is not going to change the issue fundamentally - maybe the error will be a little clear, but in the reported scenario the user will still end up with an upload queue being "stuck".

What I meant by "validation" for this is we need to validate the type of the field when writing, which is when the mismatch occurs. The best place for that is probably in the triggers, such as here.

[LucDeCaf]

Okay, will try that out and update PR later

[rkistner]

I discussed with @simolus3 - The additional casts here are still good to have, we just additionally need validations when writing.

[LucDeCaf]

@rkistner Should I make a PR in the sqlite repo as well then?

[LucDeCaf]

I have two ideas - either to CAST NEW.id to a string or to check if NEW.id matches a regex that corresponds to UUIDv4 (I believe that is the only supported format for Id's right now)

The regex I believe will work is this (assuming powersync-sqlite-core supports REGEXP):

/[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-4[a-fA-F0-9]{3}-[89abAB][a-fA-F0-9]{3}-[a-fA-F0-9]{12}/

[rkistner]

We support any TEXT ids, not only UUIDs (although we do recommend UUIDs for most use cases). I think validating that the id is TEXT is the best route here (probably using the sqlite typeof function), since automatically casting could cause unexpected behavior for developers down the line.

[LucDeCaf]

Right, I remember that somewhere in the back of my mind. I agree, auto-casting would be confusing - rather just raise an error.

I imagine something like this?

SELECT CASE
      WHEN (NEW.id IS NULL)
      THEN RAISE (FAIL, 'id is required')
      WHEN (typeof(NEW.id) != 'text')
      THEN RAISE (FAIL, 'id should be text')
      END;

[simolus3]

I'm a bit of a stickler for accurate branch names

Then it looks like I've just ruined it by pushing applying the same principle to the entire powersync_core package.

I think I got the explicit casts right - unit tests pass and I've also tested these changes against the supabase-todolist demo with an updated worker.

[rkistner]

Right, I remember that somewhere in the back of my mind. I agree, auto-casting would be confusing - rather just raise an error.

I imagine something like this?

SELECT CASE
      WHEN (NEW.id IS NULL)
      THEN RAISE (FAIL, 'id is required')
      WHEN (typeof(NEW.id) != 'text')
      THEN RAISE (FAIL, 'id should be text')
      END;

Yes, that should work