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

feat: Multiple schema file support in schema add #3352

Merged
merged 12 commits into from
Jan 17, 2025
42 changes: 29 additions & 13 deletions cli/schema_add.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
)

func MakeSchemaAddCommand() *cobra.Command {
var schemaFile string
var schemaFiles []string
var cmd = &cobra.Command{
Use: "add [schema]",
Short: "Add new schema",
Expand All @@ -36,40 +36,56 @@
Example: add from file:
defradb client schema add -f schema.graphql

Example: add from multiple files:
defradb client schema add -f schema1.graphql -f schema2.graphql
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: IMO it would be nice to standardise this, so that all file-based inputs can be expected to work the same way.


Example: add from stdin:
cat schema.graphql | defradb client schema add -

Learn more about the DefraDB GraphQL Schema Language on https://docs.source.network.`,
RunE: func(cmd *cobra.Command, args []string) error {
store := mustGetContextStore(cmd)

var schema string
var combinedSchema string
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: This is a nice and simple way of ensure the whole operation takes place in a single txn.

thought: This does introduce a difference between the 3 clients - it might be nicer for users if instead we changed the other 2 to accept multiple strings. You would then be able to test this using integration tests instead of CLI specific stuff.

I think there is also an argument to be had that without changing the other 2 clients, the CLI should not have this feature, and instead users can just use cat/type in their terminal.

switch {
case schemaFile != "":
data, err := os.ReadFile(schemaFile)
if err != nil {
return err
case len(schemaFiles) > 0:
// Read schemas from files and concatenate them
for _, schemaFile := range schemaFiles {
data, err := os.ReadFile(schemaFile)
if err != nil {
return fmt.Errorf("failed to read file %s: %w", schemaFile, err)
Copy link
Member

Choose a reason for hiding this comment

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

todo: please move the errors to the cli/errors.go file.

}
combinedSchema += string(data) + "\n"

Check warning on line 58 in cli/schema_add.go

View check run for this annotation

Codecov / codecov/patch

cli/schema_add.go#L51-L58

Added lines #L51 - L58 were not covered by tests
}
schema = string(data)

case len(args) > 0 && args[0] == "-":
// Read schema from stdin

Check warning on line 62 in cli/schema_add.go

View check run for this annotation

Codecov / codecov/patch

cli/schema_add.go#L62

Added line #L62 was not covered by tests
data, err := io.ReadAll(cmd.InOrStdin())
if err != nil {
return err
return fmt.Errorf("failed to read schema from stdin: %w", err)

Check warning on line 65 in cli/schema_add.go

View check run for this annotation

Codecov / codecov/patch

cli/schema_add.go#L65

Added line #L65 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

todo: please move the errors to the cli/errors.go file.

}
schema = string(data)
combinedSchema += string(data) + "\n"

Check warning on line 67 in cli/schema_add.go

View check run for this annotation

Codecov / codecov/patch

cli/schema_add.go#L67

Added line #L67 was not covered by tests

case len(args) > 0:
schema = args[0]
// Read schema from argument string
combinedSchema += args[0] + "\n"

default:
return fmt.Errorf("schema cannot be empty")
}

cols, err := store.AddSchema(cmd.Context(), schema)
// Process the combined schema
cols, err := store.AddSchema(cmd.Context(), combinedSchema)
if err != nil {
return fmt.Errorf("failed to add schema: %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

todo: please move the errors to the cli/errors.go file.

}
if err := writeJSON(cmd, cols); err != nil {
return err
}
return writeJSON(cmd, cols)

return nil
},
}
cmd.Flags().StringVarP(&schemaFile, "file", "f", "", "File to load a schema from")
cmd.Flags().StringSliceVarP(&schemaFiles, "file", "f", []string{}, "File(s) to load schema from")
return cmd
}
7 changes: 5 additions & 2 deletions docs/website/references/cli/defradb_client_schema_add.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ Example: add from an argument string:
Example: add from file:
defradb client schema add -f schema.graphql

Example: add from multiple files:
defradb client schema add -f schema1.graphql -f schema2.graphql

Example: add from stdin:
cat schema.graphql | defradb client schema add -

Expand All @@ -29,8 +32,8 @@ defradb client schema add [schema] [flags]
### Options

```
-f, --file string File to load a schema from
-h, --help help for add
-f, --file strings File(s) to load schema from
Copy link
Member

@shahzadlone shahzadlone Jan 16, 2025

Choose a reason for hiding this comment

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

thought: saying strings over string kind of signals to me that this is valid:

  1. defradb client schema add -f "schema1.graphql" "schema2.graphql"

rather than:

  1. defradb client schema add -f "schema1.graphql" -f "schema2.graphql"

non-blocking: because maybe it might just be me

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer your suggestion, but I have a vague memory of the team having this discussion and deciding on the multiple -fs. I hope I am wrong though :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have an opinion on this, but not a strong one. Happy to go with the team consensus (or majority opinion.)

Copy link
Member

@shahzadlone shahzadlone Jan 16, 2025

Choose a reason for hiding this comment

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

I don't have a strong prefer at all on the approach we take. My thought was only on the changed documentation in this PR, I think it should say string instead of strings if we are going with the approach introduced in this PR (2. above).

Happy with strings terminology if we are switching to 1. above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I see where you are coming from now. Hmm... I think I agree. I will be changing that to the singular as you suggest.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry commented on the main PR thread, can continue here if you like: #3352 (comment)

-h, --help help for add
```

### Options inherited from parent commands
Expand Down
Loading