feat: Multiple schema file support in schema add #3352

ChrisBQu · 2025-01-02T15:59:53Z

Relevant issue(s)

Resolves #1248

Description

Previously, a single schema file could be added by calling defradb schema add -f filename. Now we can add multiple new schemas from different files in the following way: defradb schema add -f file_one -f file_two -f file_three

This PR also adjusts some import statements that were using the incorrect error import.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Specify the platform(s) on which this was tested:

Windows

codecov · 2025-01-02T18:38:15Z

Codecov Report

Attention: Patch coverage is 50.00000% with 17 lines in your changes missing coverage. Please review.

Project coverage is 78.45%. Comparing base (10e0f38) to head (25f533d).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
cli/schema_add.go	53.57%	12 Missing and 1 partial ⚠️
cli/errors.go	33.33%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3352      +/-   ##
===========================================
- Coverage    78.46%   78.45%   -0.01%     
===========================================
  Files          392      392              
  Lines        35688    35707      +19     
===========================================
+ Hits         28001    28013      +12     
- Misses        6054     6059       +5     
- Partials      1633     1635       +2

Flag	Coverage Δ
all-tests	`78.45% <50.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cli/errors.go	`9.09% <33.33%> (+9.09%)`	⬆️
cli/schema_add.go	`68.25% <53.57%> (-3.75%)`	⬇️

... and 13 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10e0f38...25f533d. Read the comment docs.

cli/config.go

AndrewSisley

question: Why is this a draft PR?

todo: Please add some tests.

AndrewSisley · 2025-01-13T16:47:33Z

cli/schema_add.go

@@ -36,40 +36,56 @@ 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


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

AndrewSisley · 2025-01-13T16:55:51Z

cli/schema_add.go

 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


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.

shahzadlone · 2025-01-16T14:46:12Z

cli/schema_add.go

+				for _, schemaFile := range schemaFiles {
+					data, err := os.ReadFile(schemaFile)
+					if err != nil {
+						return fmt.Errorf("failed to read file %s: %w", schemaFile, err)


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

shahzadlone · 2025-01-16T14:46:26Z

cli/schema_add.go

 				data, err := io.ReadAll(cmd.InOrStdin())
 				if err != nil {
-					return err
+					return fmt.Errorf("failed to read schema from stdin: %w", err)


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

shahzadlone · 2025-01-16T14:46:33Z

cli/schema_add.go

 			if err != nil {
+				return fmt.Errorf("failed to add schema: %w", err)


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

shahzadlone · 2025-01-16T14:49:41Z

docs/website/references/cli/defradb_client_schema_add.md

@@ -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


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

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

rather than:

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

non-blocking: because maybe it might just be me

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 :)

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

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.

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.

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

AndrewSisley

LGTM, thanks Chris!

Approving now, on condition that:

Shahzad is happy with the f flag docs
You have manually tested this with multiple SDLs

shahzadlone

Thanks for taking care of this, don't forget to change to string as I still see it as strings still.

Also please do make an issue to do a similar change for other file arguments in other cli commands (like Andy suggested) if they won't be done in this PR.

shahzadlone · 2025-01-16T16:56:51Z

cli/errors.go

+func NewErrFailedToReadSchemaFile(schemaFile string, inner error) error {
+	return errors.Wrap(fmt.Sprintf("failed to read file %s", schemaFile), inner)
+}
+
+func NewErrFailedToReadSchemaFromStdin(inner error) error {
+	return errors.Wrap("failed to read schema from stdin", inner)
+}
+
+func NewErrFailedToAddSchema(inner error) error {
+	return errors.Wrap("failed to add schema", inner)
+}


praise: Thanks for doing this suggestion :)

shahzadlone · 2025-01-16T18:40:19Z

thought/question:

@ChrisBQu

Looking at the wayStringSliceVarP is used, and finding out that the change from string to strings was an autogenerated thing.

I have a hunch that both the -f with list of strings and multiple -fs will both work?

I haven't tested, but I suspect that:

defradb client schema add -f="schema1.graphql,schema2.graphql" -f="schema3.graphql"

Will result in all 3 files being loaded?

Meaning that this:

defradb client schema add -f="schema1.graphql,schema2.graphql"

Also works? Have you tested this? (so 1 and 2 both work in a way from my previous comment?)

Sorry for the bother if this assumption of mine is incorrect.

Got the hunch after reading this: https://github.com/spf13/pflag/blob/v1.0.5/string_slice.go#L120

ChrisBQu · 2025-01-16T20:53:24Z

thought/question:

@ChrisBQu

Looking at the wayStringSliceVarP is used, and finding out that the change from string to strings was an autogenerated thing.

I have a hunch that both the -f with list of strings and multiple -fs will both work?

I haven't tested, but I suspect that:
defradb client schema add -f="schema1.graphql,schema2.graphql" -f="schema3.graphql"
Will result in all 3 files being loaded?

Meaning that this:
defradb client schema add -f="schema1.graphql,schema2.graphql"
Also works? Have you tested this? (so 1 and 2 both work in a way from my previous comment?)

Sorry for the bother if this assumption of mine is incorrect.

Got the hunch after reading this: https://github.com/spf13/pflag/blob/v1.0.5/string_slice.go#L120

This is correct. I did test it. In other words: separating by a comma on a single -f flag does work.

And if both work, maybe the plural of strings is desired(?)

shahzadlone · 2025-01-16T21:00:39Z

This is correct. I did test it. In other words: separating by a comma on a single -f flag does work.

And if both work, maybe the plural of strings is desired(?)

Ok perfect. Then in that case please add an example of this case in the cli command documentation demonstrating both these cases. And strings makes sense then.

ChrisBQu added 2 commits December 31, 2024 10:49

Partial work

74331e3

First commit

e4c55ad

ChrisBQu added feature New feature or request area/cli Related to the CLI binary labels Jan 2, 2025

ChrisBQu added this to the DefraDB v0.16 milestone Jan 2, 2025

ChrisBQu self-assigned this Jan 2, 2025

ChrisBQu added 2 commits January 2, 2025 11:02

Rolled back changes to some files made accidentally

746e13e

Delint

e006561

ChrisBQu added 3 commits January 3, 2025 11:46

Redo MakeSchemaAddCommand

9cb6ecd

Delint

58e71f5

Make docs

15b8d22

ChrisBQu requested a review from a team January 6, 2025 16:00

shahzadlone reviewed Jan 7, 2025

View reviewed changes

cli/config.go Outdated Show resolved Hide resolved

ChrisBQu marked this pull request as ready for review January 13, 2025 16:47

AndrewSisley requested changes Jan 13, 2025

View reviewed changes

Revert error import lines

80a507b

shahzadlone reviewed Jan 16, 2025

View reviewed changes

ChrisBQu added 2 commits January 16, 2025 10:17

Change hardcoded errors to error types

a1740f4

Changed docs

88159c4

AndrewSisley approved these changes Jan 16, 2025

View reviewed changes

shahzadlone approved these changes Jan 16, 2025

View reviewed changes

ChrisBQu added 2 commits January 17, 2025 11:03

Doc update

943173a

Merge branch 'develop' into issue-1248

25f533d

ChrisBQu merged commit 3779cbb into sourcenetwork:develop Jan 17, 2025
42 of 43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Multiple schema file support in schema add #3352

feat: Multiple schema file support in schema add #3352

ChrisBQu commented Jan 2, 2025 •

edited

Loading

codecov bot commented Jan 2, 2025 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Jan 13, 2025

AndrewSisley Jan 13, 2025

shahzadlone Jan 16, 2025

shahzadlone Jan 16, 2025

shahzadlone Jan 16, 2025

shahzadlone Jan 16, 2025 •

edited

Loading

AndrewSisley Jan 16, 2025

ChrisBQu Jan 16, 2025

shahzadlone Jan 16, 2025 •

edited

Loading

ChrisBQu Jan 16, 2025

shahzadlone Jan 16, 2025

AndrewSisley left a comment

shahzadlone left a comment

shahzadlone Jan 16, 2025

shahzadlone commented Jan 16, 2025 •

edited

Loading

ChrisBQu commented Jan 16, 2025

shahzadlone commented Jan 16, 2025

		if err != nil {
		return fmt.Errorf("failed to add schema: %w", err)

feat: Multiple schema file support in schema add #3352

feat: Multiple schema file support in schema add #3352

Conversation

ChrisBQu commented Jan 2, 2025 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Jan 2, 2025 • edited Loading

Codecov Report

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone commented Jan 16, 2025 • edited Loading

ChrisBQu commented Jan 16, 2025

shahzadlone commented Jan 16, 2025

ChrisBQu commented Jan 2, 2025 •

edited

Loading

codecov bot commented Jan 2, 2025 •

edited

Loading

shahzadlone Jan 16, 2025 •

edited

Loading

shahzadlone Jan 16, 2025 •

edited

Loading

shahzadlone commented Jan 16, 2025 •

edited

Loading