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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 25, 2023
+656 −40
Merged

feat: Allow setting of default schema version #1888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5897233
Break out setting of schema version to func
AndrewSisley Sep 15, 2023
443a703
Add SetDefaultSchemaVersion
AndrewSisley Sep 15, 2023
9b72197
Allow choice in PatchSchema to make default
AndrewSisley Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion api/http/handlerfuncs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,10 @@ func patchSchemaHandler(rw http.ResponseWriter, req *http.Request) {
return
}

err = db.PatchSchema(req.Context(), string(patch))
// Hardcode setDefault to true here, as that preserves the existing behaviour.
// This function will be ripped out very shortly and I don't think it is worth
// spending time/thought here. The new http api handles this correctly.
err = db.PatchSchema(req.Context(), string(patch), true)
if err != nil {
handleErr(req.Context(), rw, err, http.StatusInternalServerError)
return
Expand Down
14 changes: 12 additions & 2 deletions client/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ type Store interface {
AddSchema(context.Context, string) ([]CollectionDescription, error)

// PatchSchema takes the given JSON patch string and applies it to the set of CollectionDescriptions
// present in the database.
// present in the database. If true is provided, the new schema versions will be made default, otherwise
// [SetDefaultSchemaVersion] should be called to set them so.
//
// It will also update the GQL types used by the query system. It will error and not apply any of the
// requested, valid updates should the net result of the patch result in an invalid state. The
Expand All @@ -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.


// SetDefaultSchemaVersion sets the default schema version to the ID provided. It will be applied to all
// collections using the schema.
//
// This will affect all operations interacting with the schema where a schema version is not explicitly
// 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 •
edited
Loading

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.


// SetMigration sets the migration for the given source-destination schema version IDs. Is equivilent to
// calling `LensRegistry().SetMigration(ctx, cfg)`.
Expand Down
64 changes: 54 additions & 10 deletions client/mocks/db.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

61 changes: 49 additions & 12 deletions db/collection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@
existingDescriptionsByName map[string]client.CollectionDescription,
proposedDescriptionsByName map[string]client.CollectionDescription,
desc client.CollectionDescription,
setAsDefaultVersion bool,
) (client.Collection, error) {
hasChanged, err := db.validateUpdateCollection(ctx, txn, existingDescriptionsByName, proposedDescriptionsByName, desc)
if err != nil {
Expand Down Expand Up @@ -300,24 +301,19 @@
return nil, err
}

collectionSchemaKey := core.NewCollectionSchemaKey(desc.Schema.SchemaID)
err = txn.Systemstore().Put(ctx, collectionSchemaKey.ToDS(), []byte(schemaVersionID))
if err != nil {
return nil, err
}

collectionKey := core.NewCollectionKey(desc.Name)
err = txn.Systemstore().Put(ctx, collectionKey.ToDS(), []byte(schemaVersionID))
if err != nil {
return nil, err
}

schemaVersionHistoryKey := core.NewSchemaHistoryKey(desc.Schema.SchemaID, previousSchemaVersionID)
err = txn.Systemstore().Put(ctx, schemaVersionHistoryKey.ToDS(), []byte(schemaVersionID))
if err != nil {
return nil, err
}

if setAsDefaultVersion {
err = db.setDefaultSchemaVersionExplicit(ctx, txn, desc.Name, desc.Schema.SchemaID, schemaVersionID)
if err != nil {
return nil, err
}

Check warning on line 314 in db/collection.go

View check run for this annotation

Codecov / codecov/patch

db/collection.go#L313-L314 

Added lines #L313 - L314 were not covered by tests
}

return db.getCollectionByName(ctx, txn, desc.Name)
}

Expand Down Expand Up @@ -591,6 +587,47 @@
return false, nil
}

func (db *db) setDefaultSchemaVersion(
ctx context.Context,
txn datastore.Txn,
schemaVersionID string,
) error {
col, err := db.getCollectionByVersionID(ctx, txn, schemaVersionID)
if err != nil {
return err
}

desc := col.Description()
err = db.setDefaultSchemaVersionExplicit(ctx, txn, desc.Name, desc.Schema.SchemaID, schemaVersionID)
if err != nil {
return err
}

Check warning on line 604 in db/collection.go

View check run for this annotation

Codecov / codecov/patch

db/collection.go#L603-L604 

Added lines #L603 - L604 were not covered by tests

cols, err := db.getCollectionDescriptions(ctx, txn)
if err != nil {
return err
}

Check warning on line 609 in db/collection.go

View check run for this annotation

Codecov / codecov/patch

db/collection.go#L608-L609 

Added lines #L608 - L609 were not covered by tests

return db.parser.SetSchema(ctx, txn, cols)
}

func (db *db) setDefaultSchemaVersionExplicit(
ctx context.Context,
txn datastore.Txn,
collectionName string,
schemaID string,
schemaVersionID string,
) error {
collectionSchemaKey := core.NewCollectionSchemaKey(schemaID)
err := txn.Systemstore().Put(ctx, collectionSchemaKey.ToDS(), []byte(schemaVersionID))
if err != nil {
return err
}

Check warning on line 625 in db/collection.go

View check run for this annotation

Codecov / codecov/patch

db/collection.go#L624-L625 

Added lines #L624 - L625 were not covered by tests

collectionKey := core.NewCollectionKey(collectionName)
return txn.Systemstore().Put(ctx, collectionKey.ToDS(), []byte(schemaVersionID))
}

// getCollectionByVersionId returns the [*collection] at the given [schemaVersionId] version.
//
// Will return an error if the given key is empty, or not found.
Expand Down
5 changes: 3 additions & 2 deletions db/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func (db *db) getCollectionDescriptions(
// The collections (including the schema version ID) will only be updated if any changes have actually
// been made, if the net result of the patch matches the current persisted description then no changes
// will be applied.
func (db *db) patchSchema(ctx context.Context, txn datastore.Txn, patchString string) error {
func (db *db) patchSchema(ctx context.Context, txn datastore.Txn, patchString string, setAsDefaultVersion bool) error {
patch, err := jsonpatch.DecodePatch([]byte(patchString))
if err != nil {
return err
Expand Down Expand Up @@ -144,10 +144,11 @@ func (db *db) patchSchema(ctx context.Context, txn datastore.Txn, patchString st
}

for i, desc := range newDescriptions {
col, err := db.updateCollection(ctx, txn, collectionsByName, newDescriptionsByName, desc)
col, err := db.updateCollection(ctx, txn, collectionsByName, newDescriptionsByName, desc, setAsDefaultVersion)
if err != nil {
return err
}

newDescriptions[i] = col.Description()
}

Expand Down
27 changes: 23 additions & 4 deletions db/txn_db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,14 +250,14 @@
// The collections (including the schema version ID) will only be updated if any changes have actually
// been made, if the net result of the patch matches the current persisted description then no changes
// will be applied.
func (db *implicitTxnDB) PatchSchema(ctx context.Context, patchString string) error {
func (db *implicitTxnDB) PatchSchema(ctx context.Context, patchString string, setAsDefaultVersion bool) error {
txn, err := db.NewTxn(ctx, false)
if err != nil {
return err
}
defer txn.Discard(ctx)

err = db.patchSchema(ctx, txn, patchString)
err = db.patchSchema(ctx, txn, patchString, setAsDefaultVersion)
if err != nil {
return err
}
Expand All @@ -276,8 +276,27 @@
// The collections (including the schema version ID) will only be updated if any changes have actually
// been made, if the net result of the patch matches the current persisted description then no changes
// will be applied.
func (db *explicitTxnDB) PatchSchema(ctx context.Context, patchString string) error {
return db.patchSchema(ctx, db.txn, patchString)
func (db *explicitTxnDB) PatchSchema(ctx context.Context, patchString string, setAsDefaultVersion bool) error {
return db.patchSchema(ctx, db.txn, patchString, setAsDefaultVersion)

Check warning on line 280 in db/txn_db.go

View check run for this annotation

Codecov / codecov/patch

db/txn_db.go#L279-L280 

Added lines #L279 - L280 were not covered by tests
}

func (db *implicitTxnDB) SetDefaultSchemaVersion(ctx context.Context, schemaVersionID string) error {
txn, err := db.NewTxn(ctx, false)
if err != nil {
return err
}

Check warning on line 287 in db/txn_db.go

View check run for this annotation

Codecov / codecov/patch

db/txn_db.go#L286-L287 

Added lines #L286 - L287 were not covered by tests
defer txn.Discard(ctx)

err = db.setDefaultSchemaVersion(ctx, txn, schemaVersionID)
if err != nil {
return err
}

return txn.Commit(ctx)
}

func (db *explicitTxnDB) SetDefaultSchemaVersion(ctx context.Context, schemaVersionID string) error {
return db.setDefaultSchemaVersion(ctx, db.txn, schemaVersionID)

Check warning on line 299 in db/txn_db.go

View check run for this annotation

Codecov / codecov/patch

db/txn_db.go#L298-L299 

Added lines #L298 - L299 were not covered by tests
}

func (db *implicitTxnDB) SetMigration(ctx context.Context, cfg client.LensConfig) error {
Expand Down
25 changes: 23 additions & 2 deletions http/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,31 @@
return cols, nil
}

func (c *Client) PatchSchema(ctx context.Context, patch string) error {
type patchSchemaRequest struct {
Patch string
SetAsDefaultVersion bool
}

func (c *Client) PatchSchema(ctx context.Context, patch string, setAsDefaultVersion bool) error {

Check warning on line 220 in http/client.go

View check run for this annotation

Codecov / codecov/patch

http/client.go#L220 

Added line #L220 was not covered by tests
methodURL := c.http.baseURL.JoinPath("schema")

req, err := http.NewRequestWithContext(ctx, http.MethodPatch, methodURL.String(), strings.NewReader(patch))
body, err := json.Marshal(patchSchemaRequest{patch, setAsDefaultVersion})
if err != nil {
return err
}

Check warning on line 226 in http/client.go

View check run for this annotation

Codecov / codecov/patch

http/client.go#L223-L226 

Added lines #L223 - L226 were not covered by tests

req, err := http.NewRequestWithContext(ctx, http.MethodPatch, methodURL.String(), bytes.NewBuffer(body))
if err != nil {
return err
}
_, err = c.http.request(req)
return err

Check warning on line 233 in http/client.go

View check run for this annotation

Codecov / codecov/patch

http/client.go#L228-L233 

Added lines #L228 - L233 were not covered by tests
}

func (c *Client) SetDefaultSchemaVersion(ctx context.Context, schemaVersionID string) error {
methodURL := c.http.baseURL.JoinPath("schema", "default")

req, err := http.NewRequestWithContext(ctx, http.MethodPost, methodURL.String(), strings.NewReader(schemaVersionID))

Check warning on line 239 in http/client.go

View check run for this annotation

Codecov / codecov/patch

http/client.go#L236-L239 

Added lines #L236 - L239 were not covered by tests
if err != nil {
return err
}
Expand Down
22 changes: 20 additions & 2 deletions http/handler_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,30 @@
func (s *storeHandler) PatchSchema(rw http.ResponseWriter, req *http.Request) {
store := req.Context().Value(storeContextKey).(client.Store)

patch, err := io.ReadAll(req.Body)
var message patchSchemaRequest
err := requestJSON(req, &message)

Check warning on line 155 in http/handler_store.go

View check run for this annotation

Codecov / codecov/patch

http/handler_store.go#L154-L155 

Added lines #L154 - L155 were not covered by tests
if err != nil {
responseJSON(rw, http.StatusBadRequest, errorResponse{err})
return
}
err = store.PatchSchema(req.Context(), string(patch))

err = store.PatchSchema(req.Context(), message.Patch, message.SetAsDefaultVersion)
if err != nil {
responseJSON(rw, http.StatusBadRequest, errorResponse{err})
return
}
rw.WriteHeader(http.StatusOK)

Check warning on line 166 in http/handler_store.go

View check run for this annotation

Codecov / codecov/patch

http/handler_store.go#L161-L166 

Added lines #L161 - L166 were not covered by tests
}

func (s *storeHandler) SetDefaultSchemaVersion(rw http.ResponseWriter, req *http.Request) {
store := req.Context().Value(storeContextKey).(client.Store)

schemaVersionID, err := io.ReadAll(req.Body)
if err != nil {
responseJSON(rw, http.StatusBadRequest, errorResponse{err})
return
}
err = store.SetDefaultSchemaVersion(req.Context(), string(schemaVersionID))

Check warning on line 177 in http/handler_store.go

View check run for this annotation

Codecov / codecov/patch

http/handler_store.go#L169-L177 

Added lines #L169 - L177 were not covered by tests
if err != nil {
responseJSON(rw, http.StatusBadRequest, errorResponse{err})
return
Expand Down
1 change: 1 addition & 0 deletions http/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ func NewServer(db client.DB) *Server {
api.Route("/schema", func(schema chi.Router) {
schema.Post("/", store_handler.AddSchema)
schema.Patch("/", store_handler.PatchSchema)
schema.Post("/default", store_handler.SetDefaultSchemaVersion)
})
api.Route("/collections", func(collections chi.Router) {
collections.Get("/", store_handler.GetCollection)
Expand Down
8 changes: 6 additions & 2 deletions http/wrapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,12 @@
return w.client.AddSchema(ctx, schema)
}

func (w *Wrapper) PatchSchema(ctx context.Context, patch string) error {
return w.client.PatchSchema(ctx, patch)
func (w *Wrapper) PatchSchema(ctx context.Context, patch string, setAsDefaultVersion bool) error {
return w.client.PatchSchema(ctx, patch, setAsDefaultVersion)

Check warning on line 90 in http/wrapper.go

View check run for this annotation

Codecov / codecov/patch

http/wrapper.go#L89-L90 

Added lines #L89 - L90 were not covered by tests
}

func (w *Wrapper) SetDefaultSchemaVersion(ctx context.Context, schemaVersionID string) error {
return w.client.SetDefaultSchemaVersion(ctx, schemaVersionID)

Check warning on line 94 in http/wrapper.go

View check run for this annotation

Codecov / codecov/patch

http/wrapper.go#L93-L94 

Added lines #L93 - L94 were not covered by tests
}

func (w *Wrapper) SetMigration(ctx context.Context, config client.LensConfig) error {
Expand Down
Loading