Skip to content

Commit

Permalink
Merge pull request #1696 from eclipse-ditto/bugfix/if-equals-wrong-ht…
Browse files Browse the repository at this point in the history 
…tp-status-code

#1690 use correct HTTP status code for "if-equal": skip on equality
  • Loading branch information
@thjaeckle
thjaeckle authored Jul 21, 2023
2 parents 8840cb6 + 73fb51c commit 1539e35
Show file tree
Hide file tree
Showing 10 changed files with 100 additions and 52 deletions.
9 changes: 9 additions & 0 deletions ...connectivity/model/signals/commands/exceptions/ConnectionPreconditionFailedException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,15 @@ private ConnectionPreconditionFailedException(final DittoHeaders dittoHeaders,
super(ERROR_CODE, HttpStatus.PRECONDITION_FAILED, dittoHeaders, message, description, cause, href);
}

/**
* A mutable builder for a {@link ConnectionPreconditionFailedException}.
*
* @return the builder.
*/
public static Builder newBuilder() {
return new Builder();
}

/**
* A mutable builder for a {@link ConnectionPreconditionFailedException}.
*
Expand Down
4 changes: 2 additions & 2 deletions ...aging/persistence/strategies/commands/ConnectionsConditionalHeadersValidatorProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ public DittoRuntimeExceptionBuilder<?> createPreconditionNotModifiedExceptionBui
}

@Override
public DittoRuntimeExceptionBuilder<?> createPreconditionNotModifiedForEqualityExceptionBuilder() {
return ConnectionPreconditionNotModifiedException.newBuilder()
public DittoRuntimeExceptionBuilder<?> createPreconditionFailedForEqualityExceptionBuilder() {
return ConnectionPreconditionFailedException.newBuilder()
.message("The previous value was equal to the new value and the 'if-equal' header was set to 'skip'.")
.description("Your changes were not applied, which is probably the expected outcome.");
}
Expand Down
6 changes: 3 additions & 3 deletions ...ava/org/eclipse/ditto/internal/utils/headers/conditional/ConditionalHeadersValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,14 @@ DittoRuntimeExceptionBuilder<?> createPreconditionNotModifiedExceptionBuilder(St
String matched);

/**
* Returns a builder for a {@link DittoRuntimeException} in case status {@code 304 (Not Modified)} should be
* returned for when an updated value was equal to its previous value and the {@code if-equal} condition was
* Returns a builder for a {@link DittoRuntimeException} in case status {@code 412 (Precondition failed)} should
* be returned for when an updated value was equal to its previous value and the {@code if-equal} condition was
* set to "skip".
*
* @return the builder.
* @since 3.3.0
*/
DittoRuntimeExceptionBuilder<?> createPreconditionNotModifiedForEqualityExceptionBuilder();
DittoRuntimeExceptionBuilder<?> createPreconditionFailedForEqualityExceptionBuilder();
}

private final ValidationSettings validationSettings;
Expand Down
97 changes: 59 additions & 38 deletions .../java/org/eclipse/ditto/internal/utils/headers/conditional/IfEqualPreconditionHeader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,43 +111,10 @@ public boolean meetsConditionFor(@Nullable final Entity<?> entity) {
if (ifEqual == IfEqual.SKIP) {
if (command.getCategory() == Command.Category.MODIFY &&
command instanceof WithOptionalEntity withOptionalEntity) {
return withOptionalEntity.getEntity()
.map(newValue -> {
final Predicate<JsonField> nonHiddenAndNamespace =
FieldType.notHidden()
.or(jsonField -> jsonField.getKey().equals(JsonKey.of("_namespace")));
final Optional<JsonValue> previousValue = entity.toJson(JsonSchemaVersion.LATEST, nonHiddenAndNamespace)
.getValue(command.getResourcePath());
return previousValue.filter(jsonValue -> jsonValue.equals(newValue))
.isPresent();
})
.orElse(false);
return meetsConditionForModifyCommand(entity, withOptionalEntity);
} else if (command.getCategory() == Command.Category.MERGE &&
command instanceof WithOptionalEntity withOptionalEntity) {
return withOptionalEntity.getEntity()
.map(newValue -> {
final Optional<JsonValue> previousValue = entity.toJson()
.getValue(command.getResourcePath());
if (newValue.isObject()) {
return previousValue.filter(JsonValue::isObject)
.map(JsonValue::asObject)
.filter(previousObject -> {
final JsonObject patchedAndSortedNewObject =
JsonFactory.mergeJsonValues(newValue.asObject(), previousObject)
.asObject().stream()
.sorted(Comparator.comparing(j -> j.getKey().toString()))
.collect(JsonCollectors.fieldsToObject());
final JsonObject sortedOldObject = previousObject.stream()
.sorted(Comparator.comparing(j -> j.getKey().toString()))
.collect(JsonCollectors.fieldsToObject());
return patchedAndSortedNewObject.equals(sortedOldObject);
}).isPresent();
} else {
return previousValue.filter(jsonValue -> jsonValue.equals(newValue))
.isPresent();
}
})
.orElse(false);
return meetsConditionForMergeCommand(entity, withOptionalEntity);
} else {
// other commands to "MODIFY" and "MERGE" do never match the "if-equal" precondition header
return false;
Expand All @@ -158,6 +125,60 @@ public boolean meetsConditionFor(@Nullable final Entity<?> entity) {
}
}

private Boolean meetsConditionForModifyCommand(final Entity<?> entity,
final WithOptionalEntity withOptionalEntity) {

return withOptionalEntity.getEntity()
.map(newValue -> {
final Predicate<JsonField> nonHiddenAndNamespace =
FieldType.notHidden()
.or(jsonField -> jsonField.getKey().equals(JsonKey.of("_namespace")));
final Optional<JsonValue> previousValue =
entity.toJson(JsonSchemaVersion.LATEST, nonHiddenAndNamespace)
.getValue(command.getResourcePath());
return previousValue.filter(jsonValue -> jsonValue.equals(newValue))
.isPresent();
})
.orElse(false);
}

private Boolean meetsConditionForMergeCommand(final Entity<?> entity, final WithOptionalEntity withOptionalEntity) {

return withOptionalEntity.getEntity()
.map(newValue -> {
final Optional<JsonValue> previousValue = entity.toJson().getValue(command.getResourcePath());
if (newValue.isObject()) {
final JsonObject newObject;
if (command.getResourcePath().isEmpty()) {
// filter "special fields" for e.g. on thing level the inline "_policy":
newObject = newValue.asObject()
.stream()
.filter(jsonField -> !jsonField.getKeyName().startsWith("_"))
.collect(JsonCollectors.fieldsToObject());
} else {
newObject = newValue.asObject();
}
return previousValue.filter(JsonValue::isObject)
.map(JsonValue::asObject)
.filter(previousObject -> {
final JsonObject patchedAndSortedNewObject =
JsonFactory.mergeJsonValues(newObject, previousObject)
.asObject().stream()
.sorted(Comparator.comparing(j -> j.getKey().toString()))
.collect(JsonCollectors.fieldsToObject());
final JsonObject sortedOldObject = previousObject.stream()
.sorted(Comparator.comparing(j -> j.getKey().toString()))
.collect(JsonCollectors.fieldsToObject());
return patchedAndSortedNewObject.equals(sortedOldObject);
}).isPresent();
} else {
return previousValue.filter(jsonValue -> jsonValue.equals(newValue))
.isPresent();
}
})
.orElse(false);
}

/**
* Handles the {@link #command} field of this class by invoking the passed {@code isCompletelyEqualSupplier} to
* check whether the affected entity would be completely equal after applying the {@link #command}.
Expand All @@ -176,7 +197,7 @@ C handleCommand(final BooleanSupplier isCompletelyEqualSupplier) {
final Command.Category category = command.getCategory();
if (completelyEqual &&
(category == Command.Category.MODIFY || category == Command.Category.MERGE)) {
potentiallyAdjustedCommand = respondWithNotModified();
potentiallyAdjustedCommand = respondWithPreconditionFailed();
} else {
potentiallyAdjustedCommand = command;
}
Expand All @@ -186,9 +207,9 @@ C handleCommand(final BooleanSupplier isCompletelyEqualSupplier) {
return potentiallyAdjustedCommand;
}

private C respondWithNotModified() {
private C respondWithPreconditionFailed() {
throw validationSettings
.createPreconditionNotModifiedForEqualityExceptionBuilder()
.createPreconditionFailedForEqualityExceptionBuilder()
.dittoHeaders(command.getDittoHeaders())
.build();
}
Expand Down
11 changes: 10 additions & 1 deletion ...e/ditto/policies/model/signals/commands/exceptions/PolicyPreconditionFailedException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@
import javax.annotation.concurrent.Immutable;
import javax.annotation.concurrent.NotThreadSafe;

import org.eclipse.ditto.json.JsonObject;
import org.eclipse.ditto.base.model.common.HttpStatus;
import org.eclipse.ditto.base.model.exceptions.DittoRuntimeException;
import org.eclipse.ditto.base.model.exceptions.DittoRuntimeExceptionBuilder;
import org.eclipse.ditto.base.model.headers.DittoHeaders;
import org.eclipse.ditto.base.model.json.JsonParsableException;
import org.eclipse.ditto.json.JsonObject;
import org.eclipse.ditto.policies.model.PolicyException;

/**
Expand Down Expand Up @@ -55,6 +55,15 @@ private PolicyPreconditionFailedException(final DittoHeaders dittoHeaders,
super(ERROR_CODE, HttpStatus.PRECONDITION_FAILED, dittoHeaders, message, description, cause, href);
}

/**
* A mutable builder for a {@link org.eclipse.ditto.policies.model.signals.commands.exceptions.PolicyPreconditionFailedException}.
*
* @return the builder.
*/
public static Builder newBuilder() {
return new Builder();
}

/**
* A mutable builder for a {@link org.eclipse.ditto.policies.model.signals.commands.exceptions.PolicyPreconditionFailedException}.
*
Expand Down
4 changes: 2 additions & 2 deletions ...e/persistence/actors/strategies/commands/PoliciesConditionalHeadersValidatorProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ public DittoRuntimeExceptionBuilder<?> createPreconditionNotModifiedExceptionBui
}

@Override
public DittoRuntimeExceptionBuilder<?> createPreconditionNotModifiedForEqualityExceptionBuilder() {
return PolicyPreconditionNotModifiedException.newBuilder()
public DittoRuntimeExceptionBuilder<?> createPreconditionFailedForEqualityExceptionBuilder() {
return PolicyPreconditionFailedException.newBuilder()
.message("The previous value was equal to the new value and the 'if-equal' header was set to 'skip'.")
.description("Your changes were not applied, which is probably the expected outcome.");
}
Expand Down
2 changes: 1 addition & 1 deletion ...rvice/persistence/actors/strategies/commands/PoliciesConditionalHeadersValidatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ public void ifEqualDoesThrowExceptionWhenIfEqualSkipAndValueIsEqual() {
final ModifyPolicyImport command = ModifyPolicyImport.of(policyId, policyImport,
DittoHeaders.newBuilder().ifEqual(IfEqual.SKIP).build());

assertThatExceptionOfType(PolicyPreconditionNotModifiedException.class)
assertThatExceptionOfType(PolicyPreconditionFailedException.class)
.isThrownBy(() -> SUT.applyIfEqualHeader(command, policy))
.withMessage("The previous value was equal to the new value and the 'if-equal' header was set to 'skip'.");
}
Expand Down
11 changes: 10 additions & 1 deletion ...ipse/ditto/things/model/signals/commands/exceptions/ThingPreconditionFailedException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@
import javax.annotation.concurrent.Immutable;
import javax.annotation.concurrent.NotThreadSafe;

import org.eclipse.ditto.json.JsonObject;
import org.eclipse.ditto.base.model.common.HttpStatus;
import org.eclipse.ditto.base.model.exceptions.DittoRuntimeException;
import org.eclipse.ditto.base.model.exceptions.DittoRuntimeExceptionBuilder;
import org.eclipse.ditto.base.model.headers.DittoHeaders;
import org.eclipse.ditto.base.model.json.JsonParsableException;
import org.eclipse.ditto.json.JsonObject;
import org.eclipse.ditto.things.model.ThingException;

/**
Expand Down Expand Up @@ -55,6 +55,15 @@ private ThingPreconditionFailedException(final DittoHeaders dittoHeaders,
super(ERROR_CODE, HttpStatus.PRECONDITION_FAILED, dittoHeaders, message, description, cause, href);
}

/**
* A mutable builder for a {@link ThingPreconditionFailedException}.
*
* @return the builder.
*/
public static Builder newBuilder() {
return new Builder();
}

/**
* A mutable builder for a {@link ThingPreconditionFailedException}.
*
Expand Down
4 changes: 2 additions & 2 deletions ...ice/persistence/actors/strategies/commands/ThingsConditionalHeadersValidatorProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ public DittoRuntimeExceptionBuilder<?> createPreconditionNotModifiedExceptionBui
}

@Override
public DittoRuntimeExceptionBuilder<?> createPreconditionNotModifiedForEqualityExceptionBuilder() {
return ThingPreconditionNotModifiedException.newBuilder()
public DittoRuntimeExceptionBuilder<?> createPreconditionFailedForEqualityExceptionBuilder() {
return ThingPreconditionFailedException.newBuilder()
.message("The previous value was equal to the new value and the 'if-equal' header was set to 'skip'.")
.description("Your changes were not applied, which is probably the expected outcome.");
}
Expand Down
4 changes: 2 additions & 2 deletions ...service/persistence/actors/strategies/commands/ThingsConditionalHeadersValidatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ public void ifEqualDoesThrowExceptionWhenIfEqualSkipAndValueIsEqual() {
final ModifyAttribute command = ModifyAttribute.of(thingId, attributePath, attributeValue,
DittoHeaders.newBuilder().ifEqual(IfEqual.SKIP).build());

assertThatExceptionOfType(ThingPreconditionNotModifiedException.class)
assertThatExceptionOfType(ThingPreconditionFailedException.class)
.isThrownBy(() -> SUT.applyIfEqualHeader(command, thing))
.withMessage("The previous value was equal to the new value and the 'if-equal' header was set to 'skip'.");
}
Expand All @@ -196,7 +196,7 @@ public void ifEqualDoesThrowExceptionWhenIfEqualSkipAndValueIsEqualUsingMerge()
final MergeThing command = MergeThing.of(thingId, JsonPointer.empty(), thing.toJson(),
DittoHeaders.newBuilder().ifEqual(IfEqual.SKIP).build());

assertThatExceptionOfType(ThingPreconditionNotModifiedException.class)
assertThatExceptionOfType(ThingPreconditionFailedException.class)
.isThrownBy(() -> SUT.applyIfEqualHeader(command, thing))
.withMessage("The previous value was equal to the new value and the 'if-equal' header was set to 'skip'.");
}
Expand Down

0 comments on commit 1539e35

Please sign in to comment.