Skip to content

Commit

Permalink
#2074: fix creating a thing with a merge/patch request keeps null values
Browse files Browse the repository at this point in the history
  • Loading branch information
@thjaeckle
thjaeckle committed Jan 2, 2025
1 parent 1921e28 commit 69a26e2
Show file tree
Hide file tree
Showing 2 changed files with 141 additions and 15 deletions.
55 changes: 40 additions & 15 deletions ...java/org/eclipse/ditto/things/service/enforcement/pre/ModifyToCreateThingTransformer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import org.apache.pekko.actor.ActorSystem;
import org.eclipse.ditto.base.model.signals.Signal;
import org.eclipse.ditto.base.service.signaltransformer.SignalTransformer;
import org.eclipse.ditto.json.JsonCollectors;
import org.eclipse.ditto.json.JsonField;
import org.eclipse.ditto.json.JsonObject;
import org.eclipse.ditto.things.model.Thing;
import org.eclipse.ditto.things.model.ThingsModelFactory;
Expand Down Expand Up @@ -65,24 +67,47 @@ public CompletionStage<Signal<?>> apply(final Signal<?> signal) {
}

private static Optional<InputParams> calculateInputParams(final Signal<?> signal) {
if (signal instanceof ModifyThing modifyThing) {
return Optional.of(new InputParams(modifyThing,
modifyThing.getThing(),
modifyThing.getInitialPolicy().orElse(null),
modifyThing.getPolicyIdOrPlaceholder().orElse(null)
));
} else if (signal instanceof MergeThing mergeThing && mergeThing.getPath().isEmpty()) {
final JsonObject mergeThingObject = mergeThing.getEntity().orElseGet(mergeThing::getValue).asObject();
return Optional.of(new InputParams(mergeThing,
ThingsModelFactory.newThing(mergeThingObject),
mergeThing.getInitialPolicy().orElse(null),
mergeThing.getPolicyIdOrPlaceholder().orElse(null)
));
} else {
return Optional.empty();
switch (signal) {
case ModifyThing modifyThing -> {
return Optional.of(new InputParams(modifyThing,
modifyThing.getThing(),
modifyThing.getInitialPolicy().orElse(null),
modifyThing.getPolicyIdOrPlaceholder().orElse(null)
));
}
case MergeThing mergeThing when mergeThing.getPath().isEmpty() -> {
final JsonObject mergeThingObject = mergeThing.getEntity().orElseGet(mergeThing::getValue).asObject();
final JsonObject nullFilteredMergeThingObject = filterOutNullValues(mergeThingObject);
return Optional.of(new InputParams(mergeThing,
ThingsModelFactory.newThing(nullFilteredMergeThingObject),
mergeThing.getInitialPolicy().orElse(null),
mergeThing.getPolicyIdOrPlaceholder().orElse(null)
));
}
default -> {
return Optional.empty();
}
}
}

private static JsonObject filterOutNullValues(final JsonObject mergeThingObject) {
return mergeThingObject.stream()
.filter(entry -> !entry.getValue().isNull())
.map(field -> {
if (field.getValue().isObject()) {
return JsonField.newInstance(field.getKey(), filterOutNullValues(field.getValue().asObject()));
} else if (field.getValue().isArray()) {
return JsonField.newInstance(field.getKey(), field.getValue().asArray().stream()
.filter(value -> !value.isNull())
.map(value -> value.isObject() ? filterOutNullValues(value.asObject()) : value)
.collect(JsonCollectors.valuesToArray()));
} else {
return field;
}
})
.collect(JsonCollectors.fieldsToObject());
}

private record InputParams(ThingModifyCommand<?> thingModifyCommand, Thing thing,
@Nullable JsonObject initialPolicy,
@Nullable String policyIdOrPlaceholder) {}
Expand Down
101 changes: 101 additions & 0 deletions .../org/eclipse/ditto/things/service/enforcement/pre/ModifyToCreateThingTransformerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@

import org.eclipse.ditto.base.model.headers.DittoHeaders;
import org.eclipse.ditto.base.model.signals.Signal;
import org.eclipse.ditto.json.JsonArray;
import org.eclipse.ditto.json.JsonObject;
import org.eclipse.ditto.json.JsonPointer;
import org.eclipse.ditto.json.JsonValue;
import org.eclipse.ditto.things.model.Attributes;
import org.eclipse.ditto.things.model.Feature;
import org.eclipse.ditto.things.model.FeatureProperties;
import org.eclipse.ditto.things.model.Features;
import org.eclipse.ditto.things.model.Thing;
import org.eclipse.ditto.things.model.ThingId;
import org.eclipse.ditto.things.model.ThingsModelFactory;
Expand Down Expand Up @@ -77,6 +85,99 @@ public void mergeThingBecomesCreateThingPolicyWhenNotYetExisting() {
verify(existenceChecker).checkExistence(mergeThing);
}

@Test
public void mergeThingWithNullsBecomesCreateThingRemovingNullsWhenNotYetExisting() {
final var thingId = ThingId.generateRandom();
final var attributes = Attributes.newBuilder()
.set("one", 1)
.set("oops_null", JsonValue.nullLiteral())
.set("null_value", "null")
.build();
final var mergeThing = MergeThing.withThing(thingId, Thing.newBuilder()
.setId(thingId)
.setAttributes(attributes)
.build(),
DittoHeaders.of(Map.of("foo", "bar")));
when(existenceChecker.checkExistence(mergeThing)).thenReturn(CompletableFuture.completedFuture(false));

final Signal<?> result = underTest.apply(mergeThing).toCompletableFuture().join();

assertThat(result).isInstanceOf(CreateThing.class);
final CreateThing createThing = (CreateThing) result;
assertThat(createThing.getEntityId().toString()).hasToString(thingId.toString());
final Thing thingWithoutNullValues = ThingsModelFactory.newThingBuilder(mergeThing.getValue().asObject())
.removeAttribute(JsonPointer.of("oops_null"))
.build();
assertThat(createThing.getThing()).isEqualTo(thingWithoutNullValues);
assertThat(createThing.getDittoHeaders()).isSameAs(mergeThing.getDittoHeaders());
verify(existenceChecker).checkExistence(mergeThing);
}

@Test
public void mergeThingWithNestedNullsInFeaturesBecomesCreateThingRemovingNullsWhenNotYetExisting() {
final var thingId = ThingId.generateRandom();
final var attributes = Attributes.newBuilder()
.set("one", 1)
.set("oops_null", JsonValue.nullLiteral())
.set("null_value", "null")
.build();
final var features = Features.newBuilder()
.set(Feature.newBuilder()
.properties(FeatureProperties.newBuilder()
.set("some_property", "123")
.set("some_property_null", JsonValue.nullLiteral())
.set("some_object", JsonObject.newBuilder()
.set("some_nested_int", 123)
.set("some_nested_null", JsonValue.nullLiteral())
.build()
)
.build()
)
.withId("feature1")
.build()
)
.set(Feature.newBuilder()
.desiredProperties(FeatureProperties.newBuilder()
.set("some_desired_property", "123")
.set("some_desired_property_null", JsonValue.nullLiteral())
.set("some_desired_array", JsonArray.newBuilder()
.add(JsonObject.newBuilder()
.set("some_desired_array_nested_int", 123)
.set("some_desired_array_nested_null", JsonValue.nullLiteral())
.build())
.build()
)
.build()
)
.withId("feature2")
.build()
)
.build();
final var mergeThing = MergeThing.withThing(thingId, Thing.newBuilder()
.setId(thingId)
.setAttributes(attributes)
.setFeatures(features)
.build(),
DittoHeaders.of(Map.of("foo", "bar")));
when(existenceChecker.checkExistence(mergeThing)).thenReturn(CompletableFuture.completedFuture(false));

final Signal<?> result = underTest.apply(mergeThing).toCompletableFuture().join();

assertThat(result).isInstanceOf(CreateThing.class);
final CreateThing createThing = (CreateThing) result;
assertThat(createThing.getEntityId().toString()).hasToString(thingId.toString());
final Thing thingWithoutNullValues = ThingsModelFactory.newThingBuilder(mergeThing.getValue().asObject())
.removeAttribute(JsonPointer.of("oops_null"))
.removeFeatureProperty("feature1", JsonPointer.of("some_property_null"))
.removeFeatureProperty("feature1", JsonPointer.of("some_object/some_nested_null"))
.removeFeatureDesiredProperty("feature2", JsonPointer.of("some_desired_property_null"))
.removeFeatureDesiredProperty("feature2", JsonPointer.of("some_desired_array/some_desired_array_nested_null"))
.build();
assertThat(createThing.getThing()).isEqualTo(thingWithoutNullValues);
assertThat(createThing.getDittoHeaders()).isSameAs(mergeThing.getDittoHeaders());
verify(existenceChecker).checkExistence(mergeThing);
}

@Test
public void mergeThingStaysMergeThingWhenAlreadyExisting() {
final var thingId = ThingId.generateRandom();
Expand Down

0 comments on commit 69a26e2

Please sign in to comment.