From 046f75295e87f318aeacfb10fa3e1c05879a9b4c Mon Sep 17 00:00:00 2001 From: Victor Vrantchan Date: Wed, 4 Jan 2017 13:18:21 -0500 Subject: [PATCH] consolidate delete operations in mysql store (#746) Adds a helper method which soft deletes entities from the database. --- server/datastore/datastore_hosts_test.go | 6 +++--- server/datastore/datastore_invites_test.go | 2 +- server/datastore/datastore_queries_test.go | 2 +- server/datastore/inmem/hosts.go | 8 +++---- server/datastore/inmem/invites.go | 8 +++---- server/datastore/inmem/queries.go | 8 +++---- server/datastore/mysql/delete.go | 24 +++++++++++++++++++++ server/datastore/mysql/hosts.go | 15 ++----------- server/datastore/mysql/invites.go | 13 ++--------- server/datastore/mysql/labels.go | 8 +------ server/datastore/mysql/packs.go | 12 +---------- server/datastore/mysql/queries.go | 15 ++----------- server/datastore/mysql/scheduled_queries.go | 12 +---------- server/kolide/hosts.go | 2 +- server/kolide/invites.go | 2 +- server/kolide/queries.go | 2 +- server/kolide/traits.go | 7 ------ server/service/service_hosts.go | 12 +---------- server/service/service_invites.go | 6 +----- server/service/service_queries.go | 12 +---------- server/service/service_users.go | 3 ++- 21 files changed, 58 insertions(+), 121 deletions(-) create mode 100644 server/datastore/mysql/delete.go diff --git a/server/datastore/datastore_hosts_test.go b/server/datastore/datastore_hosts_test.go index 45d4a1be2d..351ea59f9a 100644 --- a/server/datastore/datastore_hosts_test.go +++ b/server/datastore/datastore_hosts_test.go @@ -108,7 +108,7 @@ func testSaveHosts(t *testing.T, ds kolide.Datastore) { require.NotNil(t, host) assert.Nil(t, host.PrimaryNetworkInterfaceID) - err = ds.DeleteHost(host) + err = ds.DeleteHost(host.ID) assert.Nil(t, err) host, err = ds.Host(host.ID) @@ -126,7 +126,7 @@ func testDeleteHost(t *testing.T, ds kolide.Datastore) { require.Nil(t, err) require.NotNil(t, host) - err = ds.DeleteHost(host) + err = ds.DeleteHost(host.ID) assert.Nil(t, err) host, err = ds.Host(host.ID) @@ -183,7 +183,7 @@ func testListHost(t *testing.T, ds kolide.Datastore) { assert.Equal(t, "en1", hosts2[1].NetworkInterfaces[1].Interface) assert.Equal(t, "en2", hosts2[3].NetworkInterfaces[0].Interface) - err = ds.DeleteHost(hosts[0]) + err = ds.DeleteHost(hosts[0].ID) require.Nil(t, err) hosts2, err = ds.ListHosts(kolide.ListOptions{}) require.Nil(t, err) diff --git a/server/datastore/datastore_invites_test.go b/server/datastore/datastore_invites_test.go index d0e588152f..9f86580fef 100644 --- a/server/datastore/datastore_invites_test.go +++ b/server/datastore/datastore_invites_test.go @@ -96,7 +96,7 @@ func testDeleteInvite(t *testing.T, ds kolide.Datastore) { assert.Nil(t, err) assert.NotNil(t, invite) - err = ds.DeleteInvite(invite) + err = ds.DeleteInvite(invite.ID) assert.Nil(t, err) invite, err = ds.InviteByEmail("user0@foo.com") diff --git a/server/datastore/datastore_queries_test.go b/server/datastore/datastore_queries_test.go index 247c21c81b..889e9617c8 100644 --- a/server/datastore/datastore_queries_test.go +++ b/server/datastore/datastore_queries_test.go @@ -24,7 +24,7 @@ func testDeleteQuery(t *testing.T, ds kolide.Datastore) { require.NotNil(t, query) assert.NotEqual(t, query.ID, 0) - err = ds.DeleteQuery(query) + err = ds.DeleteQuery(query.ID) require.Nil(t, err) assert.NotEqual(t, query.ID, 0) diff --git a/server/datastore/inmem/hosts.go b/server/datastore/inmem/hosts.go index ef4ef64b11..e047aea9cf 100644 --- a/server/datastore/inmem/hosts.go +++ b/server/datastore/inmem/hosts.go @@ -45,15 +45,15 @@ func (d *Datastore) SaveHost(host *kolide.Host) error { return nil } -func (d *Datastore) DeleteHost(host *kolide.Host) error { +func (d *Datastore) DeleteHost(hid uint) error { d.mtx.Lock() defer d.mtx.Unlock() - if _, ok := d.hosts[host.ID]; !ok { - return notFound("Host").WithID(host.ID) + if _, ok := d.hosts[hid]; !ok { + return notFound("Host").WithID(hid) } - delete(d.hosts, host.ID) + delete(d.hosts, hid) return nil } diff --git a/server/datastore/inmem/invites.go b/server/datastore/inmem/invites.go index 340743ea76..1cb93d1cc8 100644 --- a/server/datastore/inmem/invites.go +++ b/server/datastore/inmem/invites.go @@ -121,13 +121,13 @@ func (d *Datastore) SaveInvite(invite *kolide.Invite) error { } // DeleteInvite deletes an invitation. -func (d *Datastore) DeleteInvite(invite *kolide.Invite) error { +func (d *Datastore) DeleteInvite(id uint) error { d.mtx.Lock() defer d.mtx.Unlock() - if _, ok := d.invites[invite.ID]; !ok { - return notFound("Invite").WithID(invite.ID) + if _, ok := d.invites[id]; !ok { + return notFound("Invite").WithID(id) } - delete(d.invites, invite.ID) + delete(d.invites, id) return nil } diff --git a/server/datastore/inmem/queries.go b/server/datastore/inmem/queries.go index 62dafd4191..c0cdf3d72f 100644 --- a/server/datastore/inmem/queries.go +++ b/server/datastore/inmem/queries.go @@ -37,15 +37,15 @@ func (d *Datastore) SaveQuery(query *kolide.Query) error { return nil } -func (d *Datastore) DeleteQuery(query *kolide.Query) error { +func (d *Datastore) DeleteQuery(qid uint) error { d.mtx.Lock() defer d.mtx.Unlock() - if _, ok := d.queries[query.ID]; !ok { - return notFound("Query").WithID(query.ID) + if _, ok := d.queries[qid]; !ok { + return notFound("Query").WithID(qid) } - delete(d.queries, query.ID) + delete(d.queries, qid) return nil } diff --git a/server/datastore/mysql/delete.go b/server/datastore/mysql/delete.go new file mode 100644 index 0000000000..d18a9f240b --- /dev/null +++ b/server/datastore/mysql/delete.go @@ -0,0 +1,24 @@ +package mysql + +import ( + "fmt" + + "github.com/pkg/errors" +) + +func (d *Datastore) deleteEntity(dbTable string, id uint) error { + deleteStmt := fmt.Sprintf( + ` + UPDATE %s SET deleted_at = ?, deleted = TRUE + WHERE id = ? + `, dbTable) + result, err := d.db.Exec(deleteStmt, d.clock.Now(), id) + if err != nil { + return errors.Wrap(err, fmt.Sprintf("delete %s", dbTable)) + } + rows, _ := result.RowsAffected() + if rows != 1 { + return notFound(dbTable).WithID(id) + } + return nil +} diff --git a/server/datastore/mysql/hosts.go b/server/datastore/mysql/hosts.go index 467f1db326..f4e8b087a6 100644 --- a/server/datastore/mysql/hosts.go +++ b/server/datastore/mysql/hosts.go @@ -239,19 +239,8 @@ func (d *Datastore) SaveHost(host *kolide.Host) error { return nil } -func (d *Datastore) DeleteHost(host *kolide.Host) error { - sqlStatement := ` - UPDATE hosts SET - deleted = TRUE, - deleted_at = ? - WHERE id = ? - ` - _, err := d.db.Exec(sqlStatement, d.clock.Now(), host.ID) - if err != nil { - return errors.DatabaseError(err) - } - - return nil +func (d *Datastore) DeleteHost(hid uint) error { + return d.deleteEntity("hosts", hid) } // TODO needs test diff --git a/server/datastore/mysql/invites.go b/server/datastore/mysql/invites.go index 03794ffc2f..ac91edebe9 100644 --- a/server/datastore/mysql/invites.go +++ b/server/datastore/mysql/invites.go @@ -101,15 +101,6 @@ func (d *Datastore) SaveInvite(i *kolide.Invite) error { } -func (d *Datastore) DeleteInvite(i *kolide.Invite) error { - i.MarkDeleted(d.clock.Now()) - sql := ` - UPDATE invites SET deleted_at = ?, deleted = ? - WHERE id = ? - ` - _, err := d.db.Exec(sql, i.DeletedAt, true, i.ID) - if err != nil { - return errors.Wrap(err, "delete invite") - } - return nil +func (d *Datastore) DeleteInvite(id uint) error { + return d.deleteEntity("invites", id) } diff --git a/server/datastore/mysql/labels.go b/server/datastore/mysql/labels.go index 861f687369..70257c7757 100644 --- a/server/datastore/mysql/labels.go +++ b/server/datastore/mysql/labels.go @@ -34,13 +34,7 @@ func (d *Datastore) NewLabel(label *kolide.Label) (*kolide.Label, error) { // DeleteLabel soft deletes a kolide.Label func (d *Datastore) DeleteLabel(lid uint) error { - sql := ` - UPDATE labels - SET deleted_at = ?, deleted = TRUE - WHERE id = ? - ` - _, err := d.db.Exec(sql, d.clock.Now(), lid) - return errors.DatabaseError(err) + return d.deleteEntity("labels", lid) } // Label returns a kolide.Label identified by lid if one exists diff --git a/server/datastore/mysql/packs.go b/server/datastore/mysql/packs.go index 03822495d2..aca213cb5f 100644 --- a/server/datastore/mysql/packs.go +++ b/server/datastore/mysql/packs.go @@ -42,17 +42,7 @@ func (d *Datastore) SavePack(pack *kolide.Pack) error { // DeletePack soft deletes a kolide.Pack so that it won't show up in results func (d *Datastore) DeletePack(pid uint) error { - sql := ` - UPDATE packs - SET deleted_at = ?, deleted = TRUE - WHERE id = ? - ` - _, err := d.db.Exec(sql, d.clock.Now(), pid) - if err != nil { - return errors.DatabaseError(err) - } - - return nil + return d.deleteEntity("packs", pid) } // Pack fetch kolide.Pack with matching ID diff --git a/server/datastore/mysql/queries.go b/server/datastore/mysql/queries.go index 411ed9446a..4a630cc62c 100644 --- a/server/datastore/mysql/queries.go +++ b/server/datastore/mysql/queries.go @@ -44,19 +44,8 @@ func (d *Datastore) SaveQuery(q *kolide.Query) error { } // DeleteQuery soft deletes Query identified by Query.ID -func (d *Datastore) DeleteQuery(query *kolide.Query) error { - query.MarkDeleted(d.clock.Now()) - sql := ` - UPDATE queries - SET deleted_at = ?, deleted = true - WHERE id = ? - ` - _, err := d.db.Exec(sql, query.DeletedAt, query.ID) - if err != nil { - return errors.DatabaseError(err) - } - - return nil +func (d *Datastore) DeleteQuery(qid uint) error { + return d.deleteEntity("queries", qid) } // DeleteQueries (soft) deletes the existing query objects with the provided diff --git a/server/datastore/mysql/scheduled_queries.go b/server/datastore/mysql/scheduled_queries.go index 04eae499ee..2596ff17a4 100644 --- a/server/datastore/mysql/scheduled_queries.go +++ b/server/datastore/mysql/scheduled_queries.go @@ -66,17 +66,7 @@ func (d *Datastore) SaveScheduledQuery(sq *kolide.ScheduledQuery) (*kolide.Sched } func (d *Datastore) DeleteScheduledQuery(id uint) error { - query := ` - UPDATE scheduled_queries - SET deleted_at = ?, deleted = ? - WHERE id = ? - ` - _, err := d.db.Exec(query, d.clock.Now(), true, id) - if err != nil { - return errors.Wrap(err, "deleting a scheduled query") - } - - return nil + return d.deleteEntity("scheduled_queries", id) } func (d *Datastore) ScheduledQuery(id uint) (*kolide.ScheduledQuery, error) { diff --git a/server/kolide/hosts.go b/server/kolide/hosts.go index 2c9808bb37..0deae099a7 100644 --- a/server/kolide/hosts.go +++ b/server/kolide/hosts.go @@ -11,7 +11,7 @@ import ( type HostStore interface { NewHost(host *Host) (*Host, error) SaveHost(host *Host) error - DeleteHost(host *Host) error + DeleteHost(hid uint) error Host(id uint) (*Host, error) ListHosts(opt ListOptions) ([]*Host, error) EnrollHost(osqueryHostId string, nodeKeySize int) (*Host, error) diff --git a/server/kolide/invites.go b/server/kolide/invites.go index aa68c92a58..d7d7656f8a 100644 --- a/server/kolide/invites.go +++ b/server/kolide/invites.go @@ -29,7 +29,7 @@ type InviteStore interface { SaveInvite(i *Invite) error // DeleteInvite deletes an invitation. - DeleteInvite(i *Invite) error + DeleteInvite(id uint) error } // InviteService contains methods for a service which deals with diff --git a/server/kolide/queries.go b/server/kolide/queries.go index 9404cfa81b..7c7a798f26 100644 --- a/server/kolide/queries.go +++ b/server/kolide/queries.go @@ -13,7 +13,7 @@ type QueryStore interface { // SaveQuery saves changes to an existing query object. SaveQuery(query *Query) error // DeleteQuery (soft) deletes an existing query object. - DeleteQuery(query *Query) error + DeleteQuery(qid uint) error // DeleteQueries (soft) deletes the existing query objects with the // provided IDs. The number of deleted queries is returned along with // any error. diff --git a/server/kolide/traits.go b/server/kolide/traits.go index 43c90b9785..128f7b7f3b 100644 --- a/server/kolide/traits.go +++ b/server/kolide/traits.go @@ -15,13 +15,6 @@ type DeleteFields struct { Deleted bool `json:"deleted"` } -// MarkDeleted indicates a record is deleted. It won't actually be removed from -// the database, but won't be returned in result sets. -func (d *DeleteFields) MarkDeleted(deleted time.Time) { - d.DeletedAt = &deleted - d.Deleted = true -} - // UpdateTimestamp contains a timestamp that is set whenever an entity is changed type UpdateTimestamp struct { UpdatedAt time.Time `json:"updated_at" db:"updated_at"` diff --git a/server/service/service_hosts.go b/server/service/service_hosts.go index d8689c464e..e9b0321851 100644 --- a/server/service/service_hosts.go +++ b/server/service/service_hosts.go @@ -25,15 +25,5 @@ func (svc service) HostStatus(ctx context.Context, host kolide.Host) string { } func (svc service) DeleteHost(ctx context.Context, id uint) error { - host, err := svc.ds.Host(id) - if err != nil { - return err - } - - err = svc.ds.DeleteHost(host) - if err != nil { - return err - } - - return nil + return svc.ds.DeleteHost(id) } diff --git a/server/service/service_invites.go b/server/service/service_invites.go index 80d5a8a85d..eda85d4378 100644 --- a/server/service/service_invites.go +++ b/server/service/service_invites.go @@ -96,9 +96,5 @@ func (svc service) VerifyInvite(ctx context.Context, token string) (*kolide.Invi } func (svc service) DeleteInvite(ctx context.Context, id uint) error { - invite, err := svc.ds.Invite(id) - if err != nil { - return err - } - return svc.ds.DeleteInvite(invite) + return svc.ds.DeleteInvite(id) } diff --git a/server/service/service_queries.go b/server/service/service_queries.go index 42935e82cf..da1592af0e 100644 --- a/server/service/service_queries.go +++ b/server/service/service_queries.go @@ -68,17 +68,7 @@ func (svc service) ModifyQuery(ctx context.Context, id uint, p kolide.QueryPaylo } func (svc service) DeleteQuery(ctx context.Context, id uint) error { - query, err := svc.ds.Query(id) - if err != nil { - return err - } - - err = svc.ds.DeleteQuery(query) - if err != nil { - return err - } - - return nil + return svc.ds.DeleteQuery(id) } func (svc service) DeleteQueries(ctx context.Context, ids []uint) (uint, error) { diff --git a/server/service/service_users.go b/server/service/service_users.go index 5a7822e7cc..bfc035fff2 100644 --- a/server/service/service_users.go +++ b/server/service/service_users.go @@ -25,7 +25,8 @@ func (svc service) NewUser(ctx context.Context, p kolide.UserPayload) (*kolide.U if err != nil { return nil, err } - err = svc.ds.DeleteInvite(invite) + + err = svc.ds.DeleteInvite(invite.ID) if err != nil { return nil, err }