XWIKI-21417: BaseObject#set always set the metadata dirty flag of the owner doc to true (#4547)

  * Only create the property when it doesn't exist in BaseObject#set
  * Provide unit test

(cherry picked from commit 71ad5fa)

Author: surli

xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/objects/BaseObject.java

@@ -466,18 +466,26 @@ public void set(String fieldname, java.lang.Object value, XWikiContext context)
         BaseClass bclass = getXClass(context);
         PropertyClass pclass = (PropertyClass) bclass.get(fieldname);
         BaseProperty prop = (BaseProperty) safeget(fieldname);
+        boolean createProp = false;
         if ((value instanceof String) && (pclass != null)) {
-            prop = pclass.fromString((String) value);
+            BaseProperty newProp = pclass.fromString((String) value);
+            if (prop == null) {
+                prop = newProp;
+                createProp = true;
+            } else {
+                prop.setValue(newProp.getValue());
+            }
         } else {
             if ((prop == null) && (pclass != null)) {
                 prop = pclass.newProperty();
+                createProp = true;
             }
             if (prop != null) {
                 prop.setValue(value);
             }
         }
 
-        if (prop != null) {
+        if (prop != null && createProp) {
             prop.setOwnerDocument(getOwnerDocument());
             safeput(fieldname, prop);
         }

xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/objects/BaseObjectTest.java

@@ -31,20 +31,29 @@
 import org.xwiki.store.merge.MergeManagerResult;
 import org.xwiki.test.junit5.mockito.MockComponent;
 
+import com.xpn.xwiki.XWikiContext;
+import com.xpn.xwiki.XWikiException;
 import com.xpn.xwiki.doc.XWikiDocument;
 import com.xpn.xwiki.doc.merge.MergeConfiguration;
 import com.xpn.xwiki.doc.merge.MergeResult;
+import com.xpn.xwiki.objects.classes.BaseClass;
+import com.xpn.xwiki.objects.classes.PropertyClass;
 import com.xpn.xwiki.test.MockitoOldcore;
 import com.xpn.xwiki.test.junit5.mockito.InjectMockitoOldcore;
 import com.xpn.xwiki.test.junit5.mockito.OldcoreTest;
 import com.xpn.xwiki.test.reference.ReferenceComponentList;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNotSame;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 /**
@@ -224,4 +233,106 @@ void cloneWithoutDetach()
         assertNotSame(object.getOwnerDocument(), clonedObject.getOwnerDocument());
         assertNotNull(clonedObject.getOwnerDocument());
     }
+
+    @Test
+    void setString() throws XWikiException
+    {
+        DocumentReference documentReference = new DocumentReference("wiki", "space", "document");
+        DocumentReference classReference = new DocumentReference("wiki", "space", "class");
+        XWikiDocument classDocument = new XWikiDocument(classReference);
+        XWikiDocument ownerDocument = new XWikiDocument(new DocumentReference("wiki", "space", "page"));
+        BaseObject object = new BaseObject();
+        object.setDocumentReference(documentReference);
+        object.setXClassReference(classReference);
+        object.setOwnerDocument(ownerDocument);
+
+        XWikiContext context = this.oldcore.getXWikiContext();;
+        String fieldName = "myField";
+        String value = "myValue";
+
+        BaseClass baseClass = mock(BaseClass.class);
+        classDocument.setXClass(baseClass);
+        when(this.oldcore.getSpyXWiki().getDocument(classReference, context)).thenReturn(classDocument);
+
+        PropertyClass propertyClass = mock(PropertyClass.class);
+        when(baseClass.get(fieldName)).thenReturn(propertyClass);
+
+        BaseProperty newProp = mock(BaseProperty.class);
+        when(propertyClass.fromString(value)).thenReturn(newProp);
+
+        object.set(fieldName, value, context);
+        assertTrue(object.isDirty());
+        assertTrue(ownerDocument.isMetaDataDirty());
+        verify(newProp, times(2)).setOwnerDocument(ownerDocument);
+        verify(newProp).setName(fieldName);
+        verify(newProp).setObject(object);
+        assertTrue(object.getFieldList().contains(newProp));
+
+        ownerDocument.setMetaDataDirty(false);
+        object.setDirty(false);
+
+        // Now the property exists let's call it again
+        BaseProperty newProp2 = mock(BaseProperty.class);
+        when(propertyClass.fromString(value)).thenReturn(newProp2);
+        when(newProp2.getValue()).thenReturn(value);
+        object.set(fieldName, value, context);
+        verify(newProp).setValue(value);
+        assertFalse(object.isDirty());
+        assertFalse(ownerDocument.isMetaDataDirty());
+
+        // no new calls
+        verify(newProp, times(2)).setOwnerDocument(ownerDocument);
+        verify(newProp).setName(fieldName);
+        verify(newProp).setObject(object);
+    }
+
+    @Test
+    void setObject() throws XWikiException
+    {
+        DocumentReference documentReference = new DocumentReference("wiki", "space", "document");
+        DocumentReference classReference = new DocumentReference("wiki", "space", "class");
+        XWikiDocument classDocument = new XWikiDocument(classReference);
+        XWikiDocument ownerDocument = new XWikiDocument(new DocumentReference("wiki", "space", "page"));
+        BaseObject object = new BaseObject();
+        object.setDocumentReference(documentReference);
+        object.setXClassReference(classReference);
+        object.setOwnerDocument(ownerDocument);
+
+        XWikiContext context = this.oldcore.getXWikiContext();;
+        String fieldName = "myField";
+        Object value = 4545;
+
+        BaseClass baseClass = mock(BaseClass.class);
+        classDocument.setXClass(baseClass);
+        when(this.oldcore.getSpyXWiki().getDocument(classReference, context)).thenReturn(classDocument);
+
+        PropertyClass propertyClass = mock(PropertyClass.class);
+        when(baseClass.get(fieldName)).thenReturn(propertyClass);
+
+        BaseProperty newProp = mock(BaseProperty.class);
+        when(propertyClass.newProperty()).thenReturn(newProp);
+
+        object.set(fieldName, value, context);
+        assertTrue(object.isDirty());
+        assertTrue(ownerDocument.isMetaDataDirty());
+        verify(newProp).setValue(value);
+        verify(newProp, times(2)).setOwnerDocument(ownerDocument);
+        verify(newProp).setName(fieldName);
+        verify(newProp).setObject(object);
+        assertTrue(object.getFieldList().contains(newProp));
+
+        ownerDocument.setMetaDataDirty(false);
+        object.setDirty(false);
+
+        // Now the property exists let's call it again
+        object.set(fieldName, value, context);
+        verify(newProp, times(2)).setValue(value);
+        assertFalse(object.isDirty());
+        assertFalse(ownerDocument.isMetaDataDirty());
+
+        // no new calls
+        verify(newProp, times(2)).setOwnerDocument(ownerDocument);
+        verify(newProp).setName(fieldName);
+        verify(newProp).setObject(object);
+    }
 }