Modify interface and implementation of BulkInsertWithAssigningIDs #53

t-tiger · 2021-11-07T11:25:04Z

This PR is a follow-up of #50.

Method only accepts a slice of struct for returning values

Previous PR accepts both slice of PK and slice of struct, but I feel it should be consistent and natural to just accept a slice of struct. Although the initial motivation was returning the created IDs, DB can return the multiple specified columns, so I think it would be better not to stick to returning PK as a library.

Modify README

Change the example using BulkInsertWithReturningValues.

t-tiger · 2021-11-07T11:26:39Z

bulk_insert_test.go

+	mock.ExpectQuery(
+		"INSERT INTO `tables` \\(`ThisIsCamelCase`, `regular_column`\\)",


I'd like to test with actual DBs using Docker containers as a follow-up task.

Personally I don't think that's necessary since this supports multiple dialects so it would require multiple containers. Also the only goal for this project is to generate SQL, not handle or parse the dialect nor handle connections.

An example directory with a docker-compose file starting the DBMs to use as integration test might make sense but I don't think the unit tests should rely on that.

I have been testing manually to see if it works in various cases like ID is int or string, and composite PK, and so on. But when I make a change like this, I feel it's inconvenient without automated tests.

An example directory with a docker-compose file starting the DBMs to use as integration test might make sense but I don't think the unit tests should rely on that.

Can I ask why we should not use DBMs for unit tests? With docker-compose, we can easily test with DBs, and since there are only two public methods, I don't feel integration tests are necessary.

I'm just used to unit tests being small and quick, testing the implementation but not the dependencies. But it's easy enough to use Docker for unit tests as well so if you feel it makes sense go for it!

Are you planning on testing all DBMs that Gorm support or only a subset?

Are you planning on testing all DBMs that Gorm support or only a subset?

I'm planning to test with MySQL and PostgreSQL since most of the users likely to use, I guess.

bulk_insert.go

t-tiger · 2021-11-07T12:07:35Z

FYI: @laushunyu I've created a follow-up of your PR. Please check if you have time🙇

laushunyu

Coooool PR!
It would be best if we suuport scanning []uint.

laushunyu · 2021-11-08T03:21:45Z

bulk_insert.go

+func BulkInsertWithReturningValues(db *gorm.DB, objects []interface{}, returnedVals interface{}, chunkSize int, excludeColumns ...string) error {
+	typ := reflect.TypeOf(returnedVals)
+	if typ.Kind() != reflect.Ptr || typ.Elem().Kind() != reflect.Slice || typ.Elem().Elem().Kind() != reflect.Struct {
+		return errors.New("returnedVals must be a pointer to a slice of struct")


we can also accept *[]uint i think, it will be more useful.

I'm not a fan of accepting a different type in a single method, rather I'd like to have a dedicated method if we accept a slice.

The reason why I changed not to accept slice is, returning statement is not only for PK, we can get values with multiple fields. Therefore, I thought we should support a wide range of use cases. However, I guess one of the major use cases is returning id that you proposed in the previous PR, so I can re-consider having a dedicated method like this.

func BulkInsertWithReturningIDs(db *gorm.DB, objects []interface{}, ids interface{}, chunkSize int, excludeColumns ...string) error // Or we should rename to use "PK" instead of "ID"? But PK is not always a single column🤔 func BulkInsertWithReturningPKs(db *gorm.DB, objects []interface{}, pks interface{}, chunkSize int, excludeColumns ...string) error

What do you think?

how about

func BulkInsertPluckingReturning(db *gorm.DB, objects []interface{}, column string, columnData interface{}, chunkSize int, excludeColumns ...string) error

Hmm, I feel BulkInsertWithReturningIDs seems better. If the caller wants to get values except for id column, it's still able to use BulkInsertWithReturningValues. If you agree to this, I'll implement as soon as possible and merge it.

bulk_insert.go

laushunyu · 2021-11-08T03:28:16Z

README.md

+	var returned []struct {
+		ID        int
+		Name	  string
+		CreatedAt time.Time
 	}


if i only need ID, i would define

var returned []struct{ ID uint }

scan it from result then

var ids []uint for _,ret := range returned{ ids = append(ids, ret.ID) }

it's odd...
its nessessary to support *[]uint or *[]string, i suggest.

laushunyu · 2021-11-08T03:30:58Z

bulk_insert.go

 //
-// [returnedValue] slice of primary_key or model, must be a *[]uint(for integer), *[]string(for uuid), *[]struct(for `returning *`)
+// [db] must be set with "gorm:insert_option" to execute RETURNING clause. e.g. db.Set("gorm:insert_option", "RETURNING id")


how about check using db.Get("gorm:insert_option") then warn it to avoid the confusing empty reply?

laushunyu · 2021-11-08T03:37:11Z

README.md

-	fmt.Printf("success to insert with returning: %+v\n", returnId)
-	// `success to insert with returning: [4 5 6]`
+	// Values of `returned` will be like this
+	// {{ID: 1, Name: "name0", CreatedAt: 2021-10-31 16:21:48.019947 +0000 UTC}, ...}


{ID: 11,...},{ID: 12,...},...
there are 10 records existed before, haha

bombsimon

Nice followup! I have no strong feelings around this but a suggestion on how to make this easier to the caller by setting the insert optipn based on the passed struct now that we require one.

bulk_insert.go

bombsimon · 2021-11-08T22:45:20Z

bulk_insert_test.go

+	mock.ExpectQuery(
+		"INSERT INTO `tables` \\(`ThisIsCamelCase`, `regular_column`\\)",


Personally I don't think that's necessary since this supports multiple dialects so it would require multiple containers. Also the only goal for this project is to generate SQL, not handle or parse the dialect nor handle connections.

An example directory with a docker-compose file starting the DBMs to use as integration test might make sense but I don't think the unit tests should rely on that.

bulk_insert.go

bombsimon · 2021-11-08T22:58:38Z

bulk_insert_test.go

@@ -26,6 +26,78 @@ type fakeTable struct {
 	UpdatedAt time.Time
 }

+func TestBulkInsertWithReturningValues(t *testing.T) {


Still think this test case should be simplified without column options and fields not needed.

You mean, we don't need insert_option related to #53 (comment) ? Sorry, I could not understand very well.

It's a super nit but I just mean the test is copied when testing custom field tags so this test is also using the field tag gorm:"column:ThisIsCamelCase". I think that's confusing since we already test that feature in a different test, this test could use a simpler struct.

type Table struct { ID uint `gorm:"primary_key;auto_increment"` ColumnOne string ColumnTwo string }

And then just refer to the default column names column_one and column_two.

bombsimon

LGTM!

t-tiger added 2 commits November 7, 2021 20:23

modify interface and implementation

e44a1e5

add test for BulkInsertWithReturningValues

3903de2

t-tiger commented Nov 7, 2021

View reviewed changes

t-tiger marked this pull request as draft November 7, 2021 11:26

t-tiger self-assigned this Nov 7, 2021

t-tiger added 5 commits November 7, 2021 20:32

add test for invalid type

d96d08e

update README

59174e7

modify comment

b4155aa

update README

88dc325

recover test

aad2d27

t-tiger commented Nov 7, 2021

View reviewed changes

bulk_insert.go Outdated Show resolved Hide resolved

modify comment

8c21b86

t-tiger force-pushed the fix-epic-assigning-ids branch from d82904f to 4a389f9 Compare November 7, 2021 11:59

update indent

4484575

t-tiger force-pushed the fix-epic-assigning-ids branch from 4a389f9 to 4484575 Compare November 7, 2021 12:02

minor fix

9eeb267

t-tiger marked this pull request as ready for review November 7, 2021 12:06

t-tiger requested a review from bombsimon November 7, 2021 12:06

laushunyu reviewed Nov 8, 2021

View reviewed changes

bombsimon reviewed Nov 8, 2021

View reviewed changes

CR fix

241f2ad

t-tiger requested a review from bombsimon November 15, 2021 00:34

bombsimon approved these changes Nov 15, 2021

View reviewed changes

implement BulkInsertWithReturningIDs

798a708

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify interface and implementation of BulkInsertWithAssigningIDs #53

Modify interface and implementation of BulkInsertWithAssigningIDs #53

t-tiger commented Nov 7, 2021 •

edited

Loading

t-tiger Nov 7, 2021

bombsimon Nov 8, 2021

t-tiger Nov 9, 2021

bombsimon Nov 9, 2021

t-tiger Nov 11, 2021 •

edited

Loading

t-tiger commented Nov 7, 2021

laushunyu left a comment

laushunyu Nov 8, 2021

t-tiger Nov 9, 2021 •

edited

Loading

laushunyu Nov 16, 2021

t-tiger Nov 16, 2021 •

edited

Loading

laushunyu Nov 23, 2021

laushunyu Nov 8, 2021

laushunyu Nov 8, 2021

laushunyu Nov 8, 2021 •

edited

Loading

bombsimon left a comment

bombsimon Nov 8, 2021

bombsimon Nov 8, 2021

t-tiger Nov 11, 2021

bombsimon Nov 15, 2021

bombsimon left a comment

		mock.ExpectQuery(
		"INSERT INTO `tables` \\(`ThisIsCamelCase`, `regular_column`\\)",

Modify interface and implementation of BulkInsertWithAssigningIDs #53

Are you sure you want to change the base?

Modify interface and implementation of BulkInsertWithAssigningIDs #53

Conversation

t-tiger commented Nov 7, 2021 • edited Loading

Method only accepts a slice of struct for returning values

Modify README

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-tiger Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

t-tiger commented Nov 7, 2021

laushunyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-tiger Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-tiger Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laushunyu Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

bombsimon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bombsimon left a comment

Choose a reason for hiding this comment

t-tiger commented Nov 7, 2021 •

edited

Loading

t-tiger Nov 11, 2021 •

edited

Loading

t-tiger Nov 9, 2021 •

edited

Loading

t-tiger Nov 16, 2021 •

edited

Loading

laushunyu Nov 8, 2021 •

edited

Loading