From 835d3dfe3a25c86e112e7323c20b6e4fbc4b31d4 Mon Sep 17 00:00:00 2001 From: Brendan Shaklovitz Date: Sun, 11 Oct 2020 17:28:44 -0500 Subject: [PATCH 1/7] Remove soft-deletion pattern * Perform migration to delete any entries with `deleted` set, and subsequently drop columns `deleted` and `deleted_at`. * Remove `deleted` and `deleted_at` references. --- .../QueryResultsTable.tests.jsx | 2 - frontend/test/stubs.js | 8 -- server/datastore/inmem/queries.go | 4 +- server/datastore/mysql/campaigns.go | 3 +- server/datastore/mysql/delete.go | 39 ++++++-- server/datastore/mysql/hosts.go | 19 ++-- server/datastore/mysql/invites.go | 39 +++----- server/datastore/mysql/labels.go | 58 +++-------- .../20171212182459_DeleteSoftDeletedPacks.go | 23 ----- ...0181119180000_DeleteSoftDeletedEntities.go | 54 ---------- ...0201011162341_CleanupSoftDeletedColumns.go | 95 ++++++++++++++++++ server/datastore/mysql/packs.go | 50 +++------- server/datastore/mysql/password_reset.go | 6 +- server/datastore/mysql/queries.go | 98 +++++-------------- server/datastore/mysql/scheduled_queries.go | 4 +- server/datastore/mysql/sessions.go | 5 +- server/datastore/mysql/targets.go | 1 - server/datastore/mysql/users.go | 4 +- server/kolide/campaigns.go | 1 - server/kolide/hosts.go | 1 - server/kolide/invites.go | 1 - server/kolide/labels.go | 1 - server/kolide/packs.go | 1 - server/kolide/queries.go | 11 +-- server/kolide/scheduled_queries.go | 1 - server/kolide/traits.go | 8 -- server/kolide/users.go | 1 - server/service/service_sessions.go | 2 +- tools/api/README.md | 4 - tools/app/demo.sql | 16 --- 30 files changed, 209 insertions(+), 351 deletions(-) delete mode 100644 server/datastore/mysql/migrations/data/20171212182459_DeleteSoftDeletedPacks.go delete mode 100644 server/datastore/mysql/migrations/data/20181119180000_DeleteSoftDeletedEntities.go create mode 100644 server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go diff --git a/frontend/components/queries/QueryResultsTable/QueryResultsTable.tests.jsx b/frontend/components/queries/QueryResultsTable/QueryResultsTable.tests.jsx index 4f3a18dcf..d58b7cb9a 100644 --- a/frontend/components/queries/QueryResultsTable/QueryResultsTable.tests.jsx +++ b/frontend/components/queries/QueryResultsTable/QueryResultsTable.tests.jsx @@ -29,8 +29,6 @@ const queryResult = { const campaignWithNoQueryResults = { created_at: '0001-01-01T00:00:00Z', - deleted: false, - deleted_at: null, hosts_count: { failed: 0, successful: 0, diff --git a/frontend/test/stubs.js b/frontend/test/stubs.js index 18b43d1f9..30401c0ef 100644 --- a/frontend/test/stubs.js +++ b/frontend/test/stubs.js @@ -59,8 +59,6 @@ export const flatConfigStub = { export const hostStub = { created_at: '2017-01-10T19:18:55Z', updated_at: '2017-01-10T20:13:52Z', - deleted_at: null, - deleted: false, id: 1, detail_updated_at: '2017-01-10T20:01:48Z', seen_time: '2017-01-10T20:13:54Z', @@ -95,8 +93,6 @@ export const hostStub = { export const labelStub = { created_at: '2017-01-16T23:11:01Z', updated_at: '2017-01-16T23:11:01Z', - deleted_at: null, - deleted: false, id: 1, name: 'All Hosts', description: '', @@ -116,8 +112,6 @@ export const labelStub = { export const packStub = { created_at: '0001-01-01T00:00:00Z', updated_at: '0001-01-01T00:00:00Z', - deleted_at: null, - deleted: false, id: 3, name: 'Pack Name', description: 'Pack Description', @@ -130,8 +124,6 @@ export const packStub = { export const queryStub = { created_at: '2016-10-17T07:06:00Z', - deleted: false, - deleted_at: null, description: '', differential: false, id: 1, diff --git a/server/datastore/inmem/queries.go b/server/datastore/inmem/queries.go index 5cc0099c2..e79a6ad06 100644 --- a/server/datastore/inmem/queries.go +++ b/server/datastore/inmem/queries.go @@ -38,8 +38,8 @@ func (d *Datastore) SaveQuery(query *kolide.Query) error { return nil } -// DeleteQueries (soft) deletes the existing query objects with the provided -// IDs. The number of deleted queries is returned along with any error. +// DeleteQueries deletes the existing query objects with the provided IDs. The +// number of deleted queries is returned along with any error. func (d *Datastore) DeleteQueries(ids []uint) (uint, error) { d.mtx.Lock() defer d.mtx.Unlock() diff --git a/server/datastore/mysql/campaigns.go b/server/datastore/mysql/campaigns.go index 9ece6dd5a..0a7eaf87e 100644 --- a/server/datastore/mysql/campaigns.go +++ b/server/datastore/mysql/campaigns.go @@ -30,7 +30,7 @@ func (d *Datastore) NewDistributedQueryCampaign(camp *kolide.DistributedQueryCam func (d *Datastore) DistributedQueryCampaign(id uint) (*kolide.DistributedQueryCampaign, error) { sql := ` - SELECT * FROM distributed_query_campaigns WHERE id = ? AND NOT deleted + SELECT * FROM distributed_query_campaigns WHERE id = ? ` campaign := &kolide.DistributedQueryCampaign{} if err := d.db.Get(campaign, sql, id); err != nil { @@ -47,7 +47,6 @@ func (d *Datastore) SaveDistributedQueryCampaign(camp *kolide.DistributedQueryCa status = ?, user_id = ? WHERE id = ? - AND NOT deleted ` result, err := d.db.Exec(sqlStatement, camp.QueryID, camp.Status, camp.UserID, camp.ID) if err != nil { diff --git a/server/datastore/mysql/delete.go b/server/datastore/mysql/delete.go index 5eb104716..65e5b44e1 100644 --- a/server/datastore/mysql/delete.go +++ b/server/datastore/mysql/delete.go @@ -3,16 +3,15 @@ package mysql import ( "fmt" + "github.com/jmoiron/sqlx" "github.com/pkg/errors" ) +// deleteEntity deletes an entity with the given id from the given DB table, +// returning a notFound error if appropriate. func (d *Datastore) deleteEntity(dbTable string, id uint) error { - deleteStmt := fmt.Sprintf( - ` - UPDATE %s SET deleted_at = ?, deleted = TRUE - WHERE id = ? AND NOT deleted - `, dbTable) - result, err := d.db.Exec(deleteStmt, d.clock.Now(), id) + deleteStmt := fmt.Sprintf(`DELETE FROM %s WHERE id = ?`, dbTable) + result, err := d.db.Exec(deleteStmt, id) if err != nil { return errors.Wrapf(err, "delete %s", dbTable) } @@ -23,9 +22,8 @@ func (d *Datastore) deleteEntity(dbTable string, id uint) error { return nil } -// deleteEntityByName hard deletes an entity with the given name from the given -// DB table, returning a notFound error if appropriate. -// Note: deleteEntity uses soft deletion, but we are moving off this pattern. +// deleteEntityByName deletes an entity with the given name from the given DB +// table, returning a notFound error if appropriate. func (d *Datastore) deleteEntityByName(dbTable string, name string) error { deleteStmt := fmt.Sprintf("DELETE FROM %s WHERE name = ?", dbTable) result, err := d.db.Exec(deleteStmt, name) @@ -41,3 +39,26 @@ func (d *Datastore) deleteEntityByName(dbTable string, name string) error { } return nil } + +// deleteEntities deletes the existing entity objects with the provided IDs. +// The number of deleted entities is returned along with any error. +func (d *Datastore) deleteEntities(dbTable string, ids []uint) (uint, error) { + deleteStmt := fmt.Sprintf(`DELETE FROM %s WHERE id IN (?)`, dbTable) + + query, args, err := sqlx.In(deleteStmt, ids) + if err != nil { + return 0, errors.Wrapf(err, "building delete entities query %s", dbTable) + } + + result, err := d.db.Exec(query, args...) + if err != nil { + return 0, errors.Wrapf(err, "executing delete entities query %s", dbTable) + } + + deleted, err := result.RowsAffected() + if err != nil { + return 0, errors.Wrapf(err, "fetching delete entities query rows affected %s", dbTable) + } + + return uint(deleted), nil +} diff --git a/server/datastore/mysql/hosts.go b/server/datastore/mysql/hosts.go index 0205c294d..ab1a328b7 100644 --- a/server/datastore/mysql/hosts.go +++ b/server/datastore/mysql/hosts.go @@ -130,7 +130,7 @@ func (d *Datastore) SaveHost(host *kolide.Host) error { } func (d *Datastore) DeleteHost(hid uint) error { - _, err := d.db.Exec("DELETE FROM hosts WHERE id = ?", hid) + err := d.deleteEntity("hosts", hid) if err != nil { return errors.Wrapf(err, "deleting host with id %d", hid) } @@ -140,7 +140,7 @@ func (d *Datastore) DeleteHost(hid uint) error { func (d *Datastore) Host(id uint) (*kolide.Host, error) { sqlStatement := ` SELECT * FROM hosts - WHERE id = ? AND NOT deleted LIMIT 1 + WHERE id = ? LIMIT 1 ` host := &kolide.Host{} err := d.db.Get(host, sqlStatement, id) @@ -154,7 +154,6 @@ func (d *Datastore) Host(id uint) (*kolide.Host, error) { func (d *Datastore) ListHosts(opt kolide.HostListOptions) ([]*kolide.Host, error) { sql := ` SELECT * FROM hosts - WHERE NOT deleted ` var params []interface{} switch opt.StatusFilter { @@ -244,8 +243,7 @@ func (d *Datastore) EnrollHost(osqueryHostID, nodeKey, secretName string) (*koli enroll_secret_name ) VALUES (?, ?, ?, ?, ?, ?) ON DUPLICATE KEY UPDATE - node_key = VALUES(node_key), - deleted = FALSE + node_key = VALUES(node_key) ` var result sql.Result @@ -281,8 +279,6 @@ func (d *Datastore) AuthenticateHost(nodeKey string) (*kolide.Host, error) { osquery_host_id, created_at, updated_at, - deleted_at, - deleted, detail_update_time, label_update_time, node_key, @@ -313,7 +309,7 @@ func (d *Datastore) AuthenticateHost(nodeKey string) (*kolide.Host, error) { config_tls_refresh, enroll_secret_name FROM hosts - WHERE node_key = ? AND NOT deleted + WHERE node_key = ? LIMIT 1 ` @@ -359,7 +355,6 @@ func (d *Datastore) searchHostsWithOmits(query string, omit ...uint) ([]*kolide. MATCH (host_name, uuid) AGAINST (? IN BOOLEAN MODE) OR MATCH (primary_ip, primary_mac) AGAINST (? IN BOOLEAN MODE) ) - AND NOT deleted AND id NOT IN (?) LIMIT 10 ` @@ -383,8 +378,7 @@ func (d *Datastore) searchHostsWithOmits(query string, omit ...uint) ([]*kolide. func (d *Datastore) searchHostsDefault(omit ...uint) ([]*kolide.Host, error) { sqlStatement := ` SELECT * FROM hosts - WHERE NOT deleted - AND id NOT IN (?) + WHERE id NOT in (?) ORDER BY seen_time DESC LIMIT 5 ` @@ -392,7 +386,7 @@ func (d *Datastore) searchHostsDefault(omit ...uint) ([]*kolide.Host, error) { var in interface{} { // use -1 if there are no values to omit. - //Avoids empty args error for `sqlx.In` + // Avoids empty args error for `sqlx.In` in = omit if len(omit) == 0 { in = -1 @@ -435,7 +429,6 @@ func (d *Datastore) SearchHosts(query string, omit ...uint) ([]*kolide.Host, err MATCH (host_name, uuid) AGAINST (? IN BOOLEAN MODE) OR MATCH (primary_ip, primary_mac) AGAINST (? IN BOOLEAN MODE) ) - AND NOT deleted LIMIT 10 ` hosts := []*kolide.Host{} diff --git a/server/datastore/mysql/invites.go b/server/datastore/mysql/invites.go index 2ecf40940..4e0b4bea0 100644 --- a/server/datastore/mysql/invites.go +++ b/server/datastore/mysql/invites.go @@ -8,31 +8,15 @@ import ( "github.com/pkg/errors" ) -// NewInvite generates a new invitation +// NewInvite generates a new invitation. func (d *Datastore) NewInvite(i *kolide.Invite) (*kolide.Invite, error) { - var ( - deletedInvite kolide.Invite - sqlStmt string - ) - err := d.db.Get(&deletedInvite, "SELECT * FROM invites WHERE email = ? AND deleted", i.Email) - switch err { - case nil: - sqlStmt = ` - REPLACE INTO invites ( invited_by, email, admin, name, position, token, deleted, sso_enabled) - VALUES ( ?, ?, ?, ?, ?, ?, ?, ?) - ` - case sql.ErrNoRows: - sqlStmt = ` - INSERT INTO invites ( invited_by, email, admin, name, position, token, deleted, sso_enabled) - VALUES ( ?, ?, ?, ?, ?, ?, ?, ?) - ` - default: - return nil, errors.Wrap(err, "check for existing invite") - } + sqlStmt := ` + INSERT INTO invites ( invited_by, email, admin, name, position, token, sso_enabled) + VALUES ( ?, ?, ?, ?, ?, ?, ?) + ` - deleted := false result, err := d.db.Exec(sqlStmt, i.InvitedBy, i.Email, i.Admin, - i.Name, i.Position, i.Token, deleted, i.SSOEnabled) + i.Name, i.Position, i.Token, i.SSOEnabled) if err != nil && isDuplicate(err) { return nil, alreadyExists("Invite", 0) } else if err != nil { @@ -43,7 +27,6 @@ func (d *Datastore) NewInvite(i *kolide.Invite) (*kolide.Invite, error) { i.ID = uint(id) return i, nil - } // ListInvites lists all invites in the Fleet database. Supply query options @@ -52,7 +35,7 @@ func (d *Datastore) ListInvites(opt kolide.ListOptions) ([]*kolide.Invite, error invites := []*kolide.Invite{} - query := appendListOptionsToSQL("SELECT * FROM invites WHERE NOT deleted", opt) + query := appendListOptionsToSQL("SELECT * FROM invites", opt) err := d.db.Select(&invites, query) if err == sql.ErrNoRows { return nil, notFound("Invite") @@ -65,7 +48,7 @@ func (d *Datastore) ListInvites(opt kolide.ListOptions) ([]*kolide.Invite, error // Invite returns Invite identified by id. func (d *Datastore) Invite(id uint) (*kolide.Invite, error) { var invite kolide.Invite - err := d.db.Get(&invite, "SELECT * FROM invites WHERE id = ? AND NOT deleted", id) + err := d.db.Get(&invite, "SELECT * FROM invites WHERE id = ?", id) if err == sql.ErrNoRows { return nil, notFound("Invite").WithID(id) } else if err != nil { @@ -77,7 +60,7 @@ func (d *Datastore) Invite(id uint) (*kolide.Invite, error) { // InviteByEmail finds an Invite with a particular email, if one exists. func (d *Datastore) InviteByEmail(email string) (*kolide.Invite, error) { var invite kolide.Invite - err := d.db.Get(&invite, "SELECT * FROM invites WHERE email = ? AND NOT deleted", email) + err := d.db.Get(&invite, "SELECT * FROM invites WHERE email = ?", email) if err == sql.ErrNoRows { return nil, notFound("Invite"). WithMessage(fmt.Sprintf("with email %s", email)) @@ -90,7 +73,7 @@ func (d *Datastore) InviteByEmail(email string) (*kolide.Invite, error) { // InviteByToken finds an Invite with a particular token, if one exists. func (d *Datastore) InviteByToken(token string) (*kolide.Invite, error) { var invite kolide.Invite - err := d.db.Get(&invite, "SELECT * FROM invites WHERE token = ? AND NOT deleted", token) + err := d.db.Get(&invite, "SELECT * FROM invites WHERE token = ?", token) if err == sql.ErrNoRows { return nil, notFound("Invite"). WithMessage(fmt.Sprintf("with token %s", token)) @@ -105,7 +88,7 @@ func (d *Datastore) SaveInvite(i *kolide.Invite) error { sql := ` UPDATE invites SET invited_by = ?, email = ?, admin = ?, name = ?, position = ?, token = ?, sso_enabled = ? - WHERE id = ? AND NOT deleted + WHERE id = ? ` results, err := d.db.Exec(sql, i.InvitedBy, i.Email, i.Admin, i.Name, i.Position, i.Token, i.SSOEnabled, i.ID, diff --git a/server/datastore/mysql/labels.go b/server/datastore/mysql/labels.go index 3194c6f57..74a375771 100644 --- a/server/datastore/mysql/labels.go +++ b/server/datastore/mysql/labels.go @@ -27,8 +27,7 @@ func (d *Datastore) ApplyLabelSpecs(specs []*kolide.LabelSpec) (err error) { query = VALUES(query), platform = VALUES(platform), label_type = VALUES(label_type), - label_membership_type = VALUES(label_membership_type), - deleted = false + label_membership_type = VALUES(label_membership_type) ` stmt, err := tx.Prepare(sql) if err != nil { @@ -179,38 +178,16 @@ func (d *Datastore) getLabelHostnames(label *kolide.LabelSpec) error { // NewLabel creates a new kolide.Label func (d *Datastore) NewLabel(label *kolide.Label, opts ...kolide.OptionalArg) (*kolide.Label, error) { db := d.getTransaction(opts) - var ( - deletedLabel kolide.Label - query string - ) - err := db.Get(&deletedLabel, - "SELECT * FROM labels WHERE name = ? AND deleted", label.Name) - switch err { - case nil: - query = ` - REPLACE INTO labels ( - name, - description, - query, - platform, - label_type, - label_membership_type - ) VALUES ( ?, ?, ?, ?, ?, ?) - ` - case sql.ErrNoRows: - query = ` - INSERT INTO labels ( - name, - description, - query, - platform, - label_type, - label_membership_type - ) VALUES ( ?, ?, ?, ?, ?, ?) + query := ` + INSERT INTO labels ( + name, + description, + query, + platform, + label_type, + label_membership_type + ) VALUES ( ?, ?, ?, ?, ?, ?) ` - default: - return nil, errors.Wrap(err, "check for existing label") - } result, err := db.Exec( query, label.Name, @@ -249,11 +226,11 @@ func (d *Datastore) DeleteLabel(name string) error { return d.deleteEntityByName("labels", name) } -// Label returns a kolide.Label identified by lid if one exists +// Label returns a kolide.Label identified by lid if one exists. func (d *Datastore) Label(lid uint) (*kolide.Label, error) { sql := ` SELECT * FROM labels - WHERE id = ? AND NOT deleted + WHERE id = ? ` label := &kolide.Label{} @@ -264,11 +241,11 @@ func (d *Datastore) Label(lid uint) (*kolide.Label, error) { return label, nil } -// ListLabels returns all labels limited or sorted by kolide.ListOptions +// ListLabels returns all labels limited or sorted by kolide.ListOptions. func (d *Datastore) ListLabels(opt kolide.ListOptions) ([]*kolide.Label, error) { query := ` SELECT *, (SELECT COUNT(1) FROM label_membership WHERE label_id = id) AS host_count - FROM labels WHERE NOT deleted + FROM labels ` query = appendListOptionsToSQL(query, opt) labels := []*kolide.Label{} @@ -464,7 +441,6 @@ func (d *Datastore) searchLabelsWithOmits(query string, omit ...uint) ([]kolide. FROM labels WHERE ( MATCH(name) AGAINST(? IN BOOLEAN MODE) - AND NOT deleted ) AND id NOT IN (?) ORDER BY label_type DESC, id ASC @@ -530,8 +506,7 @@ func (d *Datastore) searchLabelsDefault(omit ...uint) ([]kolide.Label, error) { sqlStatement := ` SELECT *, (SELECT COUNT(1) FROM label_membership WHERE label_id = id) AS host_count FROM labels - WHERE NOT deleted - AND id NOT IN (?) + WHERE id NOT IN (?) GROUP BY id ORDER BY label_type DESC, id ASC LIMIT 5 @@ -540,7 +515,7 @@ func (d *Datastore) searchLabelsDefault(omit ...uint) ([]kolide.Label, error) { var in interface{} { // use -1 if there are no values to omit. - //Avoids empty args error for `sqlx.In` + // Avoids empty args error for `sqlx.In` in = omit if len(omit) == 0 { in = -1 @@ -585,7 +560,6 @@ func (d *Datastore) SearchLabels(query string, omit ...uint) ([]kolide.Label, er FROM labels WHERE ( MATCH(name) AGAINST(? IN BOOLEAN MODE) - AND NOT deleted ) ORDER BY label_type DESC, id ASC LIMIT 10 diff --git a/server/datastore/mysql/migrations/data/20171212182459_DeleteSoftDeletedPacks.go b/server/datastore/mysql/migrations/data/20171212182459_DeleteSoftDeletedPacks.go deleted file mode 100644 index 3964410c7..000000000 --- a/server/datastore/mysql/migrations/data/20171212182459_DeleteSoftDeletedPacks.go +++ /dev/null @@ -1,23 +0,0 @@ -package data - -import ( - "database/sql" - - "github.com/pkg/errors" -) - -func init() { - MigrationClient.AddMigration(Up20171212182459, Down20171212182459) -} - -func Up20171212182459(tx *sql.Tx) error { - sql := `DELETE FROM packs WHERE deleted = 1` - if _, err := tx.Exec(sql); err != nil { - return errors.Wrap(err, "delete packs") - } - return nil -} - -func Down20171212182459(tx *sql.Tx) error { - return nil -} diff --git a/server/datastore/mysql/migrations/data/20181119180000_DeleteSoftDeletedEntities.go b/server/datastore/mysql/migrations/data/20181119180000_DeleteSoftDeletedEntities.go deleted file mode 100644 index 68c60c769..000000000 --- a/server/datastore/mysql/migrations/data/20181119180000_DeleteSoftDeletedEntities.go +++ /dev/null @@ -1,54 +0,0 @@ -package data - -import ( - "database/sql" - - "github.com/pkg/errors" -) - -func init() { - MigrationClient.AddMigration(Up20181119180000, Down20181119180000) -} - -func Up20181119180000(tx *sql.Tx) error { - sql := `DELETE FROM scheduled_queries WHERE deleted = 1` - if _, err := tx.Exec(sql); err != nil { - return errors.Wrap(err, "delete scheduled queries") - } - - sql = `DELETE FROM queries WHERE deleted = 1` - if _, err := tx.Exec(sql); err != nil { - return errors.Wrap(err, "delete queries") - } - - sql = `DELETE FROM labels WHERE deleted = 1` - if _, err := tx.Exec(sql); err != nil { - return errors.Wrap(err, "delete labels") - } - - sql = `DELETE FROM distributed_query_campaigns WHERE deleted = 1` - if _, err := tx.Exec(sql); err != nil { - return errors.Wrap(err, "delete campaigns") - } - - sql = `DELETE FROM hosts WHERE deleted = 1` - if _, err := tx.Exec(sql); err != nil { - return errors.Wrap(err, "delete hosts") - } - - sql = `DELETE FROM invites WHERE deleted = 1` - if _, err := tx.Exec(sql); err != nil { - return errors.Wrap(err, "delete invites") - } - - sql = `DELETE FROM users WHERE deleted = 1` - if _, err := tx.Exec(sql); err != nil { - return errors.Wrap(err, "delete users") - } - - return nil -} - -func Down20181119180000(tx *sql.Tx) error { - return nil -} diff --git a/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go b/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go new file mode 100644 index 000000000..d1379c794 --- /dev/null +++ b/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go @@ -0,0 +1,95 @@ +package tables + +import ( + "fmt" + + "database/sql" + "github.com/kolide/goose" +) + +func init() { + goose.AddMigration(Up20201011162341, Down20201011162341) +} + +func cleanupSoftDeleteFields(tx *sql.Tx, dbTable string) error { + deleteStmt := fmt.Sprintf("DELETE FROM `%s` WHERE deleted;", dbTable) + + _, err := tx.Exec(deleteStmt) + if err != nil { + return err + } + + for _, column := range []string{"deleted", "deleted_at"} { + alterStmt := fmt.Sprint( + "ALTER TABLE `%s` DROP COLUMN `%s`;", dbTable, column) + + _, err := tx.Exec(alterStmt) + if err != nil { + return err + } + } + + return nil +} + +func addSoftDeleteFields(tx *sql.Tx, dbTable string) error { + addDeletedStmt := fmt.Sprint( + "ALTER TABLE `%s` "+ + "ADD COLUMN `deleted` TINYINT(1) NOT NULL DEFAULT FALSE;", + dbTable) + _, err := tx.Exec(addDeletedStmt) + if err != nil { + return err + } + + addDeletedAtStmt := fmt.Sprint( + "ALTER TABLE `%s` "+ + "ADD COLUMN `deleted_at` TIMESTAMP NULL DEFAULT NULL;", + dbTable) + _, err = tx.Exec(addDeletedAtStmt) + if err != nil { + return err + } + + return nil +} + +func getTablesForCleanupSoftDeletedColumnsMigration() []string { + tables := []string{ + "distributed_query_campaigns", + "labels", + "invites", + "hosts", + "packs", + "queries", + "scheduled_queries", + "users", + } + return tables +} + +func Up20201011162341(tx *sql.Tx) error { + tables := getTablesForCleanupSoftDeletedColumnsMigration() + + for _, table := range tables { + err := cleanupSoftDeleteFields(tx, table) + if err != nil { + return err + } + } + + return nil +} + +func Down20201011162341(tx *sql.Tx) error { + tables := getTablesForCleanupSoftDeletedColumnsMigration() + + for _, table := range tables { + err := addSoftDeleteFields(tx, table) + if err != nil { + return err + } + } + + return nil +} diff --git a/server/datastore/mysql/packs.go b/server/datastore/mysql/packs.go index 6254de5a2..b8369a426 100644 --- a/server/datastore/mysql/packs.go +++ b/server/datastore/mysql/packs.go @@ -35,7 +35,7 @@ func applyPackSpec(tx *sqlx.Tx, spec *kolide.PackSpec) error { name = VALUES(name), description = VALUES(description), platform = VALUES(platform), - deleted = false + disabled = VALUES(disabled) ` if _, err := tx.Exec(query, spec.Name, spec.Description, spec.Platform); err != nil { return errors.Wrap(err, "insert/update pack") @@ -202,7 +202,7 @@ func (d *Datastore) PackByName(name string, opts ...kolide.OptionalArg) (*kolide sqlStatement := ` SELECT * FROM packs - WHERE name = ? AND NOT deleted + WHERE name = ? ` var pack kolide.Pack err := db.Get(&pack, sqlStatement, name) @@ -219,35 +219,16 @@ func (d *Datastore) PackByName(name string, opts ...kolide.OptionalArg) (*kolide // NewPack creates a new Pack func (d *Datastore) NewPack(pack *kolide.Pack, opts ...kolide.OptionalArg) (*kolide.Pack, error) { db := d.getTransaction(opts) - var ( - deletedPack kolide.Pack - query string - ) - err := db.Get(&deletedPack, - "SELECT * FROM packs WHERE name = ? AND deleted", pack.Name) - switch err { - case nil: - query = ` - REPLACE INTO packs - ( name, description, platform, disabled, deleted) - VALUES ( ?, ?, ?, ?, ?) - ` - case sql.ErrNoRows: - query = ` - INSERT INTO packs - ( name, description, platform, disabled, deleted) - VALUES ( ?, ?, ?, ?, ?) - ` - default: - return nil, errors.Wrap(err, "check for existing pack") - } - deleted := false - result, err := db.Exec(query, pack.Name, pack.Description, pack.Platform, pack.Disabled, deleted) - if err != nil && isDuplicate(err) { - return nil, alreadyExists("Pack", deletedPack.ID) - } else if err != nil { - return nil, errors.Wrap(err, "creating new pack") + query := ` + INSERT INTO packs + (name, description, platform, disabled) + VALUES ( ?, ?, ?, ?, ?) + ` + + result, err := db.Exec(query, pack.Name, pack.Description, pack.Platform, pack.Disabled) + if err != nil { + return nil, errors.Wrap(err, "inserting pack") } id, _ := result.LastInsertId() @@ -260,7 +241,7 @@ func (d *Datastore) SavePack(pack *kolide.Pack) error { query := ` UPDATE packs SET name = ?, platform = ?, disabled = ?, description = ? - WHERE id = ? AND NOT deleted + WHERE id = ? ` results, err := d.db.Exec(query, pack.Name, pack.Platform, pack.Disabled, pack.Description, pack.ID) @@ -277,14 +258,14 @@ func (d *Datastore) SavePack(pack *kolide.Pack) error { return nil } -// DeletePack soft deletes a kolide.Pack so that it won't show up in results +// DeletePack deletes a kolide.Pack so that it won't show up in results. func (d *Datastore) DeletePack(name string) error { return d.deleteEntityByName("packs", name) } // Pack fetch kolide.Pack with matching ID func (d *Datastore) Pack(pid uint) (*kolide.Pack, error) { - query := `SELECT * FROM packs WHERE id = ? AND NOT deleted` + query := `SELECT * FROM packs WHERE id = ?` pack := &kolide.Pack{} err := d.db.Get(pack, query, pid) if err == sql.ErrNoRows { @@ -298,7 +279,7 @@ func (d *Datastore) Pack(pid uint) (*kolide.Pack, error) { // ListPacks returns all kolide.Pack records limited and sorted by kolide.ListOptions func (d *Datastore) ListPacks(opt kolide.ListOptions) ([]*kolide.Pack, error) { - query := `SELECT * FROM packs WHERE NOT deleted` + query := `SELECT * FROM packs` packs := []*kolide.Pack{} err := d.db.Select(&packs, appendListOptionsToSQL(query, opt)) if err != nil && err != sql.ErrNoRows { @@ -389,7 +370,6 @@ func (d *Datastore) ListLabelsForPack(pid uint) ([]*kolide.Label, error) { pt.type = ? AND pt.pack_id = ? - AND NOT l.deleted ` labels := []*kolide.Label{} diff --git a/server/datastore/mysql/password_reset.go b/server/datastore/mysql/password_reset.go index c6304401b..2de6511aa 100644 --- a/server/datastore/mysql/password_reset.go +++ b/server/datastore/mysql/password_reset.go @@ -46,11 +46,7 @@ func (d *Datastore) SavePasswordResetRequest(req *kolide.PasswordResetRequest) e } func (d *Datastore) DeletePasswordResetRequest(req *kolide.PasswordResetRequest) error { - - sqlStatement := ` - DELETE FROM password_reset_requests WHERE id = ? - ` - _, err := d.db.Exec(sqlStatement, req.ID) + err := d.deleteEntity("password_reset_requests", req.ID) if err != nil { return errors.Wrap(err, "deleting from password reset request") } diff --git a/server/datastore/mysql/queries.go b/server/datastore/mysql/queries.go index 1327e225f..801db92a3 100644 --- a/server/datastore/mysql/queries.go +++ b/server/datastore/mysql/queries.go @@ -34,16 +34,14 @@ func (d *Datastore) ApplyQueries(authorID uint, queries []*kolide.Query) (err er description, query, author_id, - saved, - deleted - ) VALUES ( ?, ?, ?, ?, true, false ) + saved + ) VALUES ( ?, ?, ?, ?, true ) ON DUPLICATE KEY UPDATE name = VALUES(name), description = VALUES(description), query = VALUES(query), author_id = VALUES(author_id), - saved = VALUES(saved), - deleted = VALUES(deleted) + saved = VALUES(saved) ` stmt, err := tx.Prepare(sql) if err != nil { @@ -69,7 +67,7 @@ func (d *Datastore) QueryByName(name string, opts ...kolide.OptionalArg) (*kolid sqlStatement := ` SELECT * FROM queries - WHERE name = ? AND NOT deleted + WHERE name = ? ` var query kolide.Query err := db.Get(&query, sqlStatement, name) @@ -87,47 +85,21 @@ func (d *Datastore) QueryByName(name string, opts ...kolide.OptionalArg) (*kolid return &query, nil } -// NewQuery creates a New Query. If a query with the same name was soft-deleted, -// NewQuery will replace the old one. +// NewQuery creates a New Query. func (d *Datastore) NewQuery(query *kolide.Query, opts ...kolide.OptionalArg) (*kolide.Query, error) { db := d.getTransaction(opts) - var ( - deletedQuery kolide.Query - sqlStatement string - ) - err := db.Get(&deletedQuery, - "SELECT * FROM queries WHERE name = ? AND deleted", query.Name) - switch err { - case nil: - sqlStatement = ` - REPLACE INTO queries ( - name, - description, - query, - saved, - author_id, - deleted - ) VALUES ( ?, ?, ?, ?, ?, ? ) - ` - case sql.ErrNoRows: - sqlStatement = ` - INSERT INTO queries ( - name, - description, - query, - saved, - author_id, - deleted - ) VALUES ( ?, ?, ?, ?, ?, ? ) - ` - default: - return nil, errors.Wrap(err, "check for existing Query") - } - deleted := false - result, err := db.Exec(sqlStatement, query.Name, query.Description, query.Query, query.Saved, query.AuthorID, deleted) - if err != nil && isDuplicate(err) { - return nil, alreadyExists("Query", deletedQuery.ID) - } else if err != nil { + + sqlStatement := ` + INSERT INTO queries ( + name, + description, + query, + saved, + author_id + ) VALUES ( ?, ?, ?, ?, ? ) + ` + result, err := db.Exec(sqlStatement, query.Name, query.Description, query.Query, query.Saved, query.AuthorID) + if err != nil { return nil, errors.Wrap(err, "creating new Query") } @@ -142,7 +114,7 @@ func (d *Datastore) SaveQuery(q *kolide.Query) error { sql := ` UPDATE queries SET name = ?, description = ?, query = ?, author_id = ?, saved = ? - WHERE id = ? AND NOT deleted + WHERE id = ? ` result, err := d.db.Exec(sql, q.Name, q.Description, q.Query, q.AuthorID, q.Saved, q.ID) if err != nil { @@ -159,39 +131,18 @@ func (d *Datastore) SaveQuery(q *kolide.Query) error { return nil } -// DeleteQuery soft deletes Query identified by Query.ID +// DeleteQuery deletes Query identified by Query.ID. func (d *Datastore) DeleteQuery(name string) error { return d.deleteEntityByName("queries", name) } -// DeleteQueries (soft) deletes the existing query objects with the provided -// IDs. The number of deleted queries is returned along with any error. +// DeleteQueries deletes the existing query objects with the provided IDs. The +// number of deleted queries is returned along with any error. func (d *Datastore) DeleteQueries(ids []uint) (uint, error) { - sql := ` - UPDATE queries - SET deleted_at = NOW(), deleted = true - WHERE id IN (?) AND NOT deleted - ` - query, args, err := sqlx.In(sql, ids) - if err != nil { - return 0, errors.Wrap(err, "building delete query query") - } - - result, err := d.db.Exec(query, args...) - if err != nil { - return 0, errors.Wrap(err, "updating delete query") - } - - deleted, err := result.RowsAffected() - if err != nil { - return 0, errors.Wrap(err, "fetching delete query rows effected") - } - - return uint(deleted), nil + return d.deleteEntities("queries", ids) } -// Query returns a single Query identified by id, if such -// exists +// Query returns a single Query identified by id, if such exists. func (d *Datastore) Query(id uint) (*kolide.Query, error) { sql := ` SELECT q.*, COALESCE(NULLIF(u.name, ''), u.username, '') AS author_name @@ -199,7 +150,6 @@ func (d *Datastore) Query(id uint) (*kolide.Query, error) { LEFT JOIN users u ON q.author_id = u.id WHERE q.id = ? - AND NOT q.deleted ` query := &kolide.Query{} if err := d.db.Get(query, sql, id); err != nil { @@ -222,7 +172,6 @@ func (d *Datastore) ListQueries(opt kolide.ListOptions) ([]*kolide.Query, error) LEFT JOIN users u ON q.author_id = u.id WHERE saved = true - AND NOT q.deleted ` sql = appendListOptionsToSQL(sql, opt) results := []*kolide.Query{} @@ -250,7 +199,6 @@ func (d *Datastore) loadPacksForQueries(queries []*kolide.Query) error { JOIN scheduled_queries sq ON p.id = sq.pack_id WHERE query_name IN (?) - AND NOT p.deleted ` // Used to map the results diff --git a/server/datastore/mysql/scheduled_queries.go b/server/datastore/mysql/scheduled_queries.go index 253053ef7..0805a9418 100644 --- a/server/datastore/mysql/scheduled_queries.go +++ b/server/datastore/mysql/scheduled_queries.go @@ -27,7 +27,6 @@ func (d *Datastore) ListScheduledQueriesInPack(id uint, opts kolide.ListOptions) JOIN queries q ON sq.query_name = q.name WHERE sq.pack_id = ? - AND NOT sq.deleted ` query = appendListOptionsToSQL(query, opts) results := []*kolide.ScheduledQuery{} @@ -95,7 +94,7 @@ func (d *Datastore) SaveScheduledQuery(sq *kolide.ScheduledQuery) (*kolide.Sched query := ` UPDATE scheduled_queries SET pack_id = ?, query_id = ?, ` + "`interval`" + ` = ?, snapshot = ?, removed = ?, platform = ?, version = ?, shard = ? - WHERE id = ? AND NOT deleted + WHERE id = ? ` result, err := d.db.Exec(query, sq.PackID, sq.QueryID, sq.Interval, sq.Snapshot, sq.Removed, sq.Platform, sq.Version, sq.Shard, sq.ID) if err != nil { @@ -137,7 +136,6 @@ func (d *Datastore) ScheduledQuery(id uint) (*kolide.ScheduledQuery, error) { JOIN queries q ON sq.query_name = q.name WHERE sq.id = ? - AND NOT sq.deleted ` sq := &kolide.ScheduledQuery{} if err := d.db.Get(sq, query, id); err != nil { diff --git a/server/datastore/mysql/sessions.go b/server/datastore/mysql/sessions.go index c5c00fd9c..48428e140 100644 --- a/server/datastore/mysql/sessions.go +++ b/server/datastore/mysql/sessions.go @@ -68,10 +68,7 @@ func (d *Datastore) NewSession(session *kolide.Session) (*kolide.Session, error) } func (d *Datastore) DestroySession(session *kolide.Session) error { - sqlStatement := ` - DELETE FROM sessions WHERE id = ? - ` - _, err := d.db.Exec(sqlStatement, session.ID) + err := d.deleteEntity("sessions", session.ID) if err != nil { return errors.Wrap(err, "deleting session") } diff --git a/server/datastore/mysql/targets.go b/server/datastore/mysql/targets.go index 1dbfa585b..952b807ab 100644 --- a/server/datastore/mysql/targets.go +++ b/server/datastore/mysql/targets.go @@ -27,7 +27,6 @@ func (d *Datastore) CountHostsInTargets(hostIDs []uint, labelIDs []uint, now tim COALESCE(SUM(CASE WHEN DATE_ADD(created_at, INTERVAL 1 DAY) >= ? THEN 1 ELSE 0 END), 0) new FROM hosts h WHERE (id IN (?) OR (id IN (SELECT DISTINCT host_id FROM label_membership WHERE label_id IN (?)))) - AND NOT deleted `, kolide.OnlineIntervalBuffer, kolide.OnlineIntervalBuffer) // Using -1 in the ID slices for the IN clause allows us to include the diff --git a/server/datastore/mysql/users.go b/server/datastore/mysql/users.go index d9686d8ea..1d03b84bc 100644 --- a/server/datastore/mysql/users.go +++ b/server/datastore/mysql/users.go @@ -40,7 +40,7 @@ func (d *Datastore) NewUser(user *kolide.User) (*kolide.User, error) { func (d *Datastore) findUser(searchCol string, searchVal interface{}) (*kolide.User, error) { sqlStatement := fmt.Sprintf( "SELECT * FROM users "+ - "WHERE %s = ? AND NOT deleted LIMIT 1", + "WHERE %s = ? LIMIT 1", searchCol, ) @@ -66,7 +66,7 @@ func (d *Datastore) User(username string) (*kolide.User, error) { // kolide.ListOptions func (d *Datastore) ListUsers(opt kolide.ListOptions) ([]*kolide.User, error) { sqlStatement := ` - SELECT * FROM users WHERE NOT deleted + SELECT * FROM users ` sqlStatement = appendListOptionsToSQL(sqlStatement, opt) users := []*kolide.User{} diff --git a/server/kolide/campaigns.go b/server/kolide/campaigns.go index e23634ff6..ed0d8f699 100644 --- a/server/kolide/campaigns.go +++ b/server/kolide/campaigns.go @@ -66,7 +66,6 @@ const ( // query. type DistributedQueryCampaign struct { UpdateCreateTimestamps - DeleteFields Metrics TargetMetrics ID uint `json:"id"` QueryID uint `json:"query_id" db:"query_id"` diff --git a/server/kolide/hosts.go b/server/kolide/hosts.go index 238ea3044..8a45721ba 100644 --- a/server/kolide/hosts.go +++ b/server/kolide/hosts.go @@ -92,7 +92,6 @@ type HostListOptions struct { type Host struct { UpdateCreateTimestamps - DeleteFields ID uint `json:"id"` // OsqueryHostID is the key used in the request context that is // used to retrieve host information. It is sent from osquery and may currently be diff --git a/server/kolide/invites.go b/server/kolide/invites.go index 365ef7fea..51d9cccb8 100644 --- a/server/kolide/invites.go +++ b/server/kolide/invites.go @@ -59,7 +59,6 @@ type InvitePayload struct { // Invite represents an invitation for a user to join Fleet. type Invite struct { UpdateCreateTimestamps - DeleteFields ID uint `json:"id"` InvitedBy uint `json:"invited_by" db:"invited_by"` Email string `json:"email"` diff --git a/server/kolide/labels.go b/server/kolide/labels.go index 28195e84a..67936e47e 100644 --- a/server/kolide/labels.go +++ b/server/kolide/labels.go @@ -167,7 +167,6 @@ func (t *LabelMembershipType) UnmarshalJSON(b []byte) error { type Label struct { UpdateCreateTimestamps - DeleteFields ID uint `json:"id"` Name string `json:"name"` Description string `json:"description"` diff --git a/server/kolide/packs.go b/server/kolide/packs.go index deb7ea434..4dcb76c77 100644 --- a/server/kolide/packs.go +++ b/server/kolide/packs.go @@ -122,7 +122,6 @@ type PackService interface { // Pack is the structure which represents an osquery query pack. type Pack struct { UpdateCreateTimestamps - DeleteFields ID uint `json:"id"` Name string `json:"name"` Description string `json:"description"` diff --git a/server/kolide/queries.go b/server/kolide/queries.go index 8c918d03b..ff0ec5006 100644 --- a/server/kolide/queries.go +++ b/server/kolide/queries.go @@ -21,9 +21,8 @@ type QueryStore interface { SaveQuery(query *Query) error // DeleteQuery deletes an existing query object. DeleteQuery(name string) error - // DeleteQueries (soft) deletes the existing query objects with the - // provided IDs. The number of deleted queries is returned along with - // any error. + // DeleteQueries deletes the existing query objects with the provided IDs. + // The number of deleted queries is returned along with any error. DeleteQueries(ids []uint) (uint, error) // Query returns the query associated with the provided ID. Associated // packs should also be loaded. @@ -54,9 +53,8 @@ type QueryService interface { DeleteQuery(ctx context.Context, name string) error // For backwards compatibility with UI DeleteQueryByID(ctx context.Context, id uint) error - // DeleteQueries (soft) deletes the existing query objects with the - // provided IDs. The number of deleted queries is returned along with - // any error. + // DeleteQueries deletes the existing query objects with the provided IDs. + // The number of deleted queries is returned along with any error. DeleteQueries(ctx context.Context, ids []uint) (uint, error) } @@ -68,7 +66,6 @@ type QueryPayload struct { type Query struct { UpdateCreateTimestamps - DeleteFields ID uint `json:"id"` Name string `json:"name"` Description string `json:"description"` diff --git a/server/kolide/scheduled_queries.go b/server/kolide/scheduled_queries.go index a41d48eed..d51438ec4 100644 --- a/server/kolide/scheduled_queries.go +++ b/server/kolide/scheduled_queries.go @@ -24,7 +24,6 @@ type ScheduledQueryService interface { type ScheduledQuery struct { UpdateCreateTimestamps - DeleteFields ID uint `json:"id"` PackID uint `json:"pack_id" db:"pack_id"` Name string `json:"name"` diff --git a/server/kolide/traits.go b/server/kolide/traits.go index 128f7b7f3..cec4db90f 100644 --- a/server/kolide/traits.go +++ b/server/kolide/traits.go @@ -7,14 +7,6 @@ type CreateTimestamp struct { CreatedAt time.Time `json:"created_at" db:"created_at"` } -// Deleteable is used to indicate a record is deleted. We don't actually -// delete record in the database. We mark it deleted, records with Deleted -// set to true will not normally be included in results -type DeleteFields struct { - DeletedAt *time.Time `json:"deleted_at" db:"deleted_at"` - Deleted bool `json:"deleted"` -} - // 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/kolide/users.go b/server/kolide/users.go index 96f7ab319..142d77d26 100644 --- a/server/kolide/users.go +++ b/server/kolide/users.go @@ -87,7 +87,6 @@ type UserService interface { // User is the model struct which represents a kolide user type User struct { UpdateCreateTimestamps - DeleteFields ID uint `json:"id"` Username string `json:"username"` Password []byte `json:"-"` diff --git a/server/service/service_sessions.go b/server/service/service_sessions.go index e8e1357aa..cbc9440ef 100644 --- a/server/service/service_sessions.go +++ b/server/service/service_sessions.go @@ -101,7 +101,7 @@ func (svc service) CallbackSSO(ctx context.Context, auth kolide.Auth) (*kolide.S return nil, errors.Wrap(err, "finding user in sso callback") } // if user is not active they are not authorized to use the application - if !user.Enabled || user.Deleted { + if !user.Enabled { return nil, errors.New("user authorization failed") } // if the user is not sso enabled they are not authorized diff --git a/tools/api/README.md b/tools/api/README.md index 86388fd1a..723150922 100644 --- a/tools/api/README.md +++ b/tools/api/README.md @@ -20,8 +20,6 @@ export FLEET_ENV_PATH=/Users/victor/fleet_env "user": { "created_at": "2018-04-10T02:07:46Z", "updated_at": "2018-04-10T02:07:46Z", - "deleted_at": null, - "deleted": false, "id": 1, "username": "admin", "name": "admin", @@ -50,8 +48,6 @@ export FLEET_ENV_PATH=/Users/victor/fleet_env "query": { "created_at": "0001-01-01T00:00:00Z", "updated_at": "0001-01-01T00:00:00Z", - "deleted_at": null, - "deleted": false, "id": 4, "name": "system_info", "description": "", diff --git a/tools/app/demo.sql b/tools/app/demo.sql index 254ecb68d..76fbb6590 100644 --- a/tools/app/demo.sql +++ b/tools/app/demo.sql @@ -117,8 +117,6 @@ CREATE TABLE `distributed_query_campaigns` ( `id` int(10) unsigned NOT NULL AUTO_INCREMENT, `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` timestamp NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, - `deleted_at` timestamp NULL DEFAULT NULL, - `deleted` tinyint(1) NOT NULL DEFAULT '0', `query_id` int(10) unsigned DEFAULT NULL, `status` int(11) DEFAULT NULL, `user_id` int(10) unsigned DEFAULT NULL, @@ -227,8 +225,6 @@ CREATE TABLE `hosts` ( `osquery_host_id` varchar(255) NOT NULL, `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` timestamp NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, - `deleted_at` timestamp NULL DEFAULT NULL, - `deleted` tinyint(1) NOT NULL DEFAULT '0', `detail_update_time` timestamp NULL DEFAULT NULL, `node_key` varchar(255) DEFAULT NULL, `host_name` varchar(255) NOT NULL DEFAULT '', @@ -280,8 +276,6 @@ CREATE TABLE `invites` ( `id` int(10) unsigned NOT NULL AUTO_INCREMENT, `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` timestamp NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, - `deleted_at` timestamp NULL DEFAULT NULL, - `deleted` tinyint(1) NOT NULL DEFAULT '0', `invited_by` int(10) unsigned NOT NULL, `email` varchar(255) NOT NULL, `admin` tinyint(1) DEFAULT NULL, @@ -342,8 +336,6 @@ CREATE TABLE `labels` ( `id` int(10) unsigned NOT NULL AUTO_INCREMENT, `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` timestamp NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, - `deleted_at` timestamp NULL DEFAULT NULL, - `deleted` tinyint(1) NOT NULL DEFAULT '0', `name` varchar(255) NOT NULL, `description` varchar(255) DEFAULT NULL, `query` text NOT NULL, @@ -627,8 +619,6 @@ CREATE TABLE `packs` ( `id` int(10) unsigned NOT NULL AUTO_INCREMENT, `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` timestamp NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, - `deleted_at` timestamp NULL DEFAULT NULL, - `deleted` tinyint(1) NOT NULL DEFAULT '0', `disabled` tinyint(1) NOT NULL DEFAULT '0', `name` varchar(255) NOT NULL, `description` varchar(255) DEFAULT NULL, @@ -694,8 +684,6 @@ CREATE TABLE `queries` ( `id` int(10) unsigned NOT NULL AUTO_INCREMENT, `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` timestamp NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, - `deleted_at` timestamp NULL DEFAULT NULL, - `deleted` tinyint(1) NOT NULL DEFAULT '0', `saved` tinyint(1) NOT NULL DEFAULT '0', `name` varchar(255) NOT NULL, `description` text NOT NULL, @@ -763,8 +751,6 @@ CREATE TABLE `scheduled_queries` ( `id` int(10) unsigned NOT NULL AUTO_INCREMENT, `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` timestamp NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, - `deleted_at` timestamp NULL DEFAULT NULL, - `deleted` tinyint(1) NOT NULL DEFAULT '0', `pack_id` int(10) unsigned DEFAULT NULL, `query_id` int(10) unsigned DEFAULT NULL, `interval` int(10) unsigned DEFAULT NULL, @@ -893,8 +879,6 @@ CREATE TABLE `users` ( `id` int(10) unsigned NOT NULL AUTO_INCREMENT, `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` timestamp NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, - `deleted_at` timestamp NULL DEFAULT NULL, - `deleted` tinyint(1) NOT NULL DEFAULT '0', `username` varchar(255) NOT NULL, `password` varbinary(255) NOT NULL, `salt` varchar(255) NOT NULL, From a8faf6655dadebf32283f4f3291796677d5f0622 Mon Sep 17 00:00:00 2001 From: Brendan Shaklovitz Date: Fri, 16 Oct 2020 19:54:35 -0500 Subject: [PATCH 2/7] Fix migration and skip non-existent columns * Change `goose.AddMigration` to `MigrationClient.AddMigration` * If columns don't exist, skip instead of failing the migration. This might happen in the case of a partial or broken migration. --- ...0201011162341_CleanupSoftDeletedColumns.go | 36 +++++++++++++++---- .../mysql/migrations/tables/migration.go | 14 ++++++++ 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go b/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go index d1379c794..54ca95547 100644 --- a/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go +++ b/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go @@ -1,31 +1,55 @@ package tables import ( + "database/sql" "fmt" - "database/sql" - "github.com/kolide/goose" + "github.com/go-sql-driver/mysql" ) func init() { - goose.AddMigration(Up20201011162341, Down20201011162341) + MigrationClient.AddMigration(Up20201011162341, Down20201011162341) } func cleanupSoftDeleteFields(tx *sql.Tx, dbTable string) error { deleteStmt := fmt.Sprintf("DELETE FROM `%s` WHERE deleted;", dbTable) + fmt.Println("DEBUG:", deleteStmt) _, err := tx.Exec(deleteStmt) if err != nil { - return err + mysqlErr, ok := err.(*mysql.MySQLError) + if !ok { + return err + } + + if mysqlErr.Number != ER_BAD_FIELD_ERROR { + return err + } + fmt.Printf( + "Skipped deleting 'soft-deleted' entries from '%s' because the "+ + "'deleted' column does not exist.\n", + dbTable) } for _, column := range []string{"deleted", "deleted_at"} { - alterStmt := fmt.Sprint( + alterStmt := fmt.Sprintf( "ALTER TABLE `%s` DROP COLUMN `%s`;", dbTable, column) + fmt.Println("DEBUG:", alterStmt) _, err := tx.Exec(alterStmt) if err != nil { - return err + mysqlErr, ok := err.(*mysql.MySQLError) + if !ok { + return err + } + + if mysqlErr.Number != ER_CANT_DROP_FIELD_OR_KEY { + return err + } + fmt.Printf( + "Skipped dropping column '%s' on table '%s' because column "+ + "does not exist.\n", + column, dbTable) } } diff --git a/server/datastore/mysql/migrations/tables/migration.go b/server/datastore/mysql/migrations/tables/migration.go index 5fb35fb4f..bee0e4b7d 100644 --- a/server/datastore/mysql/migrations/tables/migration.go +++ b/server/datastore/mysql/migrations/tables/migration.go @@ -5,3 +5,17 @@ import "github.com/kolide/goose" var ( MigrationClient = goose.New("migration_status_tables", goose.MySqlDialect{}) ) + +const ( + // Unknown column '%s' in '%s' + // + // This error occurs when you try to use a column that doesn't exist in the + // WHERE clause of a statement. + ER_BAD_FIELD_ERROR = 1054 + + // Message: Can't DROP '%s'; check that column/key exists + // + // This error occurs when you try to use a column that doesn't exist in the + // DROP clause of an ALTER statement. + ER_CANT_DROP_FIELD_OR_KEY = 1091 +) From 2c4cac62e9194ce091b11cc31f02d05b68e8e9f9 Mon Sep 17 00:00:00 2001 From: Brendan Shaklovitz Date: Mon, 19 Oct 2020 12:45:30 -0500 Subject: [PATCH 3/7] Update migration docs * Specify changes that need to be made after running the goose create command. --- docs/development/database-migrations.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/development/database-migrations.md b/docs/development/database-migrations.md index 2767ff7a8..74d2b5b62 100644 --- a/docs/development/database-migrations.md +++ b/docs/development/database-migrations.md @@ -15,7 +15,13 @@ cd server/datastore/mysql/migrations/tables goose create AddColumnFooToUsers ``` -Find the file you created in the migrations directory and edit it. You can then update the database by running the following shell commands: +Find the file you created in the migrations directory and edit it: + +* delete the import line for goose: `github.com/pressly/goose` +* change `goose.AddMigration(...)` to `MigrationClient.AddMigration(...)` +* add your migration code + +You can then update the database by running the following shell commands: ``` bash make build From 607081451869eda73a6fe8d6617605712ade4a6c Mon Sep 17 00:00:00 2001 From: Brendan Shaklovitz Date: Mon, 19 Oct 2020 13:12:09 -0500 Subject: [PATCH 4/7] Remove test for idempotency with host delete * Without soft-delete, host deleting is no longer idempotent. Removing the test for this. --- server/datastore/datastore_hosts_test.go | 22 ---------------------- server/datastore/datastore_test.go | 1 - 2 files changed, 23 deletions(-) diff --git a/server/datastore/datastore_hosts_test.go b/server/datastore/datastore_hosts_test.go index b63fad8f8..67336975f 100644 --- a/server/datastore/datastore_hosts_test.go +++ b/server/datastore/datastore_hosts_test.go @@ -110,28 +110,6 @@ func testDeleteHost(t *testing.T, ds kolide.Datastore) { assert.NotNil(t, err) } -func testIdempotentDeleteHost(t *testing.T, ds kolide.Datastore) { - host, err := ds.NewHost(&kolide.Host{ - DetailUpdateTime: time.Now(), - LabelUpdateTime: time.Now(), - SeenTime: time.Now(), - NodeKey: "1", - UUID: "1", - HostName: "foo.local", - }) - require.Nil(t, err) - require.NotNil(t, host) - id := host.ID - err = ds.DeleteHost(host.ID) - assert.Nil(t, err) - - host, err = ds.Host(host.ID) - assert.NotNil(t, err) - - err = ds.DeleteHost(id) - assert.Nil(t, err) -} - func testListHost(t *testing.T, ds kolide.Datastore) { hosts := []*kolide.Host{} for i := 0; i < 10; i++ { diff --git a/server/datastore/datastore_test.go b/server/datastore/datastore_test.go index cacaadb79..482a68229 100644 --- a/server/datastore/datastore_test.go +++ b/server/datastore/datastore_test.go @@ -62,7 +62,6 @@ var testFunctions = [...]func(*testing.T, kolide.Datastore){ testMarkHostSeen, testCleanupIncomingHosts, testDuplicateNewQuery, - testIdempotentDeleteHost, testChangeEmail, testChangeLabelDetails, testMigrationStatus, From 6b576ade2af05f72368904540d783f0b6bc31dab Mon Sep 17 00:00:00 2001 From: Brendan Shaklovitz Date: Mon, 19 Oct 2020 13:53:04 -0500 Subject: [PATCH 5/7] Use VividCortex/mysqlerr constants * Replace definitions with the mysqlerr package constants instead of defining them here. --- .../20201011162341_CleanupSoftDeletedColumns.go | 5 +++-- .../datastore/mysql/migrations/tables/migration.go | 14 -------------- 2 files changed, 3 insertions(+), 16 deletions(-) diff --git a/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go b/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go index 54ca95547..b96c2fb2c 100644 --- a/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go +++ b/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go @@ -4,6 +4,7 @@ import ( "database/sql" "fmt" + "github.com/VividCortex/mysqlerr" "github.com/go-sql-driver/mysql" ) @@ -22,7 +23,7 @@ func cleanupSoftDeleteFields(tx *sql.Tx, dbTable string) error { return err } - if mysqlErr.Number != ER_BAD_FIELD_ERROR { + if mysqlErr.Number != mysqlerr.ER_BAD_FIELD_ERROR { return err } fmt.Printf( @@ -43,7 +44,7 @@ func cleanupSoftDeleteFields(tx *sql.Tx, dbTable string) error { return err } - if mysqlErr.Number != ER_CANT_DROP_FIELD_OR_KEY { + if mysqlErr.Number != mysqlerr.ER_CANT_DROP_FIELD_OR_KEY { return err } fmt.Printf( diff --git a/server/datastore/mysql/migrations/tables/migration.go b/server/datastore/mysql/migrations/tables/migration.go index bee0e4b7d..5fb35fb4f 100644 --- a/server/datastore/mysql/migrations/tables/migration.go +++ b/server/datastore/mysql/migrations/tables/migration.go @@ -5,17 +5,3 @@ import "github.com/kolide/goose" var ( MigrationClient = goose.New("migration_status_tables", goose.MySqlDialect{}) ) - -const ( - // Unknown column '%s' in '%s' - // - // This error occurs when you try to use a column that doesn't exist in the - // WHERE clause of a statement. - ER_BAD_FIELD_ERROR = 1054 - - // Message: Can't DROP '%s'; check that column/key exists - // - // This error occurs when you try to use a column that doesn't exist in the - // DROP clause of an ALTER statement. - ER_CANT_DROP_FIELD_OR_KEY = 1091 -) From 102ea1b91946ef1e114bc50a7807d6cf1e67c7f3 Mon Sep 17 00:00:00 2001 From: Brendan Shaklovitz Date: Mon, 19 Oct 2020 13:54:00 -0500 Subject: [PATCH 6/7] Fix duplicate query insert error message * Return appropriate error message when attempting to insert a query by ID that already exists in the DB. --- server/datastore/mysql/queries.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/datastore/mysql/queries.go b/server/datastore/mysql/queries.go index 801db92a3..9a1f11654 100644 --- a/server/datastore/mysql/queries.go +++ b/server/datastore/mysql/queries.go @@ -99,7 +99,10 @@ func (d *Datastore) NewQuery(query *kolide.Query, opts ...kolide.OptionalArg) (* ) VALUES ( ?, ?, ?, ?, ? ) ` result, err := db.Exec(sqlStatement, query.Name, query.Description, query.Query, query.Saved, query.AuthorID) - if err != nil { + + if err != nil && isDuplicate(err) { + return nil, alreadyExists("Query", 0) + } else if err != nil { return nil, errors.Wrap(err, "creating new Query") } From 214945798d681630e1885603498e78330920f7f0 Mon Sep 17 00:00:00 2001 From: Zachary Wasserman Date: Wed, 21 Oct 2020 16:27:19 -0700 Subject: [PATCH 7/7] Remove debug prints --- .../tables/20201011162341_CleanupSoftDeletedColumns.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go b/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go index b96c2fb2c..36c6c5ad6 100644 --- a/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go +++ b/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go @@ -14,7 +14,6 @@ func init() { func cleanupSoftDeleteFields(tx *sql.Tx, dbTable string) error { deleteStmt := fmt.Sprintf("DELETE FROM `%s` WHERE deleted;", dbTable) - fmt.Println("DEBUG:", deleteStmt) _, err := tx.Exec(deleteStmt) if err != nil { @@ -36,7 +35,6 @@ func cleanupSoftDeleteFields(tx *sql.Tx, dbTable string) error { alterStmt := fmt.Sprintf( "ALTER TABLE `%s` DROP COLUMN `%s`;", dbTable, column) - fmt.Println("DEBUG:", alterStmt) _, err := tx.Exec(alterStmt) if err != nil { mysqlErr, ok := err.(*mysql.MySQLError)