From a1403dc9d3f2fed18377437b38bbe9e4c468ee53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Garcia?= Date: Mon, 22 Nov 2021 19:42:31 -0300 Subject: [PATCH] Simplify the signature of the Delete() method This change was finally made because the Delete function was the only helper function that was not returning the ksql.ErrRecordNotFound when no rows were found. The other reason for this change is that we the most common use case is by far for deleting a single element, and the philosophy of the library is to optimize for the most common use-cases. For making it easier to write queries for deleting many items as well as many other less common use cases we are already implementing the `kbuilder` package which is a query builder. --- README.md | 7 ++- contracts.go | 7 ++- examples/example_service/mocks.go | 13 ++--- ksql.go | 46 ++++++++++++++---- ksql_test.go | 79 ++----------------------------- mocks.go | 8 ++-- 6 files changed, 59 insertions(+), 101 deletions(-) diff --git a/README.md b/README.md index 5bbae8b..bbd413d 100644 --- a/README.md +++ b/README.md @@ -87,10 +87,13 @@ it with as little functions as possible, so don't expect many additions: ```go // Provider describes the ksql public behavior +// +// The Insert, Update, Delete and QueryOne functions return ksql.ErrRecordNotFound +// if no record was found or no rows were changed during the operation. type Provider interface { Insert(ctx context.Context, table Table, record interface{}) error Update(ctx context.Context, table Table, record interface{}) error - Delete(ctx context.Context, table Table, idsOrRecords ...interface{}) error + Delete(ctx context.Context, table Table, idOrRecord interface{}) error Query(ctx context.Context, records interface{}, query string, params ...interface{}) error QueryOne(ctx context.Context, record interface{}, query string, params ...interface{}) error @@ -492,7 +495,7 @@ if err != nil { In the example above, since we are only interested in a couple of columns it is far simpler and more efficient for the database to only select the columns -that we actually care about, so it's better not to use composite kstructs. +that we actually care about, so it's better not to use composite structs. ### Testing Examples diff --git a/contracts.go b/contracts.go index 3325bce..8016dbd 100644 --- a/contracts.go +++ b/contracts.go @@ -14,11 +14,14 @@ var ErrRecordNotFound error = errors.Wrap(sql.ErrNoRows, "ksql: the query return // ErrAbortIteration ... var ErrAbortIteration error = fmt.Errorf("ksql: abort iteration, should only be used inside QueryChunks function") -// Provider describes the ksql public behavior +// Provider describes the ksql public behavior. +// +// The Insert, Update, Delete and QueryOne functions return ksql.ErrRecordNotFound +// if no record was found or no rows were changed during the operation. type Provider interface { Insert(ctx context.Context, table Table, record interface{}) error Update(ctx context.Context, table Table, record interface{}) error - Delete(ctx context.Context, table Table, idsOrRecords ...interface{}) error + Delete(ctx context.Context, table Table, idOrRecord interface{}) error Query(ctx context.Context, records interface{}, query string, params ...interface{}) error QueryOne(ctx context.Context, record interface{}, query string, params ...interface{}) error diff --git a/examples/example_service/mocks.go b/examples/example_service/mocks.go index 1307ab2..d80ae67 100644 --- a/examples/example_service/mocks.go +++ b/examples/example_service/mocks.go @@ -36,22 +36,17 @@ func (m *MockProvider) EXPECT() *MockProviderMockRecorder { } // Delete mocks base method. -func (m *MockProvider) Delete(ctx context.Context, table ksql.Table, idsOrRecords ...interface{}) error { +func (m *MockProvider) Delete(ctx context.Context, table ksql.Table, idOrRecord interface{}) error { m.ctrl.T.Helper() - varargs := []interface{}{ctx, table} - for _, a := range idsOrRecords { - varargs = append(varargs, a) - } - ret := m.ctrl.Call(m, "Delete", varargs...) + ret := m.ctrl.Call(m, "Delete", ctx, table, idOrRecord) ret0, _ := ret[0].(error) return ret0 } // Delete indicates an expected call of Delete. -func (mr *MockProviderMockRecorder) Delete(ctx, table interface{}, idsOrRecords ...interface{}) *gomock.Call { +func (mr *MockProviderMockRecorder) Delete(ctx, table, idOrRecord interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - varargs := append([]interface{}{ctx, table}, idsOrRecords...) - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockProvider)(nil).Delete), varargs...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockProvider)(nil).Delete), ctx, table, idOrRecord) } // Exec mocks base method. diff --git a/ksql.go b/ksql.go index b7f4b45..3190331 100644 --- a/ksql.go +++ b/ksql.go @@ -555,17 +555,35 @@ func assertStructPtr(t reflect.Type) error { return nil } -// Delete deletes one or more instances from the database by id +// Delete deletes one record from the database using the ID or IDs +// defined on the ksql.Table passed as second argument. +// +// For tables with a single ID column you can pass the record +// to be deleted as a struct, as a map or just pass the ID itself. +// +// For tables with composite keys you must pass the record +// as a struct or a map so that ksql can read all the composite keys +// from it. +// +// The examples below should work for both types of tables: +// +// err := c.Delete(ctx, UsersTable, user) +// +// err := c.Delete(ctx, UserPostsTable, map[string]interface{}{ +// "user_id": user.ID, +// "post_id": post.ID, +// }) +// +// The example below is shorter but will only work for tables with a single primary key: +// +// err := c.Delete(ctx, UsersTable, user.ID) +// func (c DB) Delete( ctx context.Context, table Table, - ids ...interface{}, + idOrRecord interface{}, ) error { - if len(ids) == 0 { - return nil - } - - idMaps, err := normalizeIDsAsMaps(table.idColumns, ids) + idMaps, err := normalizeIDsAsMaps(table.idColumns, []interface{}{idOrRecord}) if err != nil { return err } @@ -578,7 +596,19 @@ func (c DB) Delete( query, params = buildCompositeKeyDeleteQuery(c.dialect, table.name, table.idColumns, idMaps) } - _, err = c.db.ExecContext(ctx, query, params...) + result, err := c.db.ExecContext(ctx, query, params...) + if err != nil { + return err + } + + n, err := result.RowsAffected() + if err != nil { + return fmt.Errorf("unable to check if the record was succesfully deleted: %s", err) + } + + if n == 0 { + return ErrRecordNotFound + } return err } diff --git a/ksql_test.go b/ksql_test.go index a83163c..c58b61b 100644 --- a/ksql_test.go +++ b/ksql_test.go @@ -887,36 +887,6 @@ func TestDelete(t *testing.T) { t.Fatal("could not create test table!, reason:", err.Error()) } - t.Run("should ignore empty lists of ids", func(t *testing.T) { - db, closer := connectDB(t, config) - defer closer.Close() - - ctx := context.Background() - c := newTestDB(db, config.driver) - - u := User{ - Name: "Won't be deleted", - } - - err := c.Insert(ctx, UsersTable, &u) - assert.Equal(t, nil, err) - assert.NotEqual(t, uint(0), u.ID) - - result := User{} - err = getUserByID(c.db, c.dialect, &result, u.ID) - assert.Equal(t, nil, err) - - assert.Equal(t, u.ID, result.ID) - - err = c.Delete(ctx, UsersTable) - assert.Equal(t, nil, err) - - result = User{} - err = getUserByID(c.db, c.dialect, &result, u.ID) - assert.Equal(t, nil, err) - assert.Equal(t, u.ID, result.ID) - }) - t.Run("should delete one id correctly", func(t *testing.T) { tests := []struct { desc string @@ -993,58 +963,15 @@ func TestDelete(t *testing.T) { } }) - t.Run("should delete multiple ids correctly", func(t *testing.T) { + t.Run("should return ErrRecordNotFound if no rows were deleted", func(t *testing.T) { db, closer := connectDB(t, config) defer closer.Close() ctx := context.Background() c := newTestDB(db, config.driver) - u1 := User{ - Name: "Fernanda", - } - err := c.Insert(ctx, UsersTable, &u1) - assert.Equal(t, nil, err) - assert.NotEqual(t, uint(0), u1.ID) - - u2 := User{ - Name: "Juliano", - } - err = c.Insert(ctx, UsersTable, &u2) - assert.Equal(t, nil, err) - assert.NotEqual(t, uint(0), u2.ID) - - u3 := User{ - Name: "This won't be deleted", - } - err = c.Insert(ctx, UsersTable, &u3) - assert.Equal(t, nil, err) - assert.NotEqual(t, uint(0), u3.ID) - - result := User{} - err = getUserByID(c.db, c.dialect, &result, u1.ID) - assert.Equal(t, nil, err) - assert.Equal(t, u1.ID, result.ID) - - result = User{} - err = getUserByID(c.db, c.dialect, &result, u2.ID) - assert.Equal(t, nil, err) - assert.Equal(t, u2.ID, result.ID) - - result = User{} - err = getUserByID(c.db, c.dialect, &result, u3.ID) - assert.Equal(t, nil, err) - assert.Equal(t, u3.ID, result.ID) - - err = c.Delete(ctx, UsersTable, u1.ID, u2.ID) - assert.Equal(t, nil, err) - - results := []User{} - err = getUsersByID(c.db, c.dialect, &results, u1.ID, u2.ID, u3.ID) - assert.Equal(t, nil, err) - - assert.Equal(t, 1, len(results)) - assert.Equal(t, "This won't be deleted", results[0].Name) + err = c.Delete(ctx, UsersTable, 4200) + assert.Equal(t, ErrRecordNotFound, err) }) t.Run("should report error if it receives a nil pointer to a struct", func(t *testing.T) { diff --git a/mocks.go b/mocks.go index caa5ca0..ce17f9f 100644 --- a/mocks.go +++ b/mocks.go @@ -11,7 +11,7 @@ var _ Provider = Mock{} type Mock struct { InsertFn func(ctx context.Context, table Table, record interface{}) error UpdateFn func(ctx context.Context, table Table, record interface{}) error - DeleteFn func(ctx context.Context, table Table, ids ...interface{}) error + DeleteFn func(ctx context.Context, table Table, idOrRecord interface{}) error QueryFn func(ctx context.Context, records interface{}, query string, params ...interface{}) error QueryOneFn func(ctx context.Context, record interface{}, query string, params ...interface{}) error @@ -95,11 +95,11 @@ func (m Mock) Update(ctx context.Context, table Table, record interface{}) error } // Delete ... -func (m Mock) Delete(ctx context.Context, table Table, ids ...interface{}) error { +func (m Mock) Delete(ctx context.Context, table Table, idOrRecord interface{}) error { if m.DeleteFn == nil { - panic(fmt.Errorf("Mock.Delete(ctx, %v, %v) called but the ksql.Mock.DeleteFn() is not set", table, ids)) + panic(fmt.Errorf("Mock.Delete(ctx, %v, %v) called but the ksql.Mock.DeleteFn() is not set", table, idOrRecord)) } - return m.DeleteFn(ctx, table, ids...) + return m.DeleteFn(ctx, table, idOrRecord) } // Query ...