CreatedAt set to current date if empty when Update

[mhewedy]

var e = employee{Name: "Wael"}

if err = repo.Insert(context.Background(), &e); err != nil {
    panic(err)
}

fmt.Println("employee ", e, "created")

var e2 = employee{Name: "Abbas"}
e2.ID = e.ID
time.Sleep(60 * time.Second)

if err = repo.Update(context.Background(), &e2); err != nil {
    panic(err)
}
fmt.Println("employee ", e2, "updated")

Found:
e2.CreatedAt gets updated:

2020/09/24 23:43:53 [duration: 73.43047ms op: adapter-query] INSERT INTO "employees" ("created_at","updated_at","name") VALUES ($1,$2,$3) RETURNING "id";
employee  {4 Wael 2020-09-24 23:43:53 +0300 +03 2020-09-24 23:43:53 +0300 +03} created
employee  {4 Abbas 2020-09-24 23:44:53 +0300 +03 2020-09-24 23:44:53 +0300 +03} updated
2020/09/24 23:44:53 [duration: 14.41539ms op: adapter-exec] UPDATE "employees" SET "id"=$1,"name"=$2,"created_at"=$3,"updated_at"=$4 WHERE "id"=$5;

Expected:
e2.CreatedAt should kept unchanged (2020-09-24 23:43:53 +0300)

full source code: https://gist.github.com/mhewedy/a90cd7946907e9aabd04c873cb326c34

---

Suggestion:
I would suggest to check for the statement type here:
https://github.com/Fs02/rel/blob/c7b126f33cb90e773956ad66ee10a4fa5cdc6660/structset.go#L31
If it is insert then execute the CreatedAt logic, otherwise skip.

What do you think?

[Fs02]

I think this work around should work:

repo.Insert(ctx, record, rel.NewStructset(record, false), rel.Set("created_at", nil))

---

We need to analyze how many change needed if we introduce ops type to mutator, especially since inserting and updating association is actually treated as save (update if id non zero, else insert) right now, which is a more general ops type.

Another solution is to have a separate timestamp mutator like discussed #81 (https://github.com/Fs02/rel/projects/1#card-42354033)

[mhewedy]

We need to analyze how many change needed if we introduce ops type to mutator, especially since inserting and updating association is actually treated as save (update if id non zero, else insert) right now, which is a more general ops type.

Why not use the same concept here, if ID field is non-zero then ignore the createdAt field?

[Fs02]

Why not use the same concept here, if ID field is non-zero then ignore the createdAt field?

Ah you are right, we should be able to do that 🎉

[KMPolakowski]

@Fs02 pls assign me

[Fs02]

Sure, let me know if you need any help 😄

---

To recap, the agreed solution is to check whether primary field is zero before assigning created at for Structset.

rel/structset.go

Line 31 in 1d1e890

if value, ok := doc.Value(field); ok && value.(time.Time).IsZero() {

[KMPolakowski]

@Fs02 I think that we should actually decide this based on the op type as you mentioned at some point. Otherwise there will be no default value assigned for created_at for inserts with id. Not sure how bad this actually would be because db itself still should set the default value.

[Fs02]

Isn't it the same?

I think in both cases, created_at still doesn't have default value and need to be handled in db (either by allowing null for the time type, or by setting using default argument).

[Fs02]

After looking more to the problem, I think you are right, I think we can't depends on ID to decide when to set created_at or not. there's a case when we want to create a record with custom predefined ID, but still want created_at to be set.
So we need op type here.

[KMPolakowski]

@Fs02 It doesn't make to do this now especially when there are additional problems because of this trade off. We should create an issue for this an close this one.

[Fs02]

@KMPolakowski I'm unassigning you for now since the solution for these issue is not very clear yet at the moment, if you have proposal, I'll be more than glad to hear 😄

I'll let this open for now until a good/concrete solution is found unless @mhewedy decided to close this.

[KMPolakowski]

I think this work around should work:

repo.Insert(ctx, record, rel.NewStructset(record, false), rel.Set("created_at", nil))

We need to analyze how many change needed if we introduce ops type to mutator, especially since inserting and updating association is actually treated as save (update if id non zero, else insert) right now, which is a more general ops type.

Another solution is to have a separate timestamp mutator like discussed #81 (https://github.com/Fs02/rel/projects/1#card-42354033)

@Fs02 To find out the ops type we simply have to query db if record with given id exists then when it exists treat as update else insert. When no primary key supplied then treat as insert. The question is where to do this query and how.

[Fs02]

@KMPolakowski if we add a query to find out the ops type, then I think this will be expensive, now all insert and update operation will have one additional query.

Also, mutator are not supposed to access database, so we need to give the ops type before db is accessed:

• Insert will apply insert op to mutator
• Update will apply update op to mutator
• Save will apply save op to mutator - which means, it has to guess whether to insert tor update record?

Save is the most ambiguous op right now, and it's used internally when saving association.

[KMPolakowski]

@Fs02 For Save OP we could introduce a isNew param to the entities which will tell whether the Document has been fetched from the db, indicating update or has been created, indicating insert. Yet we can't do this because we have model structs defined by the user. Well I don't think there're other options except querying or assuming that primary key means update as it was propsed in the first place. I opt for the latter.

[Fs02]

@KMPolakowski agree, for saving association, the mostt feasible way right now is to assume based on primary key.

[KMPolakowski]

@Fs02 you can assign me so I can continue

[Fs02]

@KMPolakowski Sure, Thanks! 🎉

[hxgdzyuyi]

@Fs02 repo.Insert will ignore the ones I've set UpdatedAt value.

        t := time.Parse(...)
	lve.CreatedAt = t
	lve.UpdatedAt = t // will be ignored

	err = repo.Insert(ctx, &lve)

[Fs02]

yes, for now the workaround is:

repo.Insert(ctx, record, rel.NewStructset(record, false), rel.Set("updated_at", t))

maybe a better improvement from REL side is to add a two new mutator: rel.SetCreatedAt(bool) and rel.SetUpdatedAt(bool) which can be used to override this behavior 🤔

[hxgdzyuyi]

yes, for now the workaround is:

repo.Insert(ctx, record, rel.NewStructset(record, false), rel.Set("updated_at", t))

maybe a better improvement from REL side is to add a two new mutator: rel.SetCreatedAt(bool) and rel.SetUpdatedAt(bool) which can be used to override this behavior 🤔

        t := time.Parse(...)
        changeset := rel.NewChangeset(lvp)
	lvp.UpdatedAt = t // will be ignored
	err = repo.Update(ctx, lvp, changeset)

how changeset convert to NewStructset

	err = repo.Update(ctx, lvp, changeset, rel.NewStructset(changeset, false), rel.Set("updated_at", t))

?

[Fs02]

how changeset convert to NewStructset

you can't really convert changeset to structset, but you should be able to just do this:

	err = repo.Update(ctx, lvp, changeset, rel.Set("updated_at", t))

[hxgdzyuyi]

thanks