From b19c01daefcc8ce40379f1f798d0305afdbb276f Mon Sep 17 00:00:00 2001
From: Thomas Gelf <thomas@gelf.net>
Date: Mon, 5 Dec 2022 19:19:41 +0100
Subject: [PATCH] Sync: ignore null for policy = ignore

fixes #2657
---
 doc/82-Changelog.md              |  1 +
 library/Director/Import/Sync.php | 52 ++++++++++++++++++++------------
 2 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/doc/82-Changelog.md b/doc/82-Changelog.md
index 0d599eca..44556f99 100644
--- a/doc/82-Changelog.md
+++ b/doc/82-Changelog.md
@@ -32,6 +32,7 @@ This version hasn't been released yet
 * FEATURE: property modifiers can now be applied based on filters (#2756)
 * FEATURE: CIDR notation (network ranges) is supported in such filters (#2757)
 * FIX: synchronizing Service (and -Set) Templates has been fixed (#2745, #2217)
+* FIX: null properties with Sync policy "ignore" are now being ignored (#2657)
 
 ### REST API
 * FIX: Commands give 304 w/o ghost changes for same properties (#2660)
diff --git a/library/Director/Import/Sync.php b/library/Director/Import/Sync.php
index 54368e08..523dfbc8 100644
--- a/library/Director/Import/Sync.php
+++ b/library/Director/Import/Sync.php
@@ -49,6 +49,9 @@ class Sync
     /** @var array<mixed, array<int, string>> key => [property, property]*/
     protected $setNull = [];
 
+    /** @var array<mixed, array<string, mixed>> key => [propertyName, newValue]*/
+    protected $newProperties = [];
+
     /** @var bool Whether we already prepared your sync */
     protected $isPrepared = false;
 
@@ -535,6 +538,15 @@ class Sync
      */
     protected function prepareNewObject($row, DbObject $object, $objectKey, $sourceId)
     {
+        if (!isset($this->newProperties[$objectKey])) {
+            $this->newProperties[$objectKey] = [];
+        }
+        // TODO: some more improvements are possible here. First, no need to instantiate
+        //       all new objects, we could stick with the newProperties array. Next, we
+        //       should be more correct when respecting sync property order. Right now,
+        //       a property from another Import Source might win, even if property order
+        //       tells something different. This is a very rare case, but still incorrect.
+        $properties = &$this->newProperties[$objectKey];
         foreach ($this->syncProperties as $propertyKey => $p) {
             if ($p->get('source_id') !== $sourceId) {
                 continue;
@@ -565,32 +577,30 @@ class Sync
                             (array) $val
                         );
                     } else {
-                        if ($val === null) {
-                            $this->setNull[$objectKey][$prop] = $prop;
-                        } else {
-                            unset($this->setNull[$objectKey][$prop]);
-                            $object->vars()->$varName = $val;
-                        }
+                        $this->setPropertyWithNullLogic($object, $objectKey, $prop, $val, $properties);
                     }
                 } else {
-                    if ($val === null) {
-                        $this->setNull[$objectKey][$prop] = $prop;
-                    } else {
-                        unset($this->setNull[$objectKey][$prop]);
-                        $object->set($prop, $val);
-                    }
+                    $this->setPropertyWithNullLogic($object, $objectKey, $prop, $val, $properties);
                 }
             } else {
-                if ($val === null) {
-                    $this->setNull[$objectKey][$prop] = $prop;
-                } else {
-                    unset($this->setNull[$objectKey][$prop]);
-                    $object->set($prop, $val);
-                }
+                $this->setPropertyWithNullLogic($object, $objectKey, $prop, $val, $properties);
             }
         }
     }
 
+    protected function setPropertyWithNullLogic(DbObject $object, $objectKey, $property, $value, &$allProps)
+    {
+        if ($value === null) {
+            if (! array_key_exists($property, $allProps) || $allProps[$property] === null) {
+                $this->setNull[$objectKey][$property] = $property;
+            }
+        } else {
+            unset($this->setNull[$objectKey][$property]);
+            $object->set($property, $value);
+        }
+        $allProps[$property] = $value;
+    }
+
     /**
      * @return $this
      */
@@ -793,7 +803,11 @@ class Sync
             }
         }
 
-        if (isset($this->setNull[$key])) {
+        // Hint: in theory, NULL should be set on new objects, but this has no effect
+        //       anyway, and we also do not store vars.something = null, this would
+        //       instead delete the variable. So here we do not need to check for new
+        //       objects, and skip all null values with update policy = 'ignore'
+        if ($policy !== 'ignore' && isset($this->setNull[$key])) {
             foreach ($this->setNull[$key] as $property) {
                 $this->objects[$key]->set($property, null);
             }