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

Backups: subcommands, prefix-filtering, parsing #313

Merged
merged 14 commits into from
Dec 15, 2023
Merged

Conversation

jzelinskie
Copy link
Member

@jzelinskie jzelinskie commented Dec 8, 2023

This change

  • reorganizes backup functionality to match the format of the rest of the zed CLI's zed $noun $verb structure
  • adds a --prefix-filter flag to commands to filter schemas and relationships from backup/restore/parsing
  • adds zed backup parse-schema, zed backup parse-revision, zed backup parse-relationships commands to inspect backup files

@jzelinskie jzelinskie requested a review from jakedt December 8, 2023 16:44
@jzelinskie jzelinskie added the area/CLI Affects the command line label Dec 8, 2023
jakedt
jakedt previously requested changes Dec 8, 2023
Copy link
Member

@jakedt jakedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some questions.

internal/cmd/backup.go Outdated Show resolved Hide resolved

func hasRelPrefix(rel *v1.Relationship, prefix string) bool {
// Skip any relationships without the prefix on either side.
return strings.HasPrefix(rel.Resource.ObjectType, prefix) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we want to do this for either side, or if we only want to include relationships that originate on the prefix side...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way you need the definition, right? I wonder if there's something from SpiceDB's reachability analysis that could be used here for maximum correctness.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but since this is the relationships filter, you might be missing the definitions for things that are referenced... e.g. a:b#view->prefix/c:d, you won't necessarily have the definition for a

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the typesystem to guarantee that our generated schema is comprehensive, this means that if there was any non-prefixed dependencies, we would throw a failure.

We can circle back later and support for discovering the dependencies.

internal/cmd/backup.go Show resolved Hide resolved
@jzelinskie jzelinskie force-pushed the backup-prefix branch 2 times, most recently from 4869932 to 8403b5d Compare December 8, 2023 22:16
jzelinskie and others added 2 commits December 8, 2023 17:31
if one of the batches sent fails, zed will hide
the underlying error and just report EOF.

The gRPC backend will report EOF as part of an error,
which will have to eb retrieved via CloseAndRecv().

A common error that surfaced as EOF was duplicate
relationships after reimporting the same backup file.

Closes #310.

Co-authored-by: Víctor Roldán Betancort <vroldanbet@authzed.com>
also does some minor refactoring on the function to
make it testable and easier to follow
also DRY creation of decoder, and align
error handling of io.Closer values
Copy link
Contributor

@vroldanbet vroldanbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one question, otherwise LGTM

schema = string(missingAllowedTypes.ReplaceAll([]byte(schema), []byte("\n/* deleted missing allowed type error */")))
schema = string(shortRelations.ReplaceAll([]byte(schema), []byte("\n/* deleted short relation name */")))

compiledSchema, err := compiler.Compile(compiler.InputSchema{Source: "schema", SchemaString: schema}, compiler.SkipValidation())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oddly, we skip validation on the input but we do not do it on the output. That's going to cause the call to fail.

E.g. your input schema is invalid because the prefix is too short. It will be successfully compiled, but after filtering the second compilation pass will fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not block on this since at least is a safe strategy. I think the scenario I reproduced with a test was precisely what the regexes above fix, so it may be difficult to actually make it err

func writeRelationshipsFromArgsOrStdin(cmd *cobra.Command, args []string) error {
if ok := isArgsViaFile(os.Stdin) && len(args) == 0; ok {
return nil
func StdinOrExactArgs(n int) cobra.PositionalArgs {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i should probably throw this into cobrautil, haha

internal/cmd/backup.go Show resolved Hide resolved
@vroldanbet vroldanbet enabled auto-merge December 15, 2023 14:55
@jakedt jakedt dismissed their stale review December 15, 2023 15:22

victor is taking over review

@vroldanbet vroldanbet added this pull request to the merge queue Dec 15, 2023
@vroldanbet vroldanbet removed this pull request from the merge queue due to a manual request Dec 15, 2023
@vroldanbet vroldanbet added this pull request to the merge queue Dec 15, 2023
Merged via the queue into main with commit 4356522 Dec 15, 2023
15 checks passed
@vroldanbet vroldanbet deleted the backup-prefix branch December 15, 2023 16:08
@github-actions github-actions bot locked and limited conversation to collaborators Dec 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/CLI Affects the command line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants