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: Allow setting of default schema version #1888

Conversation

AndrewSisley
Copy link
Contributor

@AndrewSisley AndrewSisley commented Sep 15, 2023

Relevant issue(s)

Resolves #1884

Description

Allows setting of default schema version, allowing rapid switching between (application) api/database versions. It also allows them to define and apply schema updates eagerly, and then make the switch at a later date.

  • Wait for the 0.7 release to be cut before merging

@AndrewSisley AndrewSisley added feature New feature or request area/query Related to the query component area/schema Related to the schema system labels Sep 15, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.8 milestone Sep 15, 2023
@AndrewSisley AndrewSisley self-assigned this Sep 15, 2023
@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Patch coverage: 36.05% and project coverage change: -0.08% ⚠️

Comparison is base (667023e) 70.82% compared to head (9b72197) 70.74%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1888      +/-   ##
===========================================
- Coverage    70.82%   70.74%   -0.08%     
===========================================
  Files          233      233              
  Lines        24232    24295      +63     
===========================================
+ Hits         17160    17186      +26     
- Misses        5901     5937      +36     
- Partials      1171     1172       +1     
Flag Coverage Δ
all-tests 70.74% <36.05%> (-0.08%) ⬇️

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

Files Changed Coverage Δ
http/client.go 0.00% <0.00%> (ø)
http/handler_store.go 0.00% <0.00%> (ø)
http/wrapper.go 0.00% <0.00%> (ø)
db/collection.go 73.07% <57.14%> (+0.50%) ⬆️
db/txn_db.go 59.15% <61.11%> (+0.33%) ⬆️
api/http/handlerfuncs.go 80.35% <100.00%> (ø)
db/schema.go 82.86% <100.00%> (ø)
http/server.go 100.00% <100.00%> (ø)

... and 1 file 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 667023e...9b72197. Read the comment docs.

@AndrewSisley AndrewSisley requested a review from a team September 15, 2023 20:12
Copy link
Member

@nasdf nasdf left a comment

Choose a reason for hiding this comment

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

I left some feedback about the public API portions.

// provided. This includes GQL queries and Collection operations.
//
// It will return an error if the provided schema version ID does not exist.
SetDefaultSchemaVersion(context.Context, string) error
Copy link
Member

Choose a reason for hiding this comment

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

This was confusing for me at first. What do you think about moving this to the Collection interface instead? I think it would be more clear exactly which Collection you are setting a default for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like that, as if forces users to get a collection object when all they need is the ID, I also like the proximity to PatchSchema.

Copy link
Member

Choose a reason for hiding this comment

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

The simplicity is nice but I can see many users getting confused.

This is the use case I see being used most often when you don't know the schema version id:

cols := db.GetAllCollections()

for _, col := range cols {
  if isMatchingCollection(col) {
    col.SetDefaultSchemaVersion()
  }
}

Copy link
Contributor Author

@AndrewSisley AndrewSisley Sep 21, 2023

Choose a reason for hiding this comment

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

I do disagree :) I see the most common use case to within a migration script:

defradb patchSchema jsonStuff
...
defradb setDefaultSchemaVersion [schemaVersionID] 

if it was on the collection object it would be a bit more complicated no, would look something like the below no?

defradb collection [collectionID] setDefaultSchemaVersion [schemaVersionID] 

Copy link
Member

Choose a reason for hiding this comment

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

It's definitely more verbose but it's also much easier to parse what that command is doing.

No need to block the PR. I think we could add the shortcut function in the future if users have trouble with it.

@@ -109,7 +110,16 @@ type Store interface {
//
// Field [FieldKind] values may be provided in either their raw integer form, or as string as per
// [FieldKindStringToEnumMapping].
PatchSchema(context.Context, string) error
PatchSchema(context.Context, string, bool) error
Copy link
Member

Choose a reason for hiding this comment

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

What are your thoughts on inverting the boolean usage here?

I think it'd be nice when using the http or cli interfaces that the lack of the boolean would indicate the original functionality of setting the default.

Requiring an extra --set-default-schema cli flag or SetDefaultSchema http property would break backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want switching schema versions to be an explicit choice - it is a major, and likely breaking, change to the user's database and I don't want them to do it accidentally via parameter omission.

@@ -418,6 +418,9 @@ func executeTestCase(
case SchemaPatch:
patchSchema(s, action)

case SetDefaultSchemaVersion:
Copy link
Member

Choose a reason for hiding this comment

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

I think we'll want this action included in the change detector setup steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we do. Normal tests will just set the PatchSchema change to default using the bool, tests that use this action are perhaps going to benefit more from having this in the main (post setup) stage, as they are more likely to be testing that SetDefaultSchemaVersion works.

Copy link
Member

@nasdf nasdf left a comment

Choose a reason for hiding this comment

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

Looks good! From the discussion there's a minor API addition that we may revisit if needed.

It will be called from other locations shortly, and may be optional within the original source function.
Will allow users to change their default database schema versions on demand, allowing rapid switching between (application) api/database versions.  It also allows them to define and apply schema updates eagerly, and then make the switch at a later date.
@AndrewSisley AndrewSisley merged commit db1f41c into sourcenetwork:develop Sep 25, 2023
14 checks passed
@AndrewSisley AndrewSisley deleted the 1884-set-default-schema-version branch September 25, 2023 14:49
shahzadlone pushed a commit that referenced this pull request Oct 2, 2023
## Relevant issue(s)

Resolves #1884

## Description

Allows setting of default schema version, allowing rapid switching
between (application) api/database versions. It also allows them to
define and apply schema updates eagerly, and then make the switch at a
later date.
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
## Relevant issue(s)

Resolves sourcenetwork#1884

## Description

Allows setting of default schema version, allowing rapid switching
between (application) api/database versions. It also allows them to
define and apply schema updates eagerly, and then make the switch at a
later date.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/query Related to the query component area/schema Related to the schema system feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow users to set default schema version
2 participants