Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for specifying a list of tuples to delete #161

Closed
aaguiarz opened this issue Sep 21, 2023 · 6 comments · Fixed by #165
Closed

Add support for specifying a list of tuples to delete #161

aaguiarz opened this issue Sep 21, 2023 · 6 comments · Fixed by #165
Assignees

Comments

@aaguiarz
Copy link
Member

If we want to delete all the tuples in a store, we can currently doing by running

fga tuple read | jq -r '.tuples[] | "fga tuple delete \(.key.user) \(.key.relation) \(.key.object)"' > delete-tuples.sh
chmod +x delete-tuples.sh
./delete-tuples.sh

Running the CLI to delete each tuple is not efficient, in particular when the OpenFGA store uses OAuth authentication and requires obtaining a token in each call.

We should add an option to get the tuples to delete from a file:

fga tuple delete --file tuples.json

That would be slightly inconsistent with our current fga tuple import option, so we might want to also support

fga tuple write --file tuples.json

and deprecate fga tuple import.

@gabrielbussolo
Copy link
Contributor

Hello @aaguiarz, few questions:

Should be kept the import capabilities of write the tuples in chunks and at the end will report the tuple chunks that failed. for both delete and write?

      --max-parallel-requests int   Max number of requests to issue to the server in parallel. (default 10)
      --max-tuples-per-write int    Max tuples per write chunk. (default 1)

Also the format accepted by import is yaml but on the new version should be json?

@gabrielbussolo
Copy link
Contributor

Case we decide to ignore those features and follow with json, i have a draft for it already linked with the issue 😄

can you assign this issue to me?

@aaguiarz
Copy link
Member Author

I don't see a reason why we wouldn't want to have the same features we have in import for tuple delete...

From that perspective, 'tuple write --file' would be equivalent to tuple import --file, and tuple delete --file would be the same but deleting tuples instead of writing them.

@rhamzeh what do you think?

@rhamzeh
Copy link
Member

rhamzeh commented Sep 25, 2023

@gabrielbussolo

All the commands that accept YAML also accept JSON (though we don't currently document it).

For consistency (and ease of use), let's keep aiming for YAML support.

So both fga tuple write --file tuples.json and fga tuple write --file tuples.yaml should work


@aaguiarz, how about we do a breaking change, allow write/delete to accept an array of tuples (also making it consistent with how contextual tuples in check are written):

# import one or more tuples, consistent with
fga tuple write --tuple "user:anne writer folder:product" --tuple "folder:product parent document:roadmap"

Alongside:

fga tuple write --file tuples.{json|yaml}

And we drop the default fga tuple write "user:anne writer folder:product"


On the other hand, for the future, we need a way to make the CLI work with conditions

@rhamzeh
Copy link
Member

rhamzeh commented Sep 25, 2023

@gabrielbussolo discussed it with @aaguiarz, for this ticket:

  • keep support for
fga tuple write user:X writer document:Y
  • add support for the same interface as [import](
fga tuple write --file tuples.{yaml|json}

This means supporting the same options import currently supports:

fga tuple import --store-id= [--model-id=] --file [--max-tuples-per-write=] [--max-parallel-requests=]

We also need to do the same for fga tuple delete.

We'll create a separate issue for reading from STDIN in write and delete

@gabrielbussolo
Copy link
Contributor

thanks for the explanation @rhamzeh, I opened the PR for review here #165

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants