Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: log operation validator messages #2939

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,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();
Expand Down Expand Up @@ -76,7 +74,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();
Expand All @@ -88,7 +86,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();
Expand Down Expand Up @@ -133,10 +131,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;
});
Expand Down Expand Up @@ -203,4 +208,8 @@ public int hashCode() {
public static void setMapper(ObjectMapper mapper) {
StateEvent.setMapper(mapper);
}

private Logger getLogger() {
return LoggerFactory.getLogger(Signal.class);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -253,7 +251,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) {
Expand Down Expand Up @@ -338,4 +352,8 @@ public String getValidationError() {
public void setValidationError(String validationError) {
this.validationError = validationError;
}

public void clearValidationError() {
this.validationError = null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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 {

Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
5 changes: 4 additions & 1 deletion packages/ts/react-signals/src/ListSignal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
2 changes: 1 addition & 1 deletion packages/ts/react-signals/src/ValueSignal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading