Kamva/mgm

Type safe partial updates?

tzusman opened this issue · 5 comments

Let's say you have this model:

type Book struct {
  mgm.DefaultModel `bson:",inline"`

  Author string `bson:"author"`

  // ... more fields ...
}

Somewhere in your code you want to change the author's name, so you have:

book := &Book{}
_, err := mgm.Coll(book).UpdateOne(mgm.Ctx(), bson.M{
  "_id": id
}, bson.M{
  "$set": bson.M{
    "author": authorName,
  },
})

Time goes on, the codebase grows, many contributors, and the realization is made that a book can have multiple authors, so the model is changed to:

type Book struct {
  mgm.DefaultModel `bson:",inline"`

  Authors []string `bson:"authors"`

  // ... more fields ...
}

The update code won't throw any sort of error or warning because there is no type safety. Without many lines of code to perform tag reflection (and even more lines if it's a nested field), is there a clean way to make a partial update to a model where type safety is preserved?

Hi @tzusman
author & authors are two different fields, so I think you will not have type safety problems here. but I can assume another problem :

what if we wanted to change the author field(not authors) to an array?
Use a migration to convert your author field to an array before your upgrade to the new version.

And what if we do not use that migration?
Set the author field's type to interface{} and add a method to get the value (as an array). In the method, you should convert string to array if needed.

I hope this is what you wanted.

Hi @mehran-prs That's not what I was getting at, sorry if I wasn't more clear.

The issue is that you can change a model, but since your code elsewhere is referring to that field using a string ("author" in this case), you don't have the power of type safety to tell you that code is outdated. In contrast, when you rename Author to Authors, the go checker will find all the cases in the codebase where you are now using a non-existant field.

I'm wondering if there is a way to do this so that $set operations are type safe. This can be done in an ugly and verbose way with struct reflection, but I'm hoping there's a better way than that.

Thank you for more details,
So the problem is the hard-coded author field in the partial updates and our raw queries that assume that field is a string, not an array.

My first recommendation is that we can simply use the model itself to update our data:

book.Author= authorName
mgm.Coll(book).Update(book)

But it's not a definitive solution, so:

We can have constants that list fields names for each model:
e.g.,

package model

const(
BookFieldAuthor="author"
// next fields here
)

type Book struct {
  mgm.DefaultModel `bson:",inline"`

  Author string `bson:"author"`

  // ... more fields ...
}

And

book := &Book{}
_, err := mgm.Coll(book).UpdateOne(mgm.Ctx(), bson.M{
  "_id": id
}, bson.M{
  "$set": bson.M{
   BookFieldAuthor: authorName,
  },
})

Let's say after a year we decide to change another to authors field, we can just simply refactor all queries which use that constant and then refactor our constant to be good with our new changes in the model (and we do a migration for our new upgrade):

package model

const(
BookFieldAuthors="authors"
)
type Book struct {
  mgm.DefaultModel `bson:",inline"`

  Author []string `bson:"authors"`

  // ... more fields ...
}

and

book := &Book{}
_, err := mgm.Coll(book).UpdateOne(mgm.Ctx(), bson.M{
  "_id": id
}, bson.M{
  "$set": bson.M{
   BookFieldAuthors: []string{authorName},
  },
})

Thanks for these ideas, @mehran-prs.

Using Update wouldn't be ideal since there would be a lot of unnecessary retrievals so that no existing data is removed.

BookFieldAuthors is certainly better than strings, but is still disconnected from the model. You can update the model's field and the go checker wouldn't recognize that the constant needs to change too. This could be mitigated by adding a check in non-production runtime, but that's not pretty. It also might get a bit unwieldy when you have nested fields - do you use BookFieldAuthorLastName? Do you concatenate BookFieldAuthor and LastName?

It's possible this just isn't possible to do in a concise way. It wouldn't be the first time with Go :)

Your welcome @tzusman
Yes, it disconnected from the model, and that check could be a good thing to prevent us from mistakes(let say as a test in our app and CI), and as you said it wouldn't be the first time with Go :)