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.

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

InsertStmt: automatically add insert columns when using InsertStmt.Record() #227

Closed
wants to merge 4 commits into from

Conversation

brunotm
Copy link

@brunotm brunotm commented Feb 19, 2021

This change automatically adds the insert columns when using
InsertStmt.Record() if they are not set by calling Columns().

Calling Columns() afterwards still maintains the same behaviour as it
overwrites the Column field.

Since there is no API incompatibility, this change was made on the
existing Record() function.

…cord()

This change automatically adds the insert columns when using
InsertStmt.Record() if they are not set by calling Columns().

Calling Columns() afterwards still maintains the same behaviour as it
overwrites the Column field.

Since there is no API incompatibility, this change was made on the
existing Record() function.
@jiyeyuran
Copy link
Contributor

there is a bug on changing fields

@brunotm
Copy link
Author

brunotm commented Mar 30, 2021

Hello @jiyeyuran, can you elaborate on that?
Thanks

@jiyeyuran
Copy link
Contributor

if len(b.Column) == 0 {
        // 'fields' is  cached in tagStore
	fields := s.get(v.Type())
	var i int
	for x := 0; x < len(fields); x++ {
		if fields[x] != "" {
                         // this also changed the cached fields 
			fields[i] = fields[x]
			i++
		}
	}
	b.Columns(fields[:i]...)
}

@brunotm
Copy link
Author

brunotm commented Mar 31, 2021

@jiyeyuran the tagstore is intialized on each Record() call with s := newTagStore(), what do you mean by cached?

The only "problem" here would be calling Record() multiple times with different struct types for which the initialised columns for the 1st struct would not match the 2nd struct columns. But this "problem" already exists if the provided struct fields don't match the specified Columns() call.

There is actually room for improvement if we make the tagstore part of the insert statement and reutilise it for both Record calls and struct validation between calls

@jiyeyuran
Copy link
Contributor

jiyeyuran commented Mar 31, 2021

The bug will show if you add test like below:

type insertTest struct {
	A int
        D string `db:"-"`
	C string `db:"b"`
	u int
}

You also need to ignore the id field with zero value.

@jiyeyuran
Copy link
Contributor

No bug version:

if len(b.Column) == 0 {
	fields := s.get(v.Type())
	for i, field := range fields {
		if field == "id" {
			if idField := v.Field(i); idField.IsZero() {
				continue
			}
		}
		if field != "" {
			b.Column = append(b.Column, field)
		}
	}
}

When calling Record() when no columns are specified, the id field values needs
to be checked before inclusion.

Thanks @jiyeyuran
@brunotm
Copy link
Author

brunotm commented Mar 31, 2021

@jiyeyuran got it, thanks. I have updated the PR with the suggested changes.

@stale
Copy link

stale bot commented Jun 11, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jun 11, 2021
@stale stale bot closed this Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants