From f1735b7cf5b65c669ce3906b8347f41b4f3fd611 Mon Sep 17 00:00:00 2001 From: taefi Date: Fri, 29 Nov 2024 16:20:02 +0200 Subject: [PATCH 1/3] fix: log operation validator messages Fixes #2916 --- .../java/com/vaadin/hilla/signals/Signal.java | 7 +++++++ .../hilla/signals/core/event/StateEvent.java | 18 ++++++++++++++++-- packages/ts/react-signals/src/ListSignal.ts | 5 ++++- packages/ts/react-signals/src/ValueSignal.ts | 2 +- 4 files changed, 28 insertions(+), 4 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 index c574bbe11..5821532e7 100644 --- 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 @@ -133,6 +133,13 @@ private void notifySubscribers(ObjectNode processedEvent) { delegate.notifySubscribers(processedEvent); return; } + if (StateEvent.isRejected(processedEvent)) { + LOGGER.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) { 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 index 80c3fb3cb..b28cdcc4c 100644 --- 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 @@ -7,8 +7,6 @@ import java.util.Arrays; import java.util.Objects; import java.util.Optional; -import java.util.function.Function; -import java.util.function.Supplier; /** * A utility class for representing state events out of an ObjectNode. This @@ -256,6 +254,22 @@ public static boolean isRejected(ObjectNode event) { && event.get(Field.VALIDATION_ERROR).asText() != null; } + 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) { return MAPPER.valueToTree(value); } diff --git a/packages/ts/react-signals/src/ListSignal.ts b/packages/ts/react-signals/src/ListSignal.ts index 5c26d4b27..95c8c8692 100644 --- a/packages/ts/react-signals/src/ListSignal.ts +++ b/packages/ts/react-signals/src/ListSignal.ts @@ -64,7 +64,10 @@ export class ListSignal extends CollectionSignal 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(event)) { diff --git a/packages/ts/react-signals/src/ValueSignal.ts b/packages/ts/react-signals/src/ValueSignal.ts index 83114ab11..e2dce28d3 100644 --- a/packages/ts/react-signals/src/ValueSignal.ts +++ b/packages/ts/react-signals/src/ValueSignal.ts @@ -111,7 +111,7 @@ export class ValueSignal extends FullStackSignal { if (event.accepted || isSnapshotStateEvent(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 From 8b71cf1d4fd7198ca4dc91486e2f3e9ea55d4b5c Mon Sep 17 00:00:00 2001 From: taefi Date: Mon, 2 Dec 2024 13:35:10 +0200 Subject: [PATCH 2/3] add tests for StateEvent public api --- .../hilla/signals/core/event/StateEvent.java | 6 ++- .../signals/core/event/StateEventTest.java | 51 +++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) 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 index b28cdcc4c..b33dd3db4 100644 --- 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 @@ -251,7 +251,7 @@ 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) { @@ -352,4 +352,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/core/event/StateEventTest.java b/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/core/event/StateEventTest.java index f43b37e01..4aae30fcf 100644 --- 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)); + } } From 65fc76c11f5507b6e6fe2d6c6503059ae57231c2 Mon Sep 17 00:00:00 2001 From: taefi Date: Mon, 2 Dec 2024 14:33:15 +0200 Subject: [PATCH 3/3] add tests for Signal warning log --- .../java/com/vaadin/hilla/signals/Signal.java | 14 +++++---- .../vaadin/hilla/signals/ValueSignalTest.java | 29 +++++++++++++++++++ 2 files changed, 37 insertions(+), 6 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 index 5821532e7..6ddeef46b 100644 --- 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 @@ -16,8 +16,6 @@ public abstract class Signal { - private static final Logger LOGGER = LoggerFactory.getLogger(Signal.class); - private final ReentrantLock lock = new ReentrantLock(); private final UUID id = UUID.randomUUID(); @@ -76,7 +74,7 @@ public Flux subscribe() { .onBackpressureBuffer(); return sink.asFlux().doOnSubscribe(ignore -> { - LOGGER.debug("New Flux subscription..."); + getLogger().debug("New Flux subscription..."); lock.lock(); try { var snapshot = createSnapshotEvent(); @@ -88,7 +86,7 @@ public Flux subscribe() { }).doFinally(ignore -> { lock.lock(); try { - LOGGER.debug("Unsubscribing from Signal..."); + getLogger().debug("Unsubscribing from Signal..."); subscribers.remove(sink); } finally { lock.unlock(); @@ -134,7 +132,7 @@ private void notifySubscribers(ObjectNode processedEvent) { return; } if (StateEvent.isRejected(processedEvent)) { - LOGGER.warn( + getLogger().warn( "Operation with id '{}' is rejected with validator message: '{}'", StateEvent.extractId(processedEvent), StateEvent.extractValidationError(processedEvent)); @@ -143,7 +141,7 @@ private void notifySubscribers(ObjectNode processedEvent) { subscribers.removeIf(sink -> { boolean failure = sink.tryEmitNext(processedEvent).isFailure(); if (failure) { - LOGGER.debug("Failed push"); + getLogger().debug("Failed push"); } return failure; }); @@ -210,4 +208,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/test/java/com/vaadin/hilla/signals/ValueSignalTest.java b/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/ValueSignalTest.java index c27768650..6de797dee 100644 --- 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 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 ObjectNode createSetEvent(T value) { var setEvent = new StateEvent<>(UUID.randomUUID().toString(), StateEvent.EventType.SET, value);