Saving and updating data

[ghost]

Hey, Jeff.

How do you think the mechanism of saving / updating info can be improved? Now it looks like this: Model

type Article struct {
    Title string
    Posted time.Time
    Body string
    Author bson.ObjectId
    Smth smth
}

func (article *Article) Save(s *mgo.Session) error {
    ...
    _, err := coll.Upsert(bson.M{"_id": article.Id}, article)
    ...
}

So to use it we have to do: Controller

func (c App) Update(article *models.Article) revel.Result {
    ...
    // Make sure that the user hasn't modified system fields
    article.Posted = ...
    article.Author = c.User
    article.Smth = smth
    article.Save(c.MongoSession)
    ...
}

So in case, we add new fields to our Article struct like this:

type Article struct {
    ...
    Smth smth
    Smth1 smth1
    Smth2 smth2
}

we have to rewrite all the fragments of updating / saving info:

...
article.Smth = smth
article.Smth1 = smth1
article.Smth2 = smth2
article.Save(c.MongoSession)

to make sure that users cannot cheat the system by sending values they are not expected to.

What do you think if we change the code a little bit the following way: Model

func (article *Article) Save(s *mgo.Session, change *bson.M) error {
    ...
    _, err := coll.Upsert(bson.M{"_id": article.Id}, change)
    ...
}

func (article *Article) GetInitialChange() *bson.M {
    return &bson.M {
        "Title": article.Title,
        "Body": article.Body,
        "Posted": article.Posted,
        "Author": article.Author,
    }
}

func (article *Article) GetBasicChange() *bson.M {
    return &bson.M {
        "$set": bson.M {
            "Title": article.Title,
            "Body": article.Body,
        },
    }
}

So our controller then is:

func (c App) Create(article *models.Article) revel.Result {
    ...
    article.Posted = time.Now()
    article.Author = c.User
    article.Save(c.MongoSession, article.GetInitialChange())
    ...
}

func (c App) Update(article *models.Article) revel.Result {
    ...
    article.Save(c.MongoSession, article.GetBasicChange())
    ...
}

Now we can add new fields to the Article structure and do not worry about a necessity to change our controller.

[jgraham909]

anonx,

Agreed I was really not happy with the existing implementation. That's what the "@todo properly implement this" in the existing Blog.Update() controller implementation is hinting at.

Your suggestion is certainly in the direction I would like this to go and a certain improvement over where things are currently.

Goals for refactoring;

1. System integrity and validation should all be in the model itself and we should not rely on the controller implementation to "do the right thing".
2. Business logic should also be moved to the model itself. This is mostly just a facet of the previous item.
3. Data handling should only require one step for population and validation outside of model code.
4. Form -> Model handling. What you have suggested is a start on the right idea but I think the implementation doesn't quite get us as far as we should go. Maybe this is as straightforward as merging GetInitialChange() and GetBasicChange() into one function, so that we always call the same method to instantiate or update our model. Perhaps it also makes sense for this to use reflection to merge an arbitrary amount of fields into an existing model?

The above is just off the top of my head and I would especially like some feedback on before we agree on a direction and goals for refactoring the model handling.

[ghost]

@jgraham909, 1, 2, 3. Agree;

4. Well, probably some other way should be used. However, for every kind of action we will have to explicitly specify a list of fields we want to use for the request anyway. So I don't see how it can be dramatically improved (the process of a change generation can be improved but we cannot escape the necessity to define the list of fields). If we only could get the info about form inputs and text areas (those ones which are really represented in the views source code)... But anyway, the use of $set instead of two separate requests to DB (See code below) is a priority number one for now, isn't it?

check := models.GetArticleByObjectId(c.MongoSession, article.Id) // #1
article.Author_id = check.Author_id
article.Something = check.Something
...
article.Save(c.MongoSession) // #2

The operations are not atomic. In theory, something bad may happen in between.