zero values inserted for nil pointers

[armhold]

I have a model with a nil-able time field, and rather than letting the database fill it in, reform is causing inserts to have zero-based dates.

My model is like:

//go:generate reform
//reform:feed_changes
type FeedChange struct {
	ID         int        `reform:"id,pk"`
	CreatedAt  *time.Time `reform:"created_at"`
	OldFeedURL string     `reform:"old_feed_url"`
	NewFeedURL string     `reform:"new_feed_url"`
	BookID  int           `reform:"book_id"`
}

And my table (this is postgres):

    Column    |           Type           | Collation | Nullable |                 Default                  
--------------+--------------------------+-----------+----------+------------------------------------------
 id           | integer                  |           | not null | nextval('feed_changes_id_seq'::regclass)
 created_at   | timestamp with time zone |           |          | CURRENT_TIMESTAMP
 book_id      | integer                  |           | not null | 
 old_feed_url | character varying(512)   |           |          | 
 new_feed_url | character varying(512)   |           |          | 
Indexes:
    "feed_changes_pkey" PRIMARY KEY, btree (id)
    "idx_feed_changes_book_id" btree (book_id)

If I use reformdb and the postgres driver, it inserts the zero value for time, rather than letting the DB fill it in. It generates SQL like the following:

SQL: >>> INSERT INTO "feed_changes" ("created_at", "old_feed_url", "new_feed_url", "book_id") VALUES ($1, $2, $3, $4) RETURNING "id" [<nil>, `http://example.com/old`, `http://example.com/new`, 1]

I think the presence of the column "created_at" in the generated SQL is causing the zero-value insert.

If I insert into this table manually without a "created_at" field, the DB automatically fills it in for me using CURRENT_TIMESTAMP.

INSERT INTO "feed_changes" ("old_feed_url", "new_feed_url", "book_id") VALUES ('http://example.com/old', 'http://example.com/new', 1) RETURNING 'id'

Am I doing something wrong here? I'm using reform via dep, locked to git revision
161dee9

Thanks very much.

[AlekSi]

Hi George, sorry for a long delay – that message was buried in my inbox.

What you hit is actually one of the hardest issues in reform design, and in ORMs in general – mapping of database default values to code. The second proprietary version of reform used ",omitempty" tag modifier which removed zero values from the generated query. That worked, but was a very confusing source of bugs – why I can't INSERT zero value? why I can't UPDATE column to set a zero value? The third version tried to use database schema information to handle columns with default values in a special way, but that was very fragile, and also did not work well when some other component (Ruby on Rails app in our case) wanted to control database schema. For the current reform version, we took a step back: what for default values are used? Most of the use cases fall into two categories: primary keys, and created_at/updated_at fields. The first one is handled separately (and that's why PK fields can't be pointers, and that's why they can't have zero values). The second category can be replaced with BeforeInserter and BeforeUpdater interfaces:

reform/internal/test/models/good.go

Lines 25 to 65 in 161dee9

	// BeforeInsert sets CreatedAt if it's not set,
	// then converts to UTC, truncates to second and strips monotonic clock reading from both CreatedAt and UpdatedAt.
	func (p *Person) BeforeInsert() error {
	if p.CreatedAt.IsZero() {
	p.CreatedAt = time.Now()
	}
	p.CreatedAt = p.CreatedAt.UTC().Truncate(time.Second).AddDate(0, 0, 0)
	if p.UpdatedAt != nil {
	p.UpdatedAt = pointer.ToTime(p.UpdatedAt.UTC().Truncate(time.Second).AddDate(0, 0, 0))
	}
	return nil
	}
	// BeforeUpdate sets CreatedAt if it's not set,
	// sets UpdatedAt,
	// then converts to UTC, truncates to second and strips monotonic clock reading from both CreatedAt and UpdatedAt.
	func (p *Person) BeforeUpdate() error {
	now := time.Now()
	if p.CreatedAt.IsZero() {
	p.CreatedAt = now
	}
	p.UpdatedAt = &now
	p.CreatedAt = p.CreatedAt.UTC().Truncate(time.Second).AddDate(0, 0, 0)
	p.UpdatedAt = pointer.ToTime(p.UpdatedAt.UTC().Truncate(time.Second).AddDate(0, 0, 0))
	return nil
	}
	// AfterFind converts to UTC and truncates to second both CreatedAt and UpdatedAt.
	func (p *Person) AfterFind() error {
	p.CreatedAt = p.CreatedAt.UTC().Truncate(time.Second)
	if p.UpdatedAt != nil {
	p.UpdatedAt = pointer.ToTime(p.UpdatedAt.UTC().Truncate(time.Second))
	}
	return nil
	}

[AlekSi]

See also #100.

[armhold]

Hi AlekSi, thanks for the reply, and no worries on the delay.

Perhaps I assumed too much, but from the example in the README:

//reform:people
type Person struct {
	ID        int32      `reform:"id,pk"`
	Name      string     `reform:"name"`
	Email     *string    `reform:"email"`
	CreatedAt time.Time  `reform:"created_at"`
	UpdatedAt *time.Time `reform:"updated_at"`
}

... I assumed that pointer fields (Email, UpdatedAt in this example) were a way to express "no value given", and that reform would ignore such nils, rather than inserting zero values.

If that is not the case (design decision due to common bugs, as you mentioned), what is the purpose of having one time.Time field a pointer and the other a value in the documentation? It implies (to me, at least) that they are handled differently. Also, the README states "Use pointers for nullable fields", so I am unsure what that is supposed to mean, if zero values will be inserted for null (nil) fields.

I ended up using a constructor for my models that fills in the CreatedAt field for me (similar to your BeforeInsert), but my preference is to have the DB do this automatically if that is at all possible. Not a big deal for me really, but the docs could use a clarification. Thank you.

[AlekSi]

Use pointers for nullable fields

That means that one normally should use pointers for NULL columns, non-pointers for NOT NULL columns. nil always maps to SQL NULL. In that example, created_at is always set, but updated_at is NULL initially. That is to highlight the fact that one has no reason to use sql.NullXXX types. In fact, there is no reason to use them at all in Go 1.1+ – database/sql handles pointers correctly, reform does not alter Scan/Value logic at all.