Fixes #15590: Renaming/moving technique category fails in several way

webapp/sources/ldap-inventory/inventory-api/src/main/scala/com/normation/inventory/domain/MachineInventory.scala

@@ -158,9 +158,8 @@ case object BSDJail extends VmType("bsdjail") with HashcodeCaching
  * - virtual machines ;
  * - physical machines.
  */
-sealed abstract class MachineType {
-  def toString : String
-}
+sealed abstract class MachineType
+
 case class VirtualMachineType(vm:VmType) extends MachineType with HashcodeCaching {
   override def toString() = vm.name
 }

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/cfclerk/services/TechniqueLibraryUpdateNotification.scala

@@ -39,7 +39,6 @@ package com.normation.cfclerk.services
 
 import com.normation.cfclerk.domain.TechniqueCategory
 import com.normation.cfclerk.domain.TechniqueCategoryId
-import com.normation.cfclerk.domain.TechniqueCategoryName
 import com.normation.cfclerk.domain.TechniqueName
 import com.normation.cfclerk.domain.TechniqueVersion
 import com.normation.eventlog.EventActor
@@ -51,7 +50,7 @@ object TechniqueCategoryModType {
   case class Updated(id: TechniqueCategory) extends TechniqueCategoryModType // change in name, description
   case class Added(cat: TechniqueCategory, parentId: TechniqueCategoryId) extends TechniqueCategoryModType
   case class Deleted(cat: TechniqueCategory) extends TechniqueCategoryModType
-  case class Moved(id: TechniqueCategoryName, oldParentId: TechniqueCategoryId, newParentId: TechniqueCategoryId) extends TechniqueCategoryModType
+  case class Moved(oldId: TechniqueCategoryId, newId: TechniqueCategoryId) extends TechniqueCategoryModType
 }
 
 sealed trait TechniqueVersionModType

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/cfclerk/services/TechniqueRepository.scala

@@ -107,12 +107,12 @@ trait TechniqueRepository {
    * If the policyName is unknown, the returned collection will
    * be empty.
    */
-  def getTechniqueVersions(name:TechniqueName) : SortedSet[TechniqueVersion]
+  def getTechniqueVersions(name: TechniqueName): SortedSet[TechniqueVersion]
 
   /**
    * Get the list of technique (with their version) for that name
    */
-  def getByName(name:TechniqueName) : Map[TechniqueVersion, Technique]
+  def getByName(name: TechniqueName): Map[TechniqueVersion, Technique]
 
   ////////////////// method for categories //////////////////
 
@@ -122,14 +122,17 @@ trait TechniqueRepository {
 
   def getParentTechniqueCategory_forTechnique(id: TechniqueId): Box[TechniqueCategory]
 
-  final def getTechniqueCategoriesBreadCrump(id: TechniqueId): Box[Seq[TechniqueCategory]] = {
+  final
+  def getTechniqueCategoriesBreadCrump(id: TechniqueId): Box[Seq[TechniqueCategory]] = {
     for {
       cat <- getParentTechniqueCategory_forTechnique(id)
       path <- sequence(cat.id.getIdPathFromRoot) { currentCatId =>
-        getTechniqueCategory(currentCatId) ?~! "'%s' category was not found but should be a parent of '%s'".format(currentCatId, cat.id)
+        getTechniqueCategory(currentCatId) ?~! s"'${currentCatId.toString}' category was not found but should be a parent of '${cat.id.toString}'"
       }
     } yield {
       path
     }
   }
+
+  def getAllCategories: Map[TechniqueCategoryId, TechniqueCategory]
 }

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/cfclerk/services/impl/TechniqueRepositoryImpl.scala

@@ -49,7 +49,7 @@ import com.normation.utils.StringUuidGenerator
 import com.normation.utils.Control
 
 class TechniqueRepositoryImpl(
-    techniqueReader: TechniqueReader
+  techniqueReader: TechniqueReader
   , refLibCallbacks: Seq[TechniquesLibraryUpdateNotification]
   , uuidGen        : StringUuidGenerator
 ) extends TechniqueRepository with UpdateTechniqueLibrary with Loggable {
@@ -95,6 +95,7 @@ class TechniqueRepositoryImpl(
   }
 
   override def update(modId: ModificationId, actor:EventActor, reason: Option[String]) : Box[Map[TechniqueName, TechniquesLibraryUpdateType]] = {
+    import TechniqueCategoryModType._
     try {
       val modifiedPackages = techniqueReader.getModifiedTechniques
       if (techniqueReader.needReload() || /* first time init */ null == techniqueInfosCache) {
@@ -114,14 +115,14 @@ class TechniqueRepositoryImpl(
 
         // check for changes in categories
         val updatedCategories: Set[TechniqueCategoryModType] = {
-          val updates = techniqueInfosCache.allCategories.flatMap { case (id, cat) =>
+          val updates: Iterable[TechniqueCategoryModType] = techniqueInfosCache.allCategories.flatMap { case (id, cat) =>
             // we only works on parents looking for their children for move/add/delete since id are not stable on move
             oldInfo.allCategories.get(id) match {
               case None      => // added or move (since id depends on parent), but it will be processed below, ignore
                 Nil
               case Some(old) =>
                 val m1 = if(old.name != cat.name || old.description != cat.description) {
-                  TechniqueCategoryModType.Updated(cat) :: Nil
+                  Updated(cat) :: Nil
                 } else Nil
 
                 val m2 = if(old.subCategoryIds != cat.subCategoryIds) {
@@ -131,18 +132,18 @@ class TechniqueRepositoryImpl(
                     // hypothesis: directory names for category are unique in technique lib
                     (oldInfo.allCategories.find(_._2.subCategoryIds.exists(_.name == changedId.name)), techniqueInfosCache.allCategories.find(_._2.subCategoryIds.exists(_.name == changedId.name))) match {
                       case (Some((oldId, _)), Some((newId, _))) =>
-                        if(oldId != newId) TechniqueCategoryModType.Moved(changedId.name, oldId, newId) :: Nil
+                        if(oldId != newId) Moved(changedId, newId) :: Nil
                         else Nil
 
                       case (None, Some((newId, _))) =>
                         //new cat should be in new info
                         techniqueInfosCache.allCategories.get(changedId).map(c =>
-                          TechniqueCategoryModType.Added(c, newId)
+                          Added(c, newId)
                         ).toList
                       case (Some(_), None) =>
                         // old cat should be in old info
                         oldInfo.allCategories.get(changedId).map(c =>
-                          TechniqueCategoryModType.Deleted(c)
+                          Deleted(c)
                         ).toList
                       case (None, None) =>
                         Nil
@@ -154,7 +155,23 @@ class TechniqueRepositoryImpl(
           }
 
           // we may have counted moved categories two time => Set
-          updates.toSet
+          val changed = updates.toSet
+          // now, we need to take care of the case in #15590. So we need to look for couple of deleted/added where
+          // {name, techniques, subcategories} are the same and transform them into move.
+          val deleted = changed.collect { case d: Deleted => d }
+          // for each delete, look for a corresponding add, and in that case mark them as to be removed from changed
+          val (moveToAdd, otherToRemove) = ((List.empty[Moved], List.empty[TechniqueCategoryModType]) /: deleted) { case ((move, toDelete), d@Deleted(currentDel)) =>
+            changed.find(c => c match {
+                // hypothesis: it's a directory rename if the display name and content is the same
+              case Added(cat, parentId) => currentDel.name == cat.name && currentDel.subCategoryIds == cat.subCategoryIds && currentDel.techniqueIds == cat.techniqueIds
+              case _ => false
+            })match {
+              case Some(a@Added(add, _)) => ((Moved(currentDel.id, add.id)::move, a :: d :: toDelete))
+              case _ => (move, toDelete)
+            }
+          }
+
+          changed -- otherToRemove ++ moveToAdd
         }
 
         val res = Control.bestEffort(callbacks) { callback =>
@@ -199,6 +216,10 @@ class TechniqueRepositoryImpl(
     }).toMap
   }
 
+  override def getAllCategories: Map[TechniqueCategoryId, TechniqueCategory] = {
+    techniqueInfosCache.allCategories
+  }
+
   override def getTechniquesInfo() = techniqueInfosCache
 
   override def getTechniqueVersions(name: TechniqueName): SortedSet[TechniqueVersion] = {

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/domain/RudderDit.scala

@@ -214,7 +214,7 @@ class RudderDit(val BASE_DN:DN) extends AbstractDit {
       , isEnabled:Boolean
       , isSystem : Boolean
     ) : LDAPEntry = {
-      val mod = LDAPEntry(new DN(new RDN(A_ACTIVE_TECHNIQUE_UUID,uuid),parentDN))
+      val mod = LDAPEntry(new DN(buildRDN(uuid),parentDN))
       mod +=! (A_OC, OC.objectClassNames(OC_ACTIVE_TECHNIQUE).toSeq:_*)
       mod +=! (A_TECHNIQUE_UUID, techniqueName.value)
       mod +=! (A_IS_ENABLED, isEnabled.toLDAPString)
@@ -223,6 +223,8 @@ class RudderDit(val BASE_DN:DN) extends AbstractDit {
       mod
     }
 
+    def buildRDN(rdn: String): RDN = new RDN(A_ACTIVE_TECHNIQUE_UUID, rdn)
+
     def directiveModel(uuid:String, techniqueVersion:TechniqueVersion, parentDN:DN) : LDAPEntry = {
       val mod = LDAPEntry(new DN(new RDN(A_DIRECTIVE_UUID,uuid),parentDN))
       mod +=! (A_TECHNIQUE_VERSION, techniqueVersion.toString)

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/DirectiveRepository.scala

@@ -416,6 +416,6 @@ trait WoDirectiveRepository {
    * Fail if the parent already contains a category of the
    * same name (name must be unique for a given level)
    */
-  def move(categoryId:ActiveTechniqueCategoryId, intoParent:ActiveTechniqueCategoryId, modificationId: ModificationId, actor: EventActor, reason: Option[String]) : Box[ActiveTechniqueCategoryId]
+  def move(categoryId:ActiveTechniqueCategoryId, intoParent:ActiveTechniqueCategoryId, optionNewName: Option[ActiveTechniqueCategoryId], modificationId: ModificationId, actor: EventActor, reason: Option[String]) : Box[ActiveTechniqueCategoryId]
 
 }

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPDirectiveRepository.scala

@@ -817,7 +817,7 @@ class WoLDAPDirectiveRepository(
    * Both category to move and destination have to exists, else it is a failure.
    * The destination category can not be a child of the category to move.
    */
-  def move(categoryId:ActiveTechniqueCategoryId, intoParent:ActiveTechniqueCategoryId, modId : ModificationId, actor: EventActor, reason: Option[String]) : Box[ActiveTechniqueCategoryId] = {
+  def move(categoryId:ActiveTechniqueCategoryId, intoParent:ActiveTechniqueCategoryId, optionNewName: Option[ActiveTechniqueCategoryId], modId : ModificationId, actor: EventActor, reason: Option[String]) : Box[ActiveTechniqueCategoryId] = {
       for {
         con            <- ldap
         oldParents     <- if(autoExportOnModify) {
@@ -837,12 +837,11 @@ class WoLDAPDirectiveRepository(
                               }
                             case _ => Failure("Can not find the category entry name for category with ID %s. Name is needed to check unicity of categories by level")
                           }
-        result         <- userLibMutex.writeLock { con.move(categoryEntry.dn, newParentEntry.dn) }
-        category       <- getActiveTechniqueCategory(categoryId)
-        autoArchive    <- (if(autoExportOnModify && !result.isInstanceOf[LDIFNoopChangeRecord] && !category.isSystem ) {
+        result         <- userLibMutex.writeLock { con.move(categoryEntry.dn, newParentEntry.dn, optionNewName.map(n => rudderDit.ACTIVE_TECHNIQUES_LIB.buildRDN(n.value))) }
+        newCat         <- getActiveTechniqueCategory(optionNewName.getOrElse(categoryId))
+        autoArchive    <- (if(autoExportOnModify && !result.isInstanceOf[LDIFNoopChangeRecord] && !newCat.isSystem ) {
                             for {
-                              newCat   <- getActiveTechniqueCategory(categoryId)
-                              parents  <- getParentsForActiveTechniqueCategory(categoryId)
+                              parents  <- getParentsForActiveTechniqueCategory(newCat.id)
                               commiter <- personIdentService.getPersonIdentOrDefault(actor.name)
                               moved    <- gitCatArchiver.moveActiveTechniqueCategory(newCat, oldParents.map( _.id), parents.map( _.id), Some((modId, commiter, reason)))
                             } yield {

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/services/policies/DeploymentService.scala

@@ -87,7 +87,6 @@ import com.normation.rudder.domain.logger.TimingDebugLogger
 import com.normation.rudder.domain.logger.PolicyLogger
 import cats.data.NonEmptyList
 import com.normation.rudder.domain.reports.OverridenPolicy
-import com.normation.rudder.hooks.HookEnvPair
 import com.normation.rudder.hooks.Hooks
 import com.normation.rudder.hooks.HooksLogger
 import monix.eval.Task

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/services/policies/TechniqueAcceptationDatetimeUpdater.scala

@@ -39,6 +39,7 @@ package com.normation.rudder.services.policies
 
 import com.normation.cfclerk.domain.RootTechniqueCategoryId
 import com.normation.cfclerk.domain.SubTechniqueCategoryId
+import com.normation.cfclerk.domain.TechniqueCategoryId
 import com.normation.cfclerk.domain.TechniqueName
 import com.normation.cfclerk.services._
 import com.normation.eventlog.ModificationId
@@ -137,6 +138,15 @@ class TechniqueAcceptationUpdater(
     }
 
     def handleCategoriesUpdate(mods: Set[TechniqueCategoryModType]): Box[Seq[Unit]] = {
+      // we use the same id for category and active category: the directory name,
+      // *safe* for root category: it's "/" for technique, and "Active Techniques" in LDAP
+      def toActiveCatId(id: TechniqueCategoryId): ActiveTechniqueCategoryId = {
+        id match {
+          case RootTechniqueCategoryId =>ActiveTechniqueCategoryId("Active Techniques")
+          case SubTechniqueCategoryId(name, parentId) => ActiveTechniqueCategoryId(name.value)
+        }
+      }
+
       // we need to sort change: first delete, then add, then move, then update
       val sorted = mods.toList.sortWith { (a, b) => (a, b) match {
         case (TechniqueCategoryModType.Deleted(_), _) => true
@@ -152,44 +162,52 @@ class TechniqueAcceptationUpdater(
 
       Control.bestEffort(sorted) { mod => mod match {
         case TechniqueCategoryModType.Deleted(cat) =>
-          logger.debug(s"Category '${cat.id.name.value}' deleted")
+          logger.debug(s"Category '${cat.id.name.value}' deleted in file system")
           if(cat.subCategoryIds.isEmpty && cat.techniqueIds.isEmpty) {
-            rwActiveTechniqueRepo.delete(ActiveTechniqueCategoryId(cat.id.name.value), modId, actor, reason).map(_ => ())
+            rwActiveTechniqueRepo.delete(toActiveCatId(cat.id), modId, actor, reason).map(_ => ())
           } else {
             logger.info(s"Not deleting non empty category: '${cat.id.toString}'")
             Full(())
           } // do nothing
         case TechniqueCategoryModType.Added(cat, parentId) =>
-          logger.debug(s"Category '${cat.id.name.value}' added into '${parentId.toString}'")
+          logger.debug(s"Category '${cat.id.toString}' added into '${parentId.toString}'")
           rwActiveTechniqueRepo.addActiveTechniqueCategory(
             ActiveTechniqueCategory(
-                ActiveTechniqueCategoryId(cat.id.name.value)
+                toActiveCatId(cat.id)
               , cat.name
               , cat.description
               , Nil
               , Nil
               , cat.isSystem
-            ), ActiveTechniqueCategoryId(parentId.name.value), modId, actor, reason
+            ), toActiveCatId(parentId), modId, actor, reason
           ).map(_ => ())
-        case TechniqueCategoryModType.Moved(name, from, to) =>
-          logger.debug(s"Category '${name.value}' moved from '${from.toString}' to '${to.name.toString}'")
-          rwActiveTechniqueRepo.move(ActiveTechniqueCategoryId(name.value), ActiveTechniqueCategoryId(to.name.value), modId, actor, reason).map(_ => ())
+        case TechniqueCategoryModType.Moved(from, to) =>
+          to match {
+            case RootTechniqueCategoryId =>
+              Failure(s"Category '${from.toString}' is trying to replace root categoy. This is likely a bug, please report it.")
+
+            case SubTechniqueCategoryId(name, parentId) =>
+              logger.debug(s"Category '${from.toString}' moved to '${to.toString}'")
+              // if rdn changed, we need to issue a modrdn
+              val newName = if(from.name == to.name) None else Some(ActiveTechniqueCategoryId(to.name.value))
+              rwActiveTechniqueRepo.move(ActiveTechniqueCategoryId(from.name.value), toActiveCatId(parentId), newName, modId, actor, reason).map(_ => ())
+          }
         case TechniqueCategoryModType.Updated(cat) =>
           logger.debug(s"Category '${cat.id.toString}' updated")
-          roActiveTechniqueRepo.getActiveTechniqueCategory(ActiveTechniqueCategoryId(cat.id.name.value)) match {
+          roActiveTechniqueRepo.getActiveTechniqueCategory(toActiveCatId(cat.id)) match {
             case Empty          =>
               cat.id match {
                 case _:RootTechniqueCategoryId.type => Full(())
                 case i:SubTechniqueCategoryId =>
                   rwActiveTechniqueRepo.addActiveTechniqueCategory(
                     ActiveTechniqueCategory(
-                        ActiveTechniqueCategoryId(cat.id.name.value)
+                        toActiveCatId(cat.id)
                       , cat.name
                       , cat.description
                       , Nil
                       , Nil
                       , cat.isSystem
-                    ), ActiveTechniqueCategoryId(i.parentId.name.value), modId, actor, reason
+                    ), toActiveCatId(i.parentId), modId, actor, reason
                   ).map(_ => ())
                 }
             case Full(existing) =>

webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/DummyTechniqueRepository.scala

@@ -97,4 +97,5 @@ class DummyTechniqueRepository(policies: Seq[Technique] = Seq()) extends Techniq
   override def getTechniqueCategory(id: TechniqueCategoryId): Box[TechniqueCategory] = null
   override def getParentTechniqueCategory_forTechnique(id: TechniqueId): Box[TechniqueCategory] = null
 
+  override def getAllCategories: Map[TechniqueCategoryId, TechniqueCategory] = ???
 }