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

Optionally return operation statuses on WriteRelationships #1903

Open benvernier-sc opened 1 month ago

benvernier-sc commented 1 month ago

Problem Statement

The SpiceDB docs cover a Two writes & commits scenario to do dual writes between an existing system and SpiceDB. Those docs suggest performing a WriteRelationships call as part of the primary write path for the data, then performing a DeleteRelationships request if a rollback if necessary.

The issue is that if I had a relationship that already existed, I performed a WriteRelationships request with a TOUCH operation on that existing relationship (which would have been a no-op internally) then want to rollback my operation, calling DeleteRelationships would actually leave my system in a different state than what it was in before I started the operation.

Similarly, if one of the operations of my WriteRelationships request was a DELETE operation but the relationship didn't actually exist in SpiceDB, I do not want to create the relationship on rollback as once again this would leave the system in a different state.

Solution Brainstorm

When making a call to WriteRelationships (with either a single or multiple updates), there are cases where it would be useful to be able to get what the status of each operation was in the response.

In particular, knowing whether a TOUCH operation actually created a relationship or if it was a no-op, and knowing whether a DELETE operation actually deleted a relationship or if it was a no-op.

I acknowledge that systematically returning that information might come at a performance cost that existing use cases that don't need this information might not be comfortable with, so this could be an option set on the request to specify whether the status breakdown should be returned.

benvernier-sc commented 1 month ago

@josephschorr I did a quick PoC for this:

Let me know your thoughts, if you're happy with the approach I can extend the implementation to cover the other datastores and actually raise a PR for the change.

josephschorr commented 1 month ago

@benvernier-sc Seems reasonable at first glance, although we'd need to clean up a bit the conditionals used around whether to add the RETURNING on not.

I did a quick check through the other datastores:

benvernier-sc commented 1 month ago

although we'd need to clean up a bit the conditionals used around whether to add the RETURNING on not.

Did you have something specific in mind? The way I could see to clean it up would be to do something like

columnsToReturn := []string{
    colNamespace,
    colObjectID,
    colRelation,
    colUsersetNamespace,
    colUsersetObjectID,
    colUsersetRelation,
}

if returnStatus {
    columnsToReturn = append(columnsToReturn,
        colCaveatContextName,
        colCaveatContext,
    )
}

builder := deleteTuple.
        Where(deleteClauses).
        Suffix(fmt.Sprintf("RETURNING %s", strings.Join(",", columnsToReturn)))

Spanner: ⚠️ PROBLEM - I'm unsure if we can do it in Spanner, which presents a problem around API compatibility. This will require more research

From a quick look, it does seem like it's going to be hard to do in Spanner with the current BufferWrite as we don't get any information back. There would be a way to achieve it by using SQL in a spanner.Statement and executing it through rwt.spannerRWT.Query() that returns a *spanner.RowIterator, but that would be a pretty significant refactor (some official examples). It also then leads to the question of whether we always use Query, or if we continue using BufferWrite when we don't need the statuses and use Query when we do, but that becomes two separate implementations to maintain over time.