Fix JSON and XML PatchBodyWriter errors output

When an error occurs on commit the result displays a global error
together with per-operation status, which is confusing and self
contradicting.

JsonPatchBodyWriter and XmlPatchBodyWriter edited to omit
per-operation status if global error is present.

Added tests for this behavior.

JIRA: NETCONF-1176
Change-Id: I228754e75189e8475fbd32fa0cb57272b35f654a
Signed-off-by: Matej Sramcik <matej.sramcik@pantheon.tech>

diff --git a/restconf/restconf-nb/src/main/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/JsonPatchStatusBodyWriter.java b/restconf/restconf-nb/src/main/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/JsonPatchStatusBodyWriter.java
index 713cc61115ee7bf833025401e4c1357a276d89b8..49a2f14be011dc2871a68da6a39100b052ffd88c 100644 (file)
--- a/restconf/restconf-nb/src/main/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/JsonPatchStatusBodyWriter.java
+++ b/restconf/restconf-nb/src/main/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/JsonPatchStatusBodyWriter.java
@@ -39,29 +39,29 @@ public class JsonPatchStatusBodyWriter extends AbstractPatchStatusBodyWriter {
 
         if (patchStatusContext.ok()) {
             reportSuccess(jsonWriter);
-            jsonWriter.endObject().endObject().flush();
-        }
-
-        final var modelContext = patchStatusContext.context();
-        final var globalErrors = patchStatusContext.globalErrors();
-        if (globalErrors != null) {
-            reportErrors(modelContext, globalErrors, jsonWriter);
-        }
+        } else {
+            final var modelContext = patchStatusContext.context();
+            final var globalErrors = patchStatusContext.globalErrors();
+            if (globalErrors != null) {
+                reportErrors(modelContext, globalErrors, jsonWriter);
+            } else {
+                jsonWriter.name("edit-status").beginObject()
+                    .name("edit").beginArray();
+                for (var editStatus : patchStatusContext.editCollection()) {
+                    jsonWriter.beginObject().name("edit-id").value(editStatus.getEditId());
 
-        jsonWriter.name("edit-status").beginObject()
-        .name("edit").beginArray();
-        for (var editStatus : patchStatusContext.editCollection()) {
-            jsonWriter.beginObject().name("edit-id").value(editStatus.getEditId());
-
-            final var editErrors = editStatus.getEditErrors();
-            if (editErrors != null) {
-                reportErrors(modelContext, editErrors, jsonWriter);
-            } else if (editStatus.isOk()) {
-                reportSuccess(jsonWriter);
+                    final var editErrors = editStatus.getEditErrors();
+                    if (editErrors != null) {
+                        reportErrors(modelContext, editErrors, jsonWriter);
+                    } else if (editStatus.isOk()) {
+                        reportSuccess(jsonWriter);
+                    }
+                    jsonWriter.endObject();
+                }
+                jsonWriter.endArray().endObject();
             }
-            jsonWriter.endObject();
         }
-        jsonWriter.endArray().endObject().endObject().endObject().flush();
+        jsonWriter.endObject().endObject().flush();
     }
 
     private static void reportSuccess(final JsonWriter jsonWriter) throws IOException {

diff --git a/restconf/restconf-nb/src/main/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/XmlPatchStatusBodyWriter.java b/restconf/restconf-nb/src/main/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/XmlPatchStatusBodyWriter.java
index 33a1187550b99549d6d426da1b75ca3e4f5095b5..2cf01f1d5adc24701b5ee7a240d4009f70c6b0d0 100644 (file)
--- a/restconf/restconf-nb/src/main/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/XmlPatchStatusBodyWriter.java
+++ b/restconf/restconf-nb/src/main/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/XmlPatchStatusBodyWriter.java
@@ -65,27 +65,26 @@ public class XmlPatchStatusBodyWriter extends AbstractPatchStatusBodyWriter {
             final var globalErrors = context.globalErrors();
             if (globalErrors != null) {
                 reportErrors(context.context(), globalErrors, writer);
-            }
-            writer.writeStartElement("edit-status");
-            for (var patchStatusEntity : context.editCollection()) {
-                writer.writeStartElement("edit");
-                writer.writeStartElement("edit-id");
-                writer.writeCharacters(patchStatusEntity.getEditId());
-                writer.writeEndElement();
-
-                final var editErrors = patchStatusEntity.getEditErrors();
-                if (editErrors != null) {
-                    reportErrors(context.context(),editErrors, writer);
-                } else if (patchStatusEntity.isOk()) {
-                    writer.writeEmptyElement("ok");
+            } else {
+                writer.writeStartElement("edit-status");
+                for (var patchStatusEntity : context.editCollection()) {
+                    writer.writeStartElement("edit");
+                    writer.writeStartElement("edit-id");
+                    writer.writeCharacters(patchStatusEntity.getEditId());
+                    writer.writeEndElement();
+
+                    final var editErrors = patchStatusEntity.getEditErrors();
+                    if (editErrors != null) {
+                        reportErrors(context.context(), editErrors, writer);
+                    } else if (patchStatusEntity.isOk()) {
+                        writer.writeEmptyElement("ok");
+                    }
+                    writer.writeEndElement();
                 }
                 writer.writeEndElement();
             }
-            writer.writeEndElement();
-
         }
         writer.writeEndElement();
-
         writer.flush();
     }
 

diff --git a/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/JsonPatchStatusBodyWriterTest.java b/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/JsonPatchStatusBodyWriterTest.java
new file mode 100644 (file)
index 0000000..fcb9ba0
--- /dev/null
+++ b/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/JsonPatchStatusBodyWriterTest.java
@@ -0,0 +1,75 @@
+/*
+ * Copyright (c) 2023 PANTHEON.tech s.r.o. All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.restconf.nb.rfc8040.jersey.providers;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.List;
+import javax.ws.rs.core.MediaType;
+import org.junit.Test;
+import org.opendaylight.restconf.common.errors.RestconfError;
+import org.opendaylight.restconf.common.patch.PatchStatusContext;
+import org.opendaylight.restconf.common.patch.PatchStatusEntity;
+import org.opendaylight.yangtools.yang.common.ErrorTag;
+import org.opendaylight.yangtools.yang.common.ErrorType;
+import org.opendaylight.yangtools.yang.model.api.EffectiveModelContext;
+
+public class JsonPatchStatusBodyWriterTest {
+    private final RestconfError error = new RestconfError(ErrorType.PROTOCOL, new ErrorTag("data-exists"),
+        "Data already exists");
+    private final PatchStatusEntity statusEntity = new PatchStatusEntity("patch1", true, null);
+    private final PatchStatusEntity statusEntityError = new PatchStatusEntity("patch1", false, List.of(error));
+    private final EffectiveModelContext context = mock(EffectiveModelContext.class);
+    private final JsonPatchStatusBodyWriter writer = new JsonPatchStatusBodyWriter();
+
+    /**
+     * Test if per-operation status is omitted if global error is present.
+     */
+    @Test
+    public void testOutputWithGlobalError() throws IOException {
+        final var outputStream = new ByteArrayOutputStream();
+        final var patchStatusContext = new PatchStatusContext(context,"patch", List.of(statusEntity),
+            false, List.of(error));
+        writer.writeTo(patchStatusContext, null, null, null, MediaType.APPLICATION_XML_TYPE, null, outputStream);
+
+        assertEquals("""
+            {"ietf-yang-patch:yang-patch-status":{\
+            "patch-id":"patch",\
+            "errors":{"error":[{\
+            "error-type":"protocol",\
+            "error-tag":"data-exists",\
+            "error-message":"Data already exists"\
+            }]}}}""", outputStream.toString(StandardCharsets.UTF_8));
+    }
+
+    /**
+     * Test if per-operation status is present if there is no global error present.
+     */
+    @Test
+    public void testOutputWithoutGlobalError() throws IOException {
+        final var outputStream = new ByteArrayOutputStream();
+        final var patchStatusContext = new PatchStatusContext(context, "patch", List.of(statusEntityError),
+            false, null);
+        writer.writeTo(patchStatusContext, null, null, null, MediaType.APPLICATION_XML_TYPE, null, outputStream);
+
+        assertEquals("""
+            {"ietf-yang-patch:yang-patch-status":{\
+            "patch-id":"patch",\
+            "edit-status":{"edit":[{\
+            "edit-id":"patch1",\
+            "errors":{"error":[{\
+            "error-type":"protocol",\
+            "error-tag":"data-exists",\
+            "error-message":"Data already exists"\
+            }]}}]}}}""", outputStream.toString(StandardCharsets.UTF_8));
+    }
+}

diff --git a/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/XmlPatchStatusBodyWriterTest.java b/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/XmlPatchStatusBodyWriterTest.java
new file mode 100644 (file)
index 0000000..f196f4d
--- /dev/null
+++ b/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/jersey/providers/XmlPatchStatusBodyWriterTest.java
@@ -0,0 +1,76 @@
+/*
+ * Copyright (c) 2023 PANTHEON.tech s.r.o. All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.restconf.nb.rfc8040.jersey.providers;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.List;
+import javax.ws.rs.core.MediaType;
+import org.junit.Test;
+import org.opendaylight.restconf.common.errors.RestconfError;
+import org.opendaylight.restconf.common.patch.PatchStatusContext;
+import org.opendaylight.restconf.common.patch.PatchStatusEntity;
+import org.opendaylight.yangtools.yang.common.ErrorTag;
+import org.opendaylight.yangtools.yang.common.ErrorType;
+import org.opendaylight.yangtools.yang.model.api.EffectiveModelContext;
+
+public class XmlPatchStatusBodyWriterTest {
+    private final RestconfError error = new RestconfError(ErrorType.PROTOCOL, new ErrorTag("data-exists"),
+        "Data already exists");
+    private final PatchStatusEntity statusEntity = new PatchStatusEntity("patch1", true, null);
+    private final PatchStatusEntity statusEntityError = new PatchStatusEntity("patch1", false, List.of(error));
+    private final EffectiveModelContext context = mock(EffectiveModelContext.class);
+    private final XmlPatchStatusBodyWriter writer = new XmlPatchStatusBodyWriter();
+
+    /**
+     * Test if per-operation status is omitted if global error is present.
+     */
+    @Test
+    public void testOutputWithGlobalError() throws IOException {
+        final var outputStream = new ByteArrayOutputStream();
+        final var patchStatusContext = new PatchStatusContext(context, "patch", List.of(statusEntity),
+            false, List.of(error));
+        writer.writeTo(patchStatusContext, null, null, null, MediaType.APPLICATION_XML_TYPE, null, outputStream);
+
+        assertEquals("""
+            <yang-patch-status xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-patch">\
+            <patch-id>patch</patch-id>\
+            <errors>\
+            <error-type>protocol</error-type>\
+            <error-tag>data-exists</error-tag>\
+            <error-message>Data already exists</error-message>\
+            </errors></yang-patch-status>""", outputStream.toString(StandardCharsets.UTF_8));
+
+    }
+
+    /**
+     * Test if per-operation status is present if there is no global error present.
+     */
+    @Test
+    public void testOutputWithoutGlobalError() throws IOException {
+        final var outputStream = new ByteArrayOutputStream();
+        final var patchStatusContext = new PatchStatusContext(context,"patch", List.of(statusEntityError),
+            false, null);
+        writer.writeTo(patchStatusContext, null, null, null, MediaType.APPLICATION_XML_TYPE, null, outputStream);
+        assertEquals("""
+            <yang-patch-status xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-patch">\
+            <patch-id>patch</patch-id>\
+            <edit-status><edit>\
+            <edit-id>patch1</edit-id>\
+            <errors>\
+            <error-type>protocol</error-type>\
+            <error-tag>data-exists</error-tag>\
+            <error-message>Data already exists</error-message>\
+            </errors></edit></edit-status>\
+            </yang-patch-status>""", outputStream.toString(StandardCharsets.UTF_8));
+    }
+}