Skip to content

Commit

Permalink
review:
Browse files Browse the repository at this point in the history 
* use "<not-available>" as correlationId in LogEntryFactory if no real one is available
* enhanced error.json and protocol-error_response.json JsonSchemas with "additionalProperties"
* don't include the complete "adaptable" in the JSON of the IllegalAdaptableException but only the required "topicPath" instead

Signed-off-by: Thomas Jaeckle <thomas.jaeckle@bosch.io>
  • Loading branch information
@thjaeckle
thjaeckle committed Dec 22, 2021
1 parent d5f95d6 commit 52da147
Show file tree
Hide file tree
Showing 9 changed files with 114 additions and 57 deletions.
12 changes: 8 additions & 4 deletions ...in/java/org/eclipse/ditto/connectivity/api/messaging/monitoring/logs/LogEntryFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@
@Immutable
public final class LogEntryFactory {

/**
* The fallback correlation ID to be used if no other was provided.
*/
public static final String FALLBACK_CORRELATION_ID = "<not-provided>";

private LogEntryFactory() {
throw new AssertionError();
}
Expand All @@ -65,7 +70,7 @@ public static LogEntry getLogEntryForFailedCommandResponseRoundTrip(final Comman
ConditionChecker.checkNotNull(commandResponse, "commandResponse");

final var logEntryBuilder = ConnectivityModelFactory.newLogEntryBuilder(
getCorrelationId(command).or(() -> getCorrelationId(commandResponse)).orElse("n/a"),
getCorrelationId(command).or(() -> getCorrelationId(commandResponse)).orElse(FALLBACK_CORRELATION_ID),
Instant.now(),
LogCategory.RESPONSE,
LogType.DROPPED,
Expand Down Expand Up @@ -111,7 +116,7 @@ public static LogEntry getLogEntryForIllegalCommandResponseAdaptable(
ConditionChecker.checkNotNull(illegalAdaptableException, "illegalAdaptableException");

final var logEntryBuilder = ConnectivityModelFactory.newLogEntryBuilder(
getCorrelationId(illegalAdaptableException).orElse("n/a"),
getCorrelationId(illegalAdaptableException).orElse(FALLBACK_CORRELATION_ID),
Instant.now(),
LogCategory.RESPONSE,
LogType.DROPPED,
Expand All @@ -125,8 +130,7 @@ public static LogEntry getLogEntryForIllegalCommandResponseAdaptable(
}

private static Optional<EntityId> getEntityId(final IllegalAdaptableException illegalAdaptableException) {
final var adaptable = illegalAdaptableException.getAdaptable();
return getEntityIdFromTopicPath(adaptable.getTopicPath());
return illegalAdaptableException.getTopicPath().flatMap(LogEntryFactory::getEntityIdFromTopicPath);
}

private static Optional<EntityId> getEntityIdFromTopicPath(final TopicPath topicPath) {
Expand Down
2 changes: 1 addition & 1 deletion ...ava/org/eclipse/ditto/connectivity/api/messaging/monitoring/logs/LogEntryFactoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public void getLogEntryForFailedCommandResponseRoundTripWithNoCorrelationIdRetur
commandResponse,
DETAIL_MESSAGE_FAILURE);

assertThat(logEntry.getCorrelationId()).isEqualTo("n/a");
assertThat(logEntry.getCorrelationId()).isEqualTo(LogEntryFactory.FALLBACK_CORRELATION_ID);
}

@Test
Expand Down
6 changes: 3 additions & 3 deletions ...rc/main/java/org/eclipse/ditto/connectivity/service/messaging/InboundDispatchingSink.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -402,9 +402,9 @@ private Optional<Signal<?>> handleIllegalAdaptableExceptionForLiveResponse(
) {
final Optional<Signal<?>> result;
countAndLogIllegalAdaptableExceptionForLiveResponse(illegalAdaptableException, inboundMessage);
final var adaptable = illegalAdaptableException.getAdaptable();
if (isResponseRequired(adaptable)) {
result = Optional.of(toErrorResponseFunction.apply(illegalAdaptableException, adaptable.getTopicPath()));
final Optional<TopicPath> topicPath = illegalAdaptableException.getTopicPath();
if (isResponseRequired(illegalAdaptableException) && topicPath.isPresent()) {
result = Optional.of(toErrorResponseFunction.apply(illegalAdaptableException, topicPath.get()));
} else {
result = Optional.empty();
}
Expand Down
3 changes: 2 additions & 1 deletion ...org/eclipse/ditto/connectivity/service/messaging/monitoring/logs/InfoProviderFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,15 @@
import org.eclipse.ditto.base.model.headers.DittoHeaderDefinition;
import org.eclipse.ditto.base.model.signals.Signal;
import org.eclipse.ditto.connectivity.api.ExternalMessage;
import org.eclipse.ditto.connectivity.api.messaging.monitoring.logs.LogEntryFactory;
import org.eclipse.ditto.connectivity.service.messaging.monitoring.ConnectionMonitor;

/**
* Factory for creating instances of {@link org.eclipse.ditto.connectivity.service.messaging.monitoring.ConnectionMonitor.InfoProvider}.
*/
public final class InfoProviderFactory {

public static final String FALLBACK_CORRELATION_ID = "<not-provided>";
public static final String FALLBACK_CORRELATION_ID = LogEntryFactory.FALLBACK_CORRELATION_ID;

private InfoProviderFactory() {
throw new AssertionError();
Expand Down
15 changes: 14 additions & 1 deletion documentation/src/main/resources/jsonschema/error.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,18 @@
"status",
"error",
"message"
]
],
"additionalProperties": {
"title": "Additional error information",
"description": "Additional error information helping to better classify the error.",
"type": [
"array",
"boolean",
"integer",
"number",
"object",
"string",
"null"
]
}
}
15 changes: 14 additions & 1 deletion documentation/src/main/resources/jsonschema/protocol-error_response.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,20 @@
"status",
"error",
"message"
]
],
"additionalProperties": {
"title": "Additional error information",
"description": "Additional error information helping to better classify the error.",
"type": [
"array",
"boolean",
"integer",
"number",
"object",
"string",
"null"
]
}
},
"status": {
"type": "integer",
Expand Down
2 changes: 1 addition & 1 deletion ...ain/java/org/eclipse/ditto/gateway/service/endpoints/routes/websocket/WebSocketRoute.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ private Flow<Message, String, NotUsed> getStrictifyFlow(final HttpRequest reques
.origin(connectionCorrelationId)
.build());
final var tracedFailure = traceSignalBuildingFailure(failure);
if (isResponseRequired(e.getAdaptable())) {
if (isResponseRequired(e)) {
result = Left.apply(tracedFailure);
} else {
result = Right.apply(Left.apply(NoOp.getInstance()));
Expand Down
104 changes: 66 additions & 38 deletions ...src/main/java/org/eclipse/ditto/protocol/mappingstrategies/IllegalAdaptableException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@
import org.eclipse.ditto.json.JsonObjectBuilder;
import org.eclipse.ditto.json.JsonParseException;
import org.eclipse.ditto.protocol.Adaptable;
import org.eclipse.ditto.protocol.JsonifiableAdaptable;
import org.eclipse.ditto.protocol.ProtocolFactory;
import org.eclipse.ditto.protocol.TopicPath;

/**
* This exception is thrown to indicate that a particular {@link Adaptable}
Expand All @@ -61,20 +61,15 @@ public final class IllegalAdaptableException extends DittoRuntimeException {
*/
static final HttpStatus HTTP_STATUS = HttpStatus.UNPROCESSABLE_ENTITY;

static final JsonFieldDefinition<JsonObject> JSON_FIELD_ADAPTABLE =
JsonFieldDefinition.ofJsonObject("adaptable", FieldType.REGULAR, JsonSchemaVersion.V_2);
static final JsonFieldDefinition<String> JSON_FIELD_TOPIC_PATH =
JsonFieldDefinition.ofString("topicPath", FieldType.REGULAR, JsonSchemaVersion.V_2);

static final JsonFieldDefinition<String> JSON_FIELD_SIGNAL_TYPE =
JsonFieldDefinition.ofString("signalType", FieldType.REGULAR, JsonSchemaVersion.V_2);

private static final long serialVersionUID = 1552465013185426687L;

private final transient Adaptable adaptable;

@SuppressWarnings({"java:S3077", "java:S1165"})
@Nullable
private transient volatile JsonObject adaptableJsonObject;

@Nullable private final transient TopicPath topicPath;
@Nullable private final transient CharSequence signalType;

private IllegalAdaptableException(final String errorCode,
Expand All @@ -89,8 +84,7 @@ private IllegalAdaptableException(final String errorCode,
builder.description,
builder.cause,
builder.href);
adaptable = builder.adaptable;
adaptableJsonObject = null;
topicPath = builder.topicPath;
signalType = builder.signalType;
}

Expand Down Expand Up @@ -119,7 +113,10 @@ public static IllegalAdaptableException newInstance(final String message,
@Nullable final String description,
final Adaptable adaptable) {

return newBuilder(message, adaptable).withDescription(description).build();
return newBuilder(message, adaptable)
.withTopicPath(adaptable.getTopicPath())
.withDescription(description)
.build();
}

/**
Expand All @@ -132,7 +129,22 @@ public static IllegalAdaptableException newInstance(final String message,
* @throws IllegalArgumentException if {@code message} is blank.
*/
public static Builder newBuilder(final String message, final Adaptable adaptable) {
return new Builder(message, adaptable);
ConditionChecker.checkNotNull(adaptable, "adaptable");
return newBuilder(message, adaptable.getDittoHeaders())
.withTopicPath(adaptable.getTopicPath());
}

/**
* Returns a mutable builder with a fluent API for constructing an instance of {@code IllegalAdaptableException}.
*
* @param message the detail message of the exception.
* @param dittoHeaders the {@code DittoHeaders} to use for the exception.
* @return the builder.
* @throws NullPointerException if any argument is {@code null}.
* @throws IllegalArgumentException if {@code message} is blank.
*/
public static Builder newBuilder(final String message, final DittoHeaders dittoHeaders) {
return new Builder(message, dittoHeaders);
}

/**
Expand All @@ -153,7 +165,8 @@ public static IllegalAdaptableException fromJson(final JsonObject jsonObject, fi
deserializeErrorCode(jsonObject),
deserializeHttpStatus(jsonObject),
dittoHeaders,
newBuilder(jsonObject.getValueOrThrow(JsonFields.MESSAGE), deserializeAdaptable(jsonObject))
newBuilder(jsonObject.getValueOrThrow(JsonFields.MESSAGE), dittoHeaders)
.withTopicPath(deserializeTopicPath(jsonObject).orElse(null))
.withSignalType(deserializeSignalType(jsonObject).orElse(null))
.withDescription(jsonObject.getValue(JsonFields.DESCRIPTION).orElse(null))
.withHref(deserializeHref(jsonObject).orElse(null))
Expand Down Expand Up @@ -197,8 +210,18 @@ private static HttpStatus deserializeHttpStatus(final JsonObject jsonObject) {
}
}

private static Adaptable deserializeAdaptable(final JsonObject jsonObject) {
return ProtocolFactory.jsonifiableAdaptableFromJson(jsonObject.getValueOrThrow(JSON_FIELD_ADAPTABLE));
private static Optional<TopicPath> deserializeTopicPath(final JsonObject jsonObject) {
final Optional<TopicPath> result;
final JsonFieldDefinition<String> fieldDefinition = JSON_FIELD_TOPIC_PATH;
final Optional<String> deserializedTopicPathOptional = jsonObject.getValue(fieldDefinition);
if (deserializedTopicPathOptional.isPresent() && isBlank(deserializedTopicPathOptional.get())) {
throw new JsonParseException(
MessageFormat.format("Value of field <{0}> must not be blank.", fieldDefinition.getPointer())
);
} else {
result = deserializedTopicPathOptional.map(ProtocolFactory::newTopicPath);
}
return result;
}

private static Optional<String> deserializeSignalType(final JsonObject jsonObject) {
Expand Down Expand Up @@ -234,12 +257,12 @@ private static Optional<URI> deserializeHref(final JsonObject jsonObject) {
}

/**
* Returns the illegal {@code Adaptable} that is subject to this exception.
* Returns the{@code TopicPath} if available.
*
* @return the illegal {@code Adaptable}.
* @return the optional topic path.
*/
public Adaptable getAdaptable() {
return adaptable;
public Optional<TopicPath> getTopicPath() {
return Optional.ofNullable(topicPath);
}

/**
Expand All @@ -259,7 +282,8 @@ public DittoRuntimeException setDittoHeaders(final DittoHeaders dittoHeaders) {
getErrorCode(),
getHttpStatus(),
dittoHeaders,
newBuilder(getMessage(), adaptable)
newBuilder(getMessage(), dittoHeaders)
.withTopicPath(topicPath)
.withSignalType(signalType)
.withDescription(getDescription().orElse(null))
.withCause(getCause())
Expand All @@ -269,22 +293,14 @@ public DittoRuntimeException setDittoHeaders(final DittoHeaders dittoHeaders) {

@Override
protected void appendToJson(final JsonObjectBuilder jsonObjectBuilder, final Predicate<JsonField> predicate) {
jsonObjectBuilder.set(JSON_FIELD_ADAPTABLE, getAdaptableJsonObject());
if (null != topicPath) {
jsonObjectBuilder.set(JSON_FIELD_TOPIC_PATH, topicPath.getPath());
}
if (null != signalType) {
jsonObjectBuilder.set(JSON_FIELD_SIGNAL_TYPE, signalType.toString());
}
}

private JsonObject getAdaptableJsonObject() {
JsonObject result = adaptableJsonObject;
if (null == result) {
final JsonifiableAdaptable jsonifiableAdaptable = ProtocolFactory.wrapAsJsonifiableAdaptable(adaptable);
result = jsonifiableAdaptable.toJson();
adaptableJsonObject = result;
}
return result;
}

@Override
public boolean equals(@Nullable final Object o) {
if (this == o) {
Expand All @@ -297,13 +313,13 @@ public boolean equals(@Nullable final Object o) {
return false;
}
final IllegalAdaptableException that = (IllegalAdaptableException) o;
return Objects.equals(getAdaptableJsonObject(), that.getAdaptableJsonObject()) &&
return Objects.equals(topicPath, that.topicPath) &&
Objects.equals(signalType, that.signalType);
}

@Override
public int hashCode() {
return Objects.hash(super.hashCode(), getAdaptableJsonObject(), signalType);
return Objects.hash(super.hashCode(), topicPath, signalType);
}

/**
Expand All @@ -313,15 +329,16 @@ public int hashCode() {
public static final class Builder {

private final String message;
private final Adaptable adaptable;
private final DittoHeaders dittoHeaders;
@Nullable private CharSequence signalType;
@Nullable private TopicPath topicPath;
@Nullable private String description;
@Nullable private Throwable cause;
@Nullable private URI href;

private Builder(final String message, final Adaptable adaptable) {
private Builder(final String message, final DittoHeaders dittoHeaders) {
this.message = checkCharSequenceArgumentNotBlank(message, "message");
this.adaptable = ConditionChecker.checkNotNull(adaptable, "adaptable");
this.dittoHeaders = ConditionChecker.checkNotNull(dittoHeaders, "dittoHeaders");
}

private static <T extends CharSequence> T checkCharSequenceArgumentNotBlank(final T charSequence,
Expand Down Expand Up @@ -349,6 +366,17 @@ public Builder withSignalType(@Nullable final CharSequence signalType) {
return this;
}

/**
* Sets the specified argument as {@code TopicPath}.
*
* @param topicPath the topic path.
* @return this builder instance for method chaining.
*/
public Builder withTopicPath(@Nullable final TopicPath topicPath) {
this.topicPath = topicPath;
return this;
}

/**
* Sets the specified string argument as description.
* The description provides further information about the cause or possible solution for the failure.
Expand Down Expand Up @@ -394,7 +422,7 @@ public Builder withHref(@Nullable final URI href) {
* @return the new {@code IllegalAdaptableException}.
*/
public IllegalAdaptableException build() {
return new IllegalAdaptableException(ERROR_CODE, HTTP_STATUS, adaptable.getDittoHeaders(), this);
return new IllegalAdaptableException(ERROR_CODE, HTTP_STATUS, dittoHeaders, this);
}

}
Expand Down

0 comments on commit 52da147

Please sign in to comment.