dashboard version cleanup: more tests and refactor

pkg/services/sqlstore/dashboard_version.go

@@ -67,10 +67,10 @@ func GetDashboardVersions(query *m.GetDashboardVersionsQuery) error {
 	return nil
 	return nil
 }
 }
 
 
+const MAX_VERSIONS_TO_DELETE = 100
+
 func DeleteExpiredVersions(cmd *m.DeleteExpiredVersionsCommand) error {
 func DeleteExpiredVersions(cmd *m.DeleteExpiredVersionsCommand) error {
 	return inTransaction(func(sess *DBSession) error {
 	return inTransaction(func(sess *DBSession) error {
-		const MAX_VERSIONS_TO_DELETE = 100
-
 		versionsToKeep := setting.DashboardVersionsToKeep
 		versionsToKeep := setting.DashboardVersionsToKeep
 		if versionsToKeep < 1 {
 		if versionsToKeep < 1 {
 			versionsToKeep = 1
 			versionsToKeep = 1
@@ -80,27 +80,25 @@ func DeleteExpiredVersions(cmd *m.DeleteExpiredVersionsCommand) error {
 		// min_version_to_keep = min_version + (versions_count - versions_to_keep)
 		// min_version_to_keep = min_version + (versions_count - versions_to_keep)
 		// where version stats is processed for each dashboard. This guarantees that we keep at least versions_to_keep
 		// where version stats is processed for each dashboard. This guarantees that we keep at least versions_to_keep
 		// versions, but in some cases (when versions are sparse) this number may be more.
 		// versions, but in some cases (when versions are sparse) this number may be more.
-		versionIdsToDeleteSubquery := `SELECT id
+		versionIdsToDeleteQuery := `SELECT id
 			FROM dashboard_version, (
 			FROM dashboard_version, (
 				SELECT dashboard_id, count(version) as count, min(version) as min
 				SELECT dashboard_id, count(version) as count, min(version) as min
 				FROM dashboard_version
 				FROM dashboard_version
 				GROUP BY dashboard_id
 				GROUP BY dashboard_id
-				) AS vtd
-				WHERE dashboard_version.dashboard_id=vtd.dashboard_id
-				AND version < vtd.min + vtd.count - ?`
+			) AS vtd
+			WHERE dashboard_version.dashboard_id=vtd.dashboard_id
+			AND version < vtd.min + vtd.count - ?`
 
 
 		var versionIdsToDelete []interface{}
 		var versionIdsToDelete []interface{}
-		err := sess.SQL(versionIdsToDeleteSubquery, versionsToKeep).Find(&versionIdsToDelete)
+		err := sess.SQL(versionIdsToDeleteQuery, versionsToKeep).Find(&versionIdsToDelete)
 		if err != nil {
 		if err != nil {
 			return err
 			return err
 		}
 		}
 
 
 		// Don't delete more than MAX_VERSIONS_TO_DELETE version per time
 		// Don't delete more than MAX_VERSIONS_TO_DELETE version per time
-		limit := MAX_VERSIONS_TO_DELETE
-		if len(versionIdsToDelete) < MAX_VERSIONS_TO_DELETE {
-			limit = len(versionIdsToDelete)
+		if len(versionIdsToDelete) > MAX_VERSIONS_TO_DELETE {
+			versionIdsToDelete = versionIdsToDelete[:MAX_VERSIONS_TO_DELETE]
 		}
 		}
-		versionIdsToDelete = versionIdsToDelete[:limit]
 
 
 		if len(versionIdsToDelete) > 0 {
 		if len(versionIdsToDelete) > 0 {
 			deleteExpiredSql := `DELETE FROM dashboard_version WHERE id IN (?` + strings.Repeat(",?", len(versionIdsToDelete)-1) + `)`
 			deleteExpiredSql := `DELETE FROM dashboard_version WHERE id IN (?` + strings.Repeat(",?", len(versionIdsToDelete)-1) + `)`

pkg/services/sqlstore/dashboard_version_test.go

@@ -141,5 +141,25 @@ func TestDeleteExpiredVersions(t *testing.T) {
 
 
 			So(len(query.Result), ShouldEqual, versionsToWrite)
 			So(len(query.Result), ShouldEqual, versionsToWrite)
 		})
 		})
+
+		Convey("Don't delete more than MAX_VERSIONS_TO_DELETE per iteration", func() {
+			versionsToWriteBigNumber := MAX_VERSIONS_TO_DELETE + versionsToWrite
+			for i := 0; i < versionsToWriteBigNumber-versionsToWrite; i++ {
+				updateTestDashboard(savedDash, map[string]interface{}{
+					"tags": "different-tag",
+				})
+			}
+
+			err := DeleteExpiredVersions(&m.DeleteExpiredVersionsCommand{})
+			So(err, ShouldBeNil)
+
+			query := m.GetDashboardVersionsQuery{DashboardId: savedDash.Id, OrgId: 1, Limit: versionsToWriteBigNumber}
+			GetDashboardVersions(&query)
+
+			// Ensure we have at least versionsToKeep versions
+			So(len(query.Result), ShouldBeGreaterThanOrEqualTo, versionsToKeep)
+			// Ensure we haven't deleted more than MAX_VERSIONS_TO_DELETE rows
+			So(versionsToWriteBigNumber-len(query.Result), ShouldBeLessThanOrEqualTo, MAX_VERSIONS_TO_DELETE)
+		})
 	})
 	})
 }
 }