From 5ebed0f75d5b73dad6c9ba520b677ccb1088a136 Mon Sep 17 00:00:00 2001
From: Michael Hoennig <michael@hoennig.de>
Date: Tue, 9 Aug 2022 18:30:06 +0200
Subject: [PATCH] extract OptionalFromJson for JSON mapper bug

---
 .../hsadminng/OptionalFromJson.java           | 29 +++++++++
 .../hs/hspackage/PackageController.java       |  8 +--
 .../hsadminng/OptionalFromJsonUnitTest.java   | 59 +++++++++++++++++++
 3 files changed, 90 insertions(+), 6 deletions(-)
 create mode 100644 src/main/java/net/hostsharing/hsadminng/OptionalFromJson.java
 create mode 100644 src/test/java/net/hostsharing/hsadminng/OptionalFromJsonUnitTest.java

diff --git a/src/main/java/net/hostsharing/hsadminng/OptionalFromJson.java b/src/main/java/net/hostsharing/hsadminng/OptionalFromJson.java
new file mode 100644
index 00000000..745e5f8a
--- /dev/null
+++ b/src/main/java/net/hostsharing/hsadminng/OptionalFromJson.java
@@ -0,0 +1,29 @@
+package net.hostsharing.hsadminng;
+
+import org.openapitools.jackson.nullable.JsonNullable;
+
+import java.util.function.Consumer;
+
+public class OptionalFromJson<T> {
+
+    private final JsonNullable<T> optionalNullableValueFromJson;
+
+    public OptionalFromJson(final JsonNullable<T> optionalNullableValueFromJson) {
+        this.optionalNullableValueFromJson = optionalNullableValueFromJson;
+    }
+
+    public static <T> OptionalFromJson<T> of(final JsonNullable<T> optionalNullableValueFromJson) {
+        return new OptionalFromJson<>(optionalNullableValueFromJson);
+    }
+
+    public void ifPresent(final Consumer<T> setter) {
+        // It looks like a bug to me, that the JsonNullable itself is null if the element was not in the JSON;
+        //  and if it is not null, isPresent() always returns true and thus ifPresent() always fires.
+        // Instead there should always be a JsonNullable instance
+        //  and ifPresent() should only fire if the element was actually in th JSON.
+        if (optionalNullableValueFromJson != null) {
+            // this will work with the bug as well as without
+            optionalNullableValueFromJson.ifPresent(setter);
+        }
+    }
+}
diff --git a/src/main/java/net/hostsharing/hsadminng/hs/hspackage/PackageController.java b/src/main/java/net/hostsharing/hsadminng/hs/hspackage/PackageController.java
index 527923a2..46587bb0 100644
--- a/src/main/java/net/hostsharing/hsadminng/hs/hspackage/PackageController.java
+++ b/src/main/java/net/hostsharing/hsadminng/hs/hspackage/PackageController.java
@@ -1,5 +1,6 @@
 package net.hostsharing.hsadminng.hs.hspackage;
 
+import net.hostsharing.hsadminng.OptionalFromJson;
 import net.hostsharing.hsadminng.context.Context;
 import net.hostsharing.hsadminng.generated.api.v1.api.PackagesApi;
 import net.hostsharing.hsadminng.generated.api.v1.model.PackageResource;
@@ -52,14 +53,9 @@ public class PackageController implements PackagesApi {
             context.assumeRoles(assumedRoles);
         }
         final var current = packageRepository.findByUuid(packageUuid);
-        if (body.getDescription() != null) {
-            body.getDescription().ifPresent(current::setDescription);
-        } else {
-            body.toString();
-        }
+        OptionalFromJson.of(body.getDescription()).ifPresent(current::setDescription);
         final var saved = packageRepository.save(current);
         final var mapped = map(saved, PackageResource.class);
         return ResponseEntity.ok(mapped);
     }
-
 }
diff --git a/src/test/java/net/hostsharing/hsadminng/OptionalFromJsonUnitTest.java b/src/test/java/net/hostsharing/hsadminng/OptionalFromJsonUnitTest.java
new file mode 100644
index 00000000..219ad4d4
--- /dev/null
+++ b/src/test/java/net/hostsharing/hsadminng/OptionalFromJsonUnitTest.java
@@ -0,0 +1,59 @@
+package net.hostsharing.hsadminng;
+
+import org.junit.jupiter.api.Test;
+import org.openapitools.jackson.nullable.JsonNullable;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class OptionalFromJsonUnitTest {
+
+    private String value = "unchanged initial value";
+
+    @Test
+    void shouldHandleActualValue() {
+        // given
+        final JsonNullable<String> given = JsonNullable.of("actual new value");
+
+        // when
+        OptionalFromJson.of(given).ifPresent(valueFromJson -> value = valueFromJson);
+
+        // then
+        assertThat(value).isEqualTo("actual new value");
+    }
+
+    @Test
+    void shouldHandleNullValue() {
+        // given
+        final JsonNullable<String> given = JsonNullable.of(null);
+
+        // when
+        OptionalFromJson.of(given).ifPresent(valueFromJson -> value = valueFromJson);
+
+        // then
+        assertThat(value).isNull();
+    }
+
+    @Test
+    void shouldHandleUndefinedValue() {
+        // given - what should have happen in the JSON mapper
+        final JsonNullable<String> given = JsonNullable.undefined();
+
+        // when
+        OptionalFromJson.of(given).ifPresent(valueFromJson -> value = valueFromJson);
+
+        // then
+        assertThat(value).isEqualTo("unchanged initial value");
+    }
+
+    @Test
+    void shouldHandleInvalidNullJsonNullable() {
+        // given - what seems to happen in the JSON mapper
+        final JsonNullable<String> given = null;
+
+        // when
+        OptionalFromJson.of(given).ifPresent(valueFromJson -> value = valueFromJson);
+
+        // then
+        assertThat(value).isEqualTo("unchanged initial value");
+    }
+}