fix: log operation validator messages (#2939) (CP: 24.6) (#3083)

fix: log operation validator messages (#2939) * fix: log operation validator messages Fixes #2916 * add tests for StateEvent public api * add tests for Signal warning log --------- Co-authored-by: Soroosh Taefi <[email protected]> Co-authored-by: Vlad Rindevich <[email protected]> Co-authored-by: Luciano Vernaschi <[email protected]>
vaadin · Dec 23, 2024 · 3d40552 · 3d40552
1 parent 021afbc
commit 3d40552
Show file tree

Hide file tree

Showing 6 changed files with 120 additions and 8 deletions.
diff --git a/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/Signal.java b/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/Signal.java
@@ -32,8 +32,6 @@
 
 public abstract class Signal<T> {
 
-    private static final Logger LOGGER = LoggerFactory.getLogger(Signal.class);
-
     private final ReentrantLock lock = new ReentrantLock();
 
     private final UUID id = UUID.randomUUID();
@@ -92,7 +90,7 @@ public Flux<ObjectNode> subscribe() {
                 .onBackpressureBuffer();
 
         return sink.asFlux().doOnSubscribe(ignore -> {
-            LOGGER.debug("New Flux subscription...");
+            getLogger().debug("New Flux subscription...");
             lock.lock();
             try {
                 var snapshot = createSnapshotEvent();
@@ -104,7 +102,7 @@ public Flux<ObjectNode> subscribe() {
         }).doFinally(ignore -> {
             lock.lock();
             try {
-                LOGGER.debug("Unsubscribing from Signal...");
+                getLogger().debug("Unsubscribing from Signal...");
                 subscribers.remove(sink);
             } finally {
                 lock.unlock();
@@ -149,10 +147,17 @@ private void notifySubscribers(ObjectNode processedEvent) {
             delegate.notifySubscribers(processedEvent);
             return;
         }
+        if (StateEvent.isRejected(processedEvent)) {
+            getLogger().warn(
+                    "Operation with id '{}' is rejected with validator message: '{}'",
+                    StateEvent.extractId(processedEvent),
+                    StateEvent.extractValidationError(processedEvent));
+            StateEvent.clearValidationError(processedEvent);
+        }
         subscribers.removeIf(sink -> {
             boolean failure = sink.tryEmitNext(processedEvent).isFailure();
             if (failure) {
-                LOGGER.debug("Failed push");
+                getLogger().debug("Failed push");
             }
             return failure;
         });
@@ -219,4 +224,8 @@ public int hashCode() {
     public static void setMapper(ObjectMapper mapper) {
         StateEvent.setMapper(mapper);
     }
+
+    private Logger getLogger() {
+        return LoggerFactory.getLogger(Signal.class);
+    }
 }
diff --git a/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/core/event/StateEvent.java b/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/core/event/StateEvent.java
@@ -267,7 +267,23 @@ public static boolean isRejected(ObjectNode event) {
         return event.has(Field.ACCEPTED)
                 && !event.get(Field.ACCEPTED).asBoolean()
                 && event.has(Field.VALIDATION_ERROR)
-                && event.get(Field.VALIDATION_ERROR).asText() != null;
+                && !event.get(Field.VALIDATION_ERROR).asText().isBlank();
+    }
+
+    public static String extractValidationError(ObjectNode event) {
+        if (!isRejected(event)) {
+            throw new IllegalStateException(
+                    "The event is not rejected, so it does not have a validation error");
+        }
+        return event.get(Field.VALIDATION_ERROR).asText();
+    }
+
+    public static void clearValidationError(ObjectNode event) {
+        if (!isRejected(event)) {
+            throw new IllegalStateException(
+                    "The event is not rejected, so it does not have a validation error");
+        }
+        event.remove(Field.VALIDATION_ERROR);
     }
 
     private static JsonNode valueAsJsonNode(Object value) {
@@ -352,4 +368,8 @@ public String getValidationError() {
     public void setValidationError(String validationError) {
         this.validationError = validationError;
     }
+
+    public void clearValidationError() {
+        this.validationError = null;
+    }
 }
diff --git a/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/ValueSignalTest.java b/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/ValueSignalTest.java
@@ -15,7 +15,10 @@
 
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
+import org.mockito.MockedStatic;
 import org.mockito.Mockito;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.springframework.context.ApplicationContext;
 import reactor.core.publisher.Flux;
 
@@ -28,6 +31,10 @@
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 public class ValueSignalTest {
 
@@ -480,6 +487,28 @@ public void readonlyInstance_doesNotAllowAnyModifications() {
         assertEquals("Foo", readonlySignal.getValue());
     }
 
+    @Test
+    public void submit_eventThatIsRejected_logsTheValidationError() {
+        Logger logger = Mockito.mock(Logger.class);
+        try (MockedStatic<LoggerFactory> mockedLoggerFactory = mockStatic(
+                LoggerFactory.class)) {
+
+            mockedLoggerFactory
+                    .when(() -> LoggerFactory.getLogger(Signal.class))
+                    .thenReturn(logger);
+
+            var signal = new ValueSignal<>("Foo", String.class)
+                    .withOperationValidator(op -> ValidationResult
+                            .reject("No changes allowed"));
+            var operation = createSetEvent("Bar");
+            signal.submit(operation);
+
+            verify(logger, times(1)).warn(
+                    "Operation with id '{}' is rejected with validator message: '{}'",
+                    StateEvent.extractId(operation), "No changes allowed");
+        }
+    }
+
     private <T> ObjectNode createSetEvent(T value) {
         var setEvent = new StateEvent<>(UUID.randomUUID().toString(),
                 StateEvent.EventType.SET, value);

diff --git a/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/core/event/StateEventTest.java b/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/core/event/StateEventTest.java
@@ -376,4 +376,55 @@ public void convertValue_jsonWithValidValue_returnsConvertedValue() {
                 eventJson.get(StateEvent.Field.VALUE), Person.class);
         assertEquals(new Person("John Doe", 42, true), person);
     }
+
+    @Test
+    public void extractValidationError_throws_whenEventIsNotRejected() {
+        var eventJson = MAPPER.createObjectNode();
+        assertThrows(IllegalStateException.class,
+                () -> StateEvent.extractValidationError(eventJson));
+    }
+
+    @Test
+    public void extractValidationError_throws_whenEventIsRejectedButDoesNotHaveValidationError() {
+        var event = new StateEvent<>("id", StateEvent.EventType.SET, "value");
+        event.setAccepted(false);
+        event.setValidationError("");
+        assertThrows(IllegalStateException.class,
+                () -> StateEvent.extractValidationError(event.toJson()));
+    }
+
+    @Test
+    public void extractValidationError_returnsValidationError() {
+        var eventJson = MAPPER.createObjectNode();
+        eventJson.put(StateEvent.Field.VALIDATION_ERROR, "error");
+        eventJson.put(StateEvent.Field.ACCEPTED, false);
+        assertEquals("error", StateEvent.extractValidationError(eventJson));
+
+        var event = new StateEvent<>("id", StateEvent.EventType.SET, "value");
+        event.setAccepted(false);
+        event.setValidationError("error");
+        assertEquals("error",
+                StateEvent.extractValidationError(event.toJson()));
+    }
+
+    @Test
+    public void clearValidationError_removesValidationError() {
+        var eventJson = MAPPER.createObjectNode();
+        eventJson.put(StateEvent.Field.VALIDATION_ERROR, "error");
+        eventJson.put(StateEvent.Field.ACCEPTED, false);
+        assertEquals("error", StateEvent.extractValidationError(eventJson));
+        StateEvent.clearValidationError(eventJson);
+        assertFalse(eventJson.has(StateEvent.Field.VALIDATION_ERROR));
+
+        var event = new StateEvent<>("id", StateEvent.EventType.SET, "value");
+        event.setAccepted(false);
+        event.setValidationError("error");
+        assertEquals("error",
+                StateEvent.extractValidationError(event.toJson()));
+        assertEquals("error", event.getValidationError());
+        event.clearValidationError();
+        var eventAsJson = event.toJson();
+        assertNull(event.getValidationError());
+        assertFalse(eventAsJson.has(StateEvent.Field.VALIDATION_ERROR));
+    }
 }
diff --git a/packages/ts/react-signals/src/ListSignal.ts b/packages/ts/react-signals/src/ListSignal.ts
@@ -64,7 +64,10 @@ export class ListSignal<T> extends CollectionSignal<ReadonlyArray<ValueSignal<T>
 
   protected override [$processServerResponse](event: StateEvent): void {
     if (!event.accepted) {
-      this[$resolveOperation](event.id, 'server rejected the operation');
+      this[$resolveOperation](
+        event.id,
+        `Server rejected the operation with id '${event.id}'. See the server log for more details.`,
+      );
       return;
     }
     if (isListSnapshotStateEvent<T>(event)) {

diff --git a/packages/ts/react-signals/src/ValueSignal.ts b/packages/ts/react-signals/src/ValueSignal.ts
@@ -111,7 +111,7 @@ export class ValueSignal<T> extends FullStackSignal<T> {
     if (event.accepted || isSnapshotStateEvent<T>(event)) {
       this.#applyAcceptedEvent(event);
     } else {
-      reason = 'server rejected the operation';
+      reason = `Server rejected the operation with id '${event.id}'. See the server log for more details.`;
     }
     // `then` callbacks can be associated to the record or the event
     // it depends on the operation that was performed