diff --git a/backend/mr-db-sql/src/main/kotlin/io/featurehub/db/services/ApplicationRolloutStrategySqlApi.kt b/backend/mr-db-sql/src/main/kotlin/io/featurehub/db/services/ApplicationRolloutStrategySqlApi.kt index 5cb6faa43..5148a8707 100644 --- a/backend/mr-db-sql/src/main/kotlin/io/featurehub/db/services/ApplicationRolloutStrategySqlApi.kt +++ b/backend/mr-db-sql/src/main/kotlin/io/featurehub/db/services/ApplicationRolloutStrategySqlApi.kt @@ -19,7 +19,7 @@ import java.util.* class ApplicationRolloutStrategySqlApi @Inject constructor( private val conversions: Conversions, - private val archiveStrategy: ArchiveStrategy, private val internalFeatureApi: InternalFeatureApi + private val internalFeatureApi: InternalFeatureApi ) : ApplicationRolloutStrategyApi { override fun createStrategy( @@ -118,6 +118,8 @@ class ApplicationRolloutStrategySqlApi @Inject constructor( if (strategy.application.id == app.id) { var notifyAttachedFeatures = false + val originalRolloutStrategy = InternalFeatureApi.toRolloutStrategy(strategy) + // check if we are renaming it and if so, are we using a duplicate name update.name?.let { newName -> if (!strategy.name.equals(newName, ignoreCase = true)) { @@ -166,7 +168,7 @@ class ApplicationRolloutStrategySqlApi @Inject constructor( // now we have to update all of the tagged features, add an additional history element, force republishing of everything associated strategy.sharedRolloutStrategies?.let { strategies -> strategies.forEach { strategyForFeatureValue -> - internalFeatureApi.updatedApplicationStrategy(strategyForFeatureValue, p) + internalFeatureApi.updatedApplicationStrategy(strategyForFeatureValue, originalRolloutStrategy, p) } } } @@ -253,6 +255,8 @@ class ApplicationRolloutStrategySqlApi @Inject constructor( // only update and publish if it _actually_ changed. We do this here instead of in ArchiveStrategy // because its not a simple publish. That class normally orchestrates it, but its simply too complex. if (strategy.whenArchived == null) { + val originalRolloutStrategy = InternalFeatureApi.toRolloutStrategy(strategy) + strategy.whenArchived = LocalDateTime.now() strategy.name = (strategy.name + Conversions.archivePrefix + isoDate.format(strategy.whenArchived)).take(150) strategy.whoChanged = p @@ -264,7 +268,7 @@ class ApplicationRolloutStrategySqlApi @Inject constructor( copy.forEach { strategyForFeatureValue -> // this needs to remove the connection, create an audit trail, and publish a new record to Edge, and trigger webhooks - internalFeatureApi.detachApplicationStrategy(strategyForFeatureValue, strategy, p) + internalFeatureApi.detachApplicationStrategy(strategyForFeatureValue, originalRolloutStrategy, p) } } } diff --git a/backend/mr-db-sql/src/main/kotlin/io/featurehub/db/services/FeatureSqlApi.kt b/backend/mr-db-sql/src/main/kotlin/io/featurehub/db/services/FeatureSqlApi.kt index 4f8d09360..e1ffa7fc3 100644 --- a/backend/mr-db-sql/src/main/kotlin/io/featurehub/db/services/FeatureSqlApi.kt +++ b/backend/mr-db-sql/src/main/kotlin/io/featurehub/db/services/FeatureSqlApi.kt @@ -36,15 +36,30 @@ interface InternalFeatureApi { fun forceVersionBump(featureIds: List, envId: UUID) fun updatedApplicationStrategy( strategyForFeatureValue: DbStrategyForFeatureValue, + originalStrategy: RolloutStrategy, personWhoUpdated: DbPerson ) fun detachApplicationStrategy( strategyForFeatureValue: DbStrategyForFeatureValue, - strategy: DbApplicationRolloutStrategy, + originalStrategy: RolloutStrategy, personWhoArchived: DbPerson ) - fun toRolloutStrategy(sharedStrategy: DbStrategyForFeatureValue): RolloutStrategy + companion object { + fun toRolloutStrategy(appStrategy: DbApplicationRolloutStrategy): RolloutStrategy { + return RolloutStrategy().id(appStrategy.shortUniqueCode) + .percentage(appStrategy.strategy.percentage) + .percentageAttributes(appStrategy.strategy.percentageAttributes) + .attributes(appStrategy.strategy.attributes) + .avatar(appStrategy.strategy.avatar) + .colouring(appStrategy.strategy.colouring) + .name(appStrategy.name).disabled(false) + } + + fun toRolloutStrategy(sharedStrategy: DbStrategyForFeatureValue): RolloutStrategy { + return toRolloutStrategy(sharedStrategy.rolloutStrategy).value(sharedStrategy.value) + } + } } class InternalFeatureSqlApi @Inject constructor(private val convertUtils: Conversions, @@ -67,10 +82,6 @@ class InternalFeatureSqlApi @Inject constructor(private val convertUtils: Conver } } - override fun toRolloutStrategy(sharedStrategy: DbStrategyForFeatureValue): RolloutStrategy { - val appStrategy = sharedStrategy.rolloutStrategy - return RolloutStrategy().id(appStrategy.shortUniqueCode).name(appStrategy.name).value(sharedStrategy.value).disabled(false) - } override fun forceVersionBump(featureIds: List, envId: UUID) { // force a version change on all these features @@ -84,11 +95,12 @@ class InternalFeatureSqlApi @Inject constructor(private val convertUtils: Conver override fun updatedApplicationStrategy( strategyForFeatureValue: DbStrategyForFeatureValue, + originalStrategy: RolloutStrategy, personWhoUpdated: DbPerson ) { // we need to bump the feature value version even though nothing ostensibly changed strategyForFeatureValue.featureValue.let { fv -> - val priorStrategies = fv.sharedRolloutStrategies.map { toRolloutStrategy(it) } + val priorStrategies = fv.sharedRolloutStrategies.map { InternalFeatureApi.toRolloutStrategy(it) } fv.whoUpdated = personWhoUpdated saveFeatureValue(fv, true) @@ -96,12 +108,16 @@ class InternalFeatureSqlApi @Inject constructor(private val convertUtils: Conver // this will cause an audit webhook automatically cacheSource.publishFeatureChange(fv) - publishFeatureMessage(fv, strategyForFeatureValue, "updated", priorStrategies) + publishFeatureMessage(fv, "updated", priorStrategies, originalStrategy, + InternalFeatureApi.toRolloutStrategy(strategyForFeatureValue)) } } private fun publishFeatureMessage(fv: DbFeatureValue, - ue: DbStrategyForFeatureValue, action: String, priorStrategies: List) { + action: String, + priorStrategies: List, + originalStrategy: RolloutStrategy, + newStrategy: RolloutStrategy?) { val singleNotUpdated = SingleFeatureValueUpdate(hasChanged = false, updated = false, previous = false) try { @@ -113,7 +129,7 @@ class InternalFeatureSqlApi @Inject constructor(private val convertUtils: Conver MultiFeatureValueUpdate(), MultiFeatureValueUpdate( true, - mutableListOf(RolloutStrategyUpdate(type = action, old = toRolloutStrategy(ue))), + mutableListOf(RolloutStrategyUpdate(type = action, old = originalStrategy, new = newStrategy)), mutableListOf(), priorStrategies ), @@ -129,12 +145,14 @@ class InternalFeatureSqlApi @Inject constructor(private val convertUtils: Conver // this needs to remove the connection, create an audit trail, and publish a new record to Edge, and trigger webhooks override fun detachApplicationStrategy( strategyForFeatureValue: DbStrategyForFeatureValue, - strategy: DbApplicationRolloutStrategy, + originalStrategy: RolloutStrategy, personWhoArchived: DbPerson ) { // this removes the strategy and forces the feature to update & create a new historical record QDbFeatureValue().id.eq(strategyForFeatureValue.featureValue.id).findOne()?.let { fv -> - val priorStrategies = fv.sharedRolloutStrategies.map { toRolloutStrategy(it) } + val priorStrategies = fv.sharedRolloutStrategies.map { InternalFeatureApi.toRolloutStrategy(it) } + // hold onto it because the object will be deleted + val originalValue = strategyForFeatureValue.value if (fv.sharedRolloutStrategies.removeIf { it.id == strategyForFeatureValue.id }) { fv.whoUpdated = personWhoArchived @@ -142,7 +160,9 @@ class InternalFeatureSqlApi @Inject constructor(private val convertUtils: Conver // this will cause an audit webhook automatically cacheSource.publishFeatureChange(fv) - publishFeatureMessage(fv, strategyForFeatureValue, "deleted", priorStrategies) + publishFeatureMessage(fv, "deleted", + priorStrategies, + originalStrategy.value(originalValue), null) } } } @@ -338,7 +358,7 @@ class FeatureSqlApi @Inject constructor( val applicationStrategyUpdates = MultiFeatureValueUpdate() applicationStrategyUpdates.hasChanged = existing.sharedRolloutStrategies.isNotEmpty() existing.sharedRolloutStrategies.forEach { rsi -> - applicationStrategyUpdates.updated.add(RolloutStrategyUpdate(type = "added", new = internalFeatureApi.toRolloutStrategy(rsi))) + applicationStrategyUpdates.updated.add(RolloutStrategyUpdate(type = "added", new = InternalFeatureApi.toRolloutStrategy(rsi))) } val retiredUpdate = SingleFeatureValueUpdate(hasChanged = featureValue.retired, updated = featureValue.retired, previous = false) @@ -603,7 +623,7 @@ class FeatureSqlApi @Inject constructor( changed = true if (todel) { addToStrategyUpdates(type = "deleted", - oldStrategy = internalFeatureApi.toRolloutStrategy(strategy), + oldStrategy = InternalFeatureApi.toRolloutStrategy(strategy), strategyUpdates = strategyUpdates) } todel @@ -627,7 +647,7 @@ class FeatureSqlApi @Inject constructor( log.debug("trying to reorder strategies and no change value permission") throw FeatureApi.NoAppropriateRole() } - addToStrategyReorders(strategyUpdates, desiredList.map { internalFeatureApi.toRolloutStrategy(it) }.toMutableList(), + addToStrategyReorders(strategyUpdates, desiredList.map { InternalFeatureApi.toRolloutStrategy(it) }.toMutableList(), historical.map { sharedStrategyToRolloutStrategyForReporting(it, foundApplicationStrategies) }) changed = true diff --git a/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/ApplicationStrategiesSpec.groovy b/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/ApplicationStrategiesSpec.groovy index 6eee2a5fd..d49734665 100644 --- a/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/ApplicationStrategiesSpec.groovy +++ b/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/ApplicationStrategiesSpec.groovy @@ -1,13 +1,13 @@ package io.featurehub.db.services -import io.featurehub.dacha.model.PublishAction + import io.featurehub.db.api.Opts import io.featurehub.db.api.PersonFeaturePermission +import io.featurehub.db.messaging.FeatureMessagingParameter +import io.featurehub.db.messaging.FeatureMessagingPublisher import io.featurehub.db.model.DbApplicationRolloutStrategy import io.featurehub.db.model.DbFeatureValue -import io.featurehub.db.model.DbPerson import io.featurehub.db.model.DbStrategyForFeatureValue -import io.featurehub.db.model.RoleType import io.featurehub.db.publish.CacheSourceFeatureGroupApi import io.featurehub.mr.events.common.CacheSource import io.featurehub.mr.model.ApplicationRoleType @@ -16,6 +16,7 @@ import io.featurehub.mr.model.CreateApplicationRolloutStrategy import io.featurehub.mr.model.CreateFeature import io.featurehub.mr.model.FeatureValue import io.featurehub.mr.model.FeatureValueType +import io.featurehub.mr.model.RolloutStrategy import io.featurehub.mr.model.RolloutStrategyInstance import io.featurehub.mr.model.UpdateApplicationRolloutStrategy @@ -26,7 +27,7 @@ class ApplicationStrategiesSpec extends Base3Spec { cacheSource = Mock(CacheSource) archiveStrategy = Mock(ArchiveStrategy) internalFeatureApi = Mock(InternalFeatureApi) - applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, archiveStrategy, internalFeatureApi) + applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, internalFeatureApi) featureSqlApi = new FeatureSqlApi(convertUtils, cacheSource, rsValidator, featureMessagingCloudEventPublisher, Mock(CacheSourceFeatureGroupApi)) } @@ -65,13 +66,12 @@ class ApplicationStrategiesSpec extends Base3Spec { def "full lifecycle of application strategies"() { given: "i have a feature for the application" String key = createFeature() - and: "full permissions" - def perms = allPermissions() and: "i have an application strategy" def strategy = createStrategy() + def updateName = ranName() when: "i associate the strategy with the feature value in the default environment" - def fv = associateStrategyWithFeatureValue(key, strategy) + associateStrategyWithFeatureValue(key, strategy) then: with(cacheSource) { 1 * publishFeatureChange( { DbFeatureValue v -> @@ -81,10 +81,16 @@ class ApplicationStrategiesSpec extends Base3Spec { 0 * _ when: "i update the application strategy" applicationRolloutStrategySqlApi.updateStrategy(app1.id, strategy.id, - new UpdateApplicationRolloutStrategy().name(updateName), superPerson.id.id, Opts.empty()) + new UpdateApplicationRolloutStrategy().percentage(20).name(updateName), superPerson.id.id, Opts.empty()) then: + 1 * 1 * internalFeatureApi.updatedApplicationStrategy({ DbStrategyForFeatureValue fvStrategy -> fvStrategy.rolloutStrategy.strategy.name == updateName + fvStrategy.rolloutStrategy.strategy.percentage == 20 + }, { RolloutStrategy original -> + original != null + original.percentage == null + original.name == strategy.name }, _) } @@ -97,7 +103,7 @@ class ApplicationStrategiesSpec extends Base3Spec { keys.each { associateStrategyWithFeatureValue(it, strategy) } and: "we reset the mock" internalFeatureApi = Mock(InternalFeatureApi) - applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, archiveStrategy, internalFeatureApi) + applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, internalFeatureApi) when: "we update the strategy name" applicationRolloutStrategySqlApi.updateStrategy(app1.id, strategy.id, new UpdateApplicationRolloutStrategy().name(ranName()), superuser, Opts.empty()) then: "there is no interaction with the features as nothing changed that was important" @@ -108,18 +114,54 @@ class ApplicationStrategiesSpec extends Base3Spec { then: 2 * internalFeatureApi.updatedApplicationStrategy({ DbStrategyForFeatureValue fvStrategy -> keys.contains(fvStrategy.featureValue.feature.key) - }, _) + }, { RolloutStrategy rs -> rs.percentage == null }, _) when: applicationRolloutStrategySqlApi.archiveStrategy(app1.id, strategy.id, superuser) - then: + then: // if we do this in separate then:'s then we can recognize the different calls the same method 1 * internalFeatureApi.detachApplicationStrategy({ DbStrategyForFeatureValue fv -> fv.featureValue.feature.key == keys.first() - }, { DbApplicationRolloutStrategy s -> - s.name != strategy.name - }, _) + }, { RolloutStrategy rs -> rs.percentage == 1000 }, _) then: 1 * internalFeatureApi.detachApplicationStrategy({ DbStrategyForFeatureValue fv -> fv.featureValue.feature.key == keys.last() }, _, _) } + + def "when we use the real internal feature service and update and delete strategies"() { + given: "we create two features" + def keys = [createFeature()] + and: "we create one strategy" + def strategy = createStrategy() + and: "we associate the strategy with both features" + keys.each { associateStrategyWithFeatureValue(it, strategy) } + and: "we ensure we are not using a mock internal feature service" + FeatureMessagingPublisher fmp = Mock() + internalFeatureApi = new InternalFeatureSqlApi(convertUtils, cacheSource, fmp) + applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, internalFeatureApi) + when: "we update the strategy percentage" + applicationRolloutStrategySqlApi.updateStrategy(app1.id, strategy.id, + new UpdateApplicationRolloutStrategy().percentage(1000), superuser, Opts.empty()) + then: + 1 * cacheSource.publishFeatureChange({ DbFeatureValue fv -> + fv.feature.key == keys.first() + fv.version == 3 + }) + 1 * fmp.publish({ FeatureMessagingParameter p -> + p.applicationStrategyUpdates.hasChanged + p.applicationStrategyUpdates.updated[0].new.percentage == 1000 + p.applicationStrategyUpdates.updated[0].old.percentage == null + }, _) + when: "we delete the application strategy" + applicationRolloutStrategySqlApi.archiveStrategy(app1.id, strategy.id, superuser) + then: + 1 * cacheSource.publishFeatureChange({ DbFeatureValue fv -> + fv.feature.key == keys.first() + fv.version == 4 + }) + 1 * fmp.publish({ FeatureMessagingParameter p -> + p.applicationStrategyUpdates.hasChanged + p.applicationStrategyUpdates.updated[0].new == null + p.applicationStrategyUpdates.updated[0].old.percentage == 1000 + }, _) + } } diff --git a/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/ApplicationStrategySpec.groovy b/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/ApplicationStrategySpec.groovy index 77b00c758..55316c97d 100644 --- a/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/ApplicationStrategySpec.groovy +++ b/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/ApplicationStrategySpec.groovy @@ -13,7 +13,7 @@ class ApplicationStrategySpec extends Base3Spec { ApplicationRolloutStrategySqlApi appStrategyApi def setup() { - appStrategyApi = new ApplicationRolloutStrategySqlApi(convertUtils, archiveStrategy, internalFeatureApi) + appStrategyApi = new ApplicationRolloutStrategySqlApi(convertUtils, internalFeatureApi) } def "i can create, update and delete an application strategy"() { diff --git a/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/FeatureAuditingApplicationStrategiesSpec.groovy b/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/FeatureAuditingApplicationStrategiesSpec.groovy index 18b978534..96c886e73 100644 --- a/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/FeatureAuditingApplicationStrategiesSpec.groovy +++ b/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/FeatureAuditingApplicationStrategiesSpec.groovy @@ -40,7 +40,7 @@ class FeatureAuditingApplicationStrategiesSpec extends Base3Spec { DbFeatureValueVersionKey histId def setup() { - applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, archiveStrategy, internalFeatureApi) + applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, internalFeatureApi) dbEnvironment = findEnvironment(env1.id) dbApplication = findApplication(app1.id)