search

authzed / spicedb

Open Source, Google Zanzibar-inspired permissions database to enable fine-grained authorization for customer applications
https://authzed.com/docs
Apache License 2.0
4.71k stars 250 forks source link

PostgreSQL Bulkload writes caveat names as null instead of empty string #1938

Closed heissa83 closed 1 week ago

heissa83 commented 2 weeks ago

What platforms are affected?

linux, macos, windows, others

What architectures are affected?

amd64, arm64, others

What SpiceDB version are you using?

v.1.33.0

Steps to Reproduce

  1. Create Backup with CLI Tool (zed backup create backup.bin)
  2. Restore Backup with CLI Tool (zed backup restore backup.bin)

But this will be the same over the normal bulkimport endpoint aswell.

Expected Result

Writen Relationtuples in the database should be the same as in the source.

Actual Result

The bulk imported tuples without a caveat have the caveatname as null value in the database. But this should be an empty string. This is problematic when we try to update an existing relation with a caveat later on because the sql check (caveat_name <> 'some_caveat_name') fails for a null value.

It looks like the BulkLoad function uses a sql.NullString instead of a simple string, this causes the problem.

josephschorr commented 2 weeks ago

This is problematic when we try to update an existing relation with a caveat later on because the sql check (caveat_name <> 'some_caveat_name') fails for a null value.

Are you making changes directly to the relationships table? If so, this is completely unsupported and can cause inconsistency and/or straight bugs.

heissa83 commented 2 weeks ago

This is problematic when we try to update an existing relation with a caveat later on because the sql check (caveat_name <> 'some_caveat_name') fails for a null value.

Are you making changes directly to the relationships table? If so, this is completely unsupported and can cause inconsistency and/or straight bugs.

No we dont. If we try to update a relationship through the WriteRelationships endpoint, set the update method to Touch and add a caveat to an existing relationship.

This works for a "normally" written relationship, because the caveat_name is an empty string. But if this relation was backuped and restored (or written through the bulk load api) then the caveat_name in the database is null. The SQL created in the code uses the <>operator to check the name and this fails for null values.

josephschorr commented 2 weeks ago

@heissa83 Ah, ok. Just making sure :)