From dd36946f256331494e46e1f24a0edecb1878fbec Mon Sep 17 00:00:00 2001 From: "Cai Yufei (INST/ECS1)" Date: Wed, 23 May 2018 15:58:44 +0200 Subject: [PATCH] Take Thing ID into account in IndexLengthRestrictionEnforcer Attempt to prevent creation of documents whose index entry does not fit in 1024 bytes. Index content is now restricted thus: namespace + thing-id + feature-id + name-of-attribute-or-feature-property + value <= 950. Values of (top-level) attributes and feature properties exceeding the limit are truncated. Signed-off-by: Cai Yufei (INST/ECS1) --- ...bstractThingsSearchUpdaterPersistence.java | 20 +---- .../write/IndexLengthRestrictionEnforcer.java | 83 ++++++++++--------- .../MongoThingsSearchUpdaterPersistence.java | 6 +- .../IndexLengthRestrictionEnforcerTest.java | 65 ++++----------- ...ctMongoEventToPersistenceStrategyTest.java | 3 +- 5 files changed, 68 insertions(+), 109 deletions(-) diff --git a/services/thingsearch/persistence/src/main/java/org/eclipse/ditto/services/thingsearch/persistence/write/AbstractThingsSearchUpdaterPersistence.java b/services/thingsearch/persistence/src/main/java/org/eclipse/ditto/services/thingsearch/persistence/write/AbstractThingsSearchUpdaterPersistence.java index c93f30c756..488ab37972 100644 --- a/services/thingsearch/persistence/src/main/java/org/eclipse/ditto/services/thingsearch/persistence/write/AbstractThingsSearchUpdaterPersistence.java +++ b/services/thingsearch/persistence/src/main/java/org/eclipse/ditto/services/thingsearch/persistence/write/AbstractThingsSearchUpdaterPersistence.java @@ -27,10 +27,6 @@ public abstract class AbstractThingsSearchUpdaterPersistence implements ThingsSe * The logger. */ protected final LoggingAdapter log; - /** - * The restriction helper. - */ - protected final IndexLengthRestrictionEnforcer indexLengthRestrictionEnforcer; /** * Default contructor. @@ -38,19 +34,7 @@ public abstract class AbstractThingsSearchUpdaterPersistence implements ThingsSe * @param loggingAdapter the logger to use for logging. */ public AbstractThingsSearchUpdaterPersistence(final LoggingAdapter loggingAdapter) { - this(loggingAdapter, IndexLengthRestrictionEnforcer.newInstance(loggingAdapter)); - } - - /** - * Default contructor. - * - * @param loggingAdapter the logger to use for logging. - * @param indexLengthRestrictionEnforcer the restriction helper. - */ - public AbstractThingsSearchUpdaterPersistence(final LoggingAdapter loggingAdapter, - final IndexLengthRestrictionEnforcer indexLengthRestrictionEnforcer) { - this.log = loggingAdapter; - this.indexLengthRestrictionEnforcer = indexLengthRestrictionEnforcer; + log = loggingAdapter; } /** @@ -60,7 +44,7 @@ public AbstractThingsSearchUpdaterPersistence(final LoggingAdapter loggingAdapte public Source insertOrUpdate(final Thing thing, final long revision, final long policyRevision) { // enforce the restrictions on the data - final Thing toSave = indexLengthRestrictionEnforcer.enforceRestrictions(thing); + final Thing toSave = IndexLengthRestrictionEnforcer.enforceRestrictions(log, thing); return save(toSave, revision, policyRevision) .recoverWithRetries(1, errorRecovery(getThingId(toSave))); } diff --git a/services/thingsearch/persistence/src/main/java/org/eclipse/ditto/services/thingsearch/persistence/write/IndexLengthRestrictionEnforcer.java b/services/thingsearch/persistence/src/main/java/org/eclipse/ditto/services/thingsearch/persistence/write/IndexLengthRestrictionEnforcer.java index b3a0f4abe9..e40e79ca11 100644 --- a/services/thingsearch/persistence/src/main/java/org/eclipse/ditto/services/thingsearch/persistence/write/IndexLengthRestrictionEnforcer.java +++ b/services/thingsearch/persistence/src/main/java/org/eclipse/ditto/services/thingsearch/persistence/write/IndexLengthRestrictionEnforcer.java @@ -11,6 +11,8 @@ */ package org.eclipse.ditto.services.thingsearch.persistence.write; +import static org.eclipse.ditto.services.thingsearch.persistence.PersistenceConstants.SLASH; + import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -32,7 +34,6 @@ import org.eclipse.ditto.model.things.FeaturesBuilder; import org.eclipse.ditto.model.things.Thing; import org.eclipse.ditto.model.things.ThingBuilder; -import org.eclipse.ditto.services.thingsearch.persistence.PersistenceConstants; import akka.event.LoggingAdapter; @@ -43,42 +44,24 @@ public final class IndexLengthRestrictionEnforcer { /** - * Max allowed length of feature properties. - */ - static final int MAX_FEATURE_PROPERTY_VALUE_LENGTH = 950; - /** - * Max allowed length of attributes. - */ - static final int MAX_ATTRIBUTE_VALUE_LENGTH = 950; - - /** - * The overhead caused by the json key of attribute entries. Use {@link IndexLengthRestrictionEnforcer#attributeOverhead()} - * ()} for calculating the concrete overhead. - */ - private static final int ATTRIBUTE_KEY_OVERHEAD = ("attributes").length(); - - /** - * The overhead caused by the json key of feature properties. Use {@link IndexLengthRestrictionEnforcer#featurePropertyOverhead(String)} - * for calculating the concrete overhead. + * Max allowed length of index content. */ - private static final int FEATURE_PROPERTY_KEY_OVERHEAD = ("features" + - PersistenceConstants.SLASH - // feature id - + PersistenceConstants.SLASH - + "properties").length(); + static final int MAX_INDEX_CONTENT_LENGTH = 950; /** * The logging adapter used to log size restriction enforcements. */ private final LoggingAdapter log; + private final int thingIdNamespaceOverhead; /** * Default constructor. * * @param log the logging adapter used to log size restriction enforcements. */ - private IndexLengthRestrictionEnforcer(final LoggingAdapter log) { + private IndexLengthRestrictionEnforcer(final LoggingAdapter log, final String thingId) { this.log = log; + this.thingIdNamespaceOverhead = calculateThingIdNamespaceOverhead(thingId); } /** @@ -87,8 +70,14 @@ private IndexLengthRestrictionEnforcer(final LoggingAdapter log) { * @param loggingAdapter the logging adapter used to log size restriction enforcements. * @return the instance. */ - public static IndexLengthRestrictionEnforcer newInstance(final LoggingAdapter loggingAdapter) { - return new IndexLengthRestrictionEnforcer(loggingAdapter); + public static IndexLengthRestrictionEnforcer newInstance(final LoggingAdapter loggingAdapter, + final String thingId) { + return new IndexLengthRestrictionEnforcer(loggingAdapter, thingId); + } + + public static Thing enforceRestrictions(final LoggingAdapter log, final Thing thing) { + return new IndexLengthRestrictionEnforcer(log, thing.getId().orElse("")) + .enforceRestrictionsWithoutCheckingIdOverhead(thing); } /** @@ -98,6 +87,10 @@ public static IndexLengthRestrictionEnforcer newInstance(final LoggingAdapter lo * @return The thing with content that satisfies the thresholds. */ public Thing enforceRestrictions(final Thing thing) { + return IndexLengthRestrictionEnforcer.enforceRestrictions(log, thing); + } + + private Thing enforceRestrictionsWithoutCheckingIdOverhead(final Thing thing) { // check if features exceed limits final Map> exceededFeatures = calculateThresholdViolations(thing.getFeatures().orElse(Features.newBuilder().build())); @@ -123,7 +116,7 @@ public Thing enforceRestrictions(final Thing thing) { fixViolation(jsonField.getKey().asPointer(), jsonField.getValue(), featurePropertyOverhead(feature.getId()), - MAX_FEATURE_PROPERTY_VALUE_LENGTH)))); + MAX_INDEX_CONTENT_LENGTH)))); intermediateThing = builder.build(); } @@ -187,14 +180,14 @@ public JsonValue enforceRestrictionsOnFeatureProperty(final String featureId, fi if (violatesThreshold(featurePointer, propertyValue, featurePropertyOverhead(featureId), - MAX_FEATURE_PROPERTY_VALUE_LENGTH)) { + MAX_INDEX_CONTENT_LENGTH)) { log.warning("Feature Property <{}> of Feature <{}> exceeds size restrictions.", featurePointer.toString(), featureId); return fixViolation(featurePointer, propertyValue, featurePropertyOverhead(featureId), - MAX_FEATURE_PROPERTY_VALUE_LENGTH); + MAX_INDEX_CONTENT_LENGTH); } return propertyValue; } @@ -256,9 +249,9 @@ public JsonValue enforceRestrictionsOnAttributeValue(final JsonPointer attribute final JsonValue attributeValue) { if (violatesThreshold(attributeKey, attributeValue, attributeOverhead(), - MAX_ATTRIBUTE_VALUE_LENGTH)) { + MAX_INDEX_CONTENT_LENGTH)) { log.warning("Attribute <{}> exceeds size restrictions", attributeKey.toString()); - return fixViolation(attributeKey, attributeValue, attributeOverhead(), MAX_ATTRIBUTE_VALUE_LENGTH); + return fixViolation(attributeKey, attributeValue, attributeOverhead(), MAX_INDEX_CONTENT_LENGTH); } return attributeValue; } @@ -290,7 +283,7 @@ private Attributes fixViolations(final Set exceededAttributes, fixViolation(field.getKey().asPointer(), field.getValue(), attributeOverhead(), - MAX_ATTRIBUTE_VALUE_LENGTH))); + MAX_INDEX_CONTENT_LENGTH))); return builder.build(); } @@ -304,7 +297,7 @@ private FeatureProperties fixViolations(final String featureId, jsonField.getKey().asPointer(), jsonField.getValue(), featurePropertyOverhead(featureId), - MAX_FEATURE_PROPERTY_VALUE_LENGTH); + MAX_INDEX_CONTENT_LENGTH); featurePropertiesBuilder.set(jsonField.getKey().asPointer(), restrictedValue); }); @@ -315,7 +308,7 @@ private JsonValue fixViolation(final JsonPointer key, final JsonValue value, final int overhead, final int threshold) { - final int cutAt = threshold - totalOverhead(key, overhead); + final int cutAt = Math.max(0, threshold - totalOverhead(key, overhead)); return JsonValue.of(value.asString().substring(0, cutAt)); } @@ -325,7 +318,7 @@ private Set calculateThresholdViolations(final Attributes attributes) .filter(field -> violatesThreshold(field.getKey().asPointer(), field.getValue(), attributeOverhead(), - MAX_ATTRIBUTE_VALUE_LENGTH)) + MAX_INDEX_CONTENT_LENGTH)) .collect(Collectors.toSet()); } @@ -352,7 +345,7 @@ private Set calculateThresholdViolations(final String featureId, field.getKey().asPointer(), field.getValue(), featurePropertyOverhead(featureId), - MAX_FEATURE_PROPERTY_VALUE_LENGTH)) + MAX_INDEX_CONTENT_LENGTH)) .collect(Collectors.toSet()); } @@ -367,7 +360,7 @@ private boolean violatesThreshold(final JsonPointer key, } private int totalOverhead(final JsonPointer key, final int additionalOverhead) { - return key.toString().length() + additionalOverhead; + return jsonPointerLengthWithoutStartingSlash(key) + additionalOverhead; } /** @@ -376,7 +369,7 @@ private int totalOverhead(final JsonPointer key, final int additionalOverhead) { * @return The overhead as a positive int. */ private int attributeOverhead() { - return ATTRIBUTE_KEY_OVERHEAD; + return thingIdNamespaceOverhead; } /** @@ -386,7 +379,7 @@ private int attributeOverhead() { * @return The overhead as a positive int. */ private int featurePropertyOverhead(final String featureKey) { - return FEATURE_PROPERTY_KEY_OVERHEAD + featureKey.length(); + return thingIdNamespaceOverhead + featureKey.length(); } @@ -400,5 +393,17 @@ private String getThingId(final Thing thing) { return thing.getId().orElseThrow(() -> new IllegalArgumentException("The thing has no ID!")); } + private static int calculateThingIdNamespaceOverhead(final String thingId) { + final int namespaceLength = Math.max(0, thingId.indexOf(':')); + return thingId.length() + namespaceLength; + } + + private static int jsonPointerLengthWithoutStartingSlash(final JsonPointer jsonPointer) { + final String stringRepresentation = jsonPointer.toString(); + return stringRepresentation.startsWith(SLASH) + ? stringRepresentation.length() - 1 + : stringRepresentation.length(); + } + } diff --git a/services/thingsearch/persistence/src/main/java/org/eclipse/ditto/services/thingsearch/persistence/write/impl/MongoThingsSearchUpdaterPersistence.java b/services/thingsearch/persistence/src/main/java/org/eclipse/ditto/services/thingsearch/persistence/write/impl/MongoThingsSearchUpdaterPersistence.java index 4a936987b2..feff7ce23e 100644 --- a/services/thingsearch/persistence/src/main/java/org/eclipse/ditto/services/thingsearch/persistence/write/impl/MongoThingsSearchUpdaterPersistence.java +++ b/services/thingsearch/persistence/src/main/java/org/eclipse/ditto/services/thingsearch/persistence/write/impl/MongoThingsSearchUpdaterPersistence.java @@ -45,6 +45,7 @@ import org.eclipse.ditto.services.thingsearch.persistence.mapping.ThingDocumentMapper; import org.eclipse.ditto.services.thingsearch.persistence.write.AbstractThingsSearchUpdaterPersistence; import org.eclipse.ditto.services.thingsearch.persistence.write.EventToPersistenceStrategyFactory; +import org.eclipse.ditto.services.thingsearch.persistence.write.IndexLengthRestrictionEnforcer; import org.eclipse.ditto.services.thingsearch.persistence.write.ThingMetadata; import org.eclipse.ditto.services.thingsearch.persistence.write.ThingsSearchUpdaterPersistence; import org.eclipse.ditto.services.utils.persistence.mongo.MongoClientWrapper; @@ -139,7 +140,8 @@ private static Document toUpdate(final Document document) { @Override protected final Source save(final Thing thing, final long revision, final long policyRevision) { log.debug("Saving Thing with revision <{}> and policy revision <{}>: <{}>", revision, policyRevision, thing); - final Bson filter = filterWithLowerThingRevisionOrLowerPolicyRevision(getThingId(thing), revision, policyRevision); + final Bson filter = + filterWithLowerThingRevisionOrLowerPolicyRevision(getThingId(thing), revision, policyRevision); final Document document = toUpdate(ThingDocumentMapper.toDocument(thing), revision, policyRevision); return Source.fromPublisher(collection.updateOne(filter, document, new UpdateOptions().upsert(true))) .map(updateResult -> updateResult.getMatchedCount() > 0 || null != updateResult.getUpsertedId()); @@ -257,7 +259,7 @@ private List> createThingUpdates(fin final T thingEvent) { final List updates = persistenceStrategyFactory .getStrategy(thingEvent) - .thingUpdates(thingEvent, indexLengthRestrictionEnforcer); + .thingUpdates(thingEvent, IndexLengthRestrictionEnforcer.newInstance(log, thingEvent.getThingId())); return updates .stream() .map(update -> new UpdateOneModel(filter, update, new UpdateOptions().upsert(true))) diff --git a/services/thingsearch/persistence/src/test/java/org/eclipse/ditto/services/thingsearch/persistence/write/IndexLengthRestrictionEnforcerTest.java b/services/thingsearch/persistence/src/test/java/org/eclipse/ditto/services/thingsearch/persistence/write/IndexLengthRestrictionEnforcerTest.java index f51c8e01ad..588156b449 100644 --- a/services/thingsearch/persistence/src/test/java/org/eclipse/ditto/services/thingsearch/persistence/write/IndexLengthRestrictionEnforcerTest.java +++ b/services/thingsearch/persistence/src/test/java/org/eclipse/ditto/services/thingsearch/persistence/write/IndexLengthRestrictionEnforcerTest.java @@ -22,7 +22,6 @@ import org.eclipse.ditto.model.things.FeatureProperties; import org.eclipse.ditto.model.things.Features; import org.eclipse.ditto.model.things.Thing; -import org.eclipse.ditto.services.thingsearch.persistence.PersistenceConstants; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -44,13 +43,13 @@ public final class IndexLengthRestrictionEnforcerTest { @Before public void setUp() { - this.indexLengthRestrictionEnforcer = IndexLengthRestrictionEnforcer.newInstance(log); + this.indexLengthRestrictionEnforcer = IndexLengthRestrictionEnforcer.newInstance(log, ""); } @Test public void newInstance() { final IndexLengthRestrictionEnforcer - indexLengthRestrictionEnforcer = IndexLengthRestrictionEnforcer.newInstance(log); + indexLengthRestrictionEnforcer = IndexLengthRestrictionEnforcer.newInstance(log, ""); assertThat(indexLengthRestrictionEnforcer).isNotNull(); } @@ -92,8 +91,8 @@ public void enforceRestrictionsOnThingAttributeViolation() { final String thingId = ":home-box"; final String key = "version"; final int maxAllowedValueForKey = - IndexLengthRestrictionEnforcer.MAX_ATTRIBUTE_VALUE_LENGTH - - ("attributes".length() + PersistenceConstants.SLASH.length() + key.length()); + IndexLengthRestrictionEnforcer.MAX_INDEX_CONTENT_LENGTH - + (key.length() + thingId.length()); final String value = createString(maxAllowedValueForKey + 1); final String featureId1 = "text-too-speech-feature"; @@ -135,14 +134,8 @@ public void enforceRestrictionsOnThingFeatureViolation() { final String featureId1 = "text-too-speech-feature"; final String featureId2 = "illuminance-sensor"; final int maxAllowedValueForKey = - IndexLengthRestrictionEnforcer.MAX_FEATURE_PROPERTY_VALUE_LENGTH - - ("features".length() - + PersistenceConstants.SLASH.length() - + featureId1.length() + - +PersistenceConstants.SLASH.length() - + "properties".length() - + PersistenceConstants.SLASH.length() - + "last-message".length()); + IndexLengthRestrictionEnforcer.MAX_INDEX_CONTENT_LENGTH - + (featureId1.length() + thingId.length() + "last-message".length()); final String value = createString(maxAllowedValueForKey + 1); final Feature feature1 = Feature.newBuilder() @@ -204,14 +197,8 @@ public void enforceRestrictionsOnFeaturesViolation() { final String featureId1 = "text-too-speech-feature"; final String featureId2 = "illuminance-sensor"; final int maxAllowedValueForKey = - IndexLengthRestrictionEnforcer.MAX_FEATURE_PROPERTY_VALUE_LENGTH - - ("features".length() - + PersistenceConstants.SLASH.length() - + featureId1.length() + - +PersistenceConstants.SLASH.length() - + "properties".length() - + PersistenceConstants.SLASH.length() - + "last-message".length()); + IndexLengthRestrictionEnforcer.MAX_INDEX_CONTENT_LENGTH - + (featureId1.length() + "last-message".length()); final String value = createString(maxAllowedValueForKey + 1); final Feature feature1 = Feature.newBuilder() .withId(featureId1) @@ -249,14 +236,8 @@ public void enforceRestrictionsOnFeaturePropertyViolation() { final String featureId = "text-too-speech-feature"; final String key = "last-message"; final int maxAllowedValueForKey = - IndexLengthRestrictionEnforcer.MAX_FEATURE_PROPERTY_VALUE_LENGTH - - ("features".length() - + PersistenceConstants.SLASH.length() - + featureId.length() + - +PersistenceConstants.SLASH.length() - + "properties".length() - + PersistenceConstants.SLASH.length() - + "last-message".length()); + IndexLengthRestrictionEnforcer.MAX_INDEX_CONTENT_LENGTH - + (featureId.length() + "last-message".length()); final String value = createString(maxAllowedValueForKey + 1); assertThat(indexLengthRestrictionEnforcer.enforceRestrictionsOnFeatureProperty(featureId, JsonPointer.of(key), @@ -279,14 +260,8 @@ public void enforceRestrictionsOnFeatureProperties() { public void enforceRestrictionsOnFeaturePropertiesViolation() { final String featureId = "text-too-speech-feature"; final int maxAllowedValueForKey = - IndexLengthRestrictionEnforcer.MAX_FEATURE_PROPERTY_VALUE_LENGTH - - ("features".length() - + PersistenceConstants.SLASH.length() - + featureId.length() + - + PersistenceConstants.SLASH.length() - + "properties".length() - + PersistenceConstants.SLASH.length() - + "last-message".length()); + IndexLengthRestrictionEnforcer.MAX_INDEX_CONTENT_LENGTH - + (featureId.length() + "last-message".length()); final String value = createString(maxAllowedValueForKey + 1); final FeatureProperties featureProperties = FeatureProperties.newBuilder() @@ -316,14 +291,8 @@ public void enforceRestrictionsOnFeature() { public void enforceRestrictionsOnFeatureViolation() { final String featureId = "text-too-speech-feature"; final int maxAllowedValueForKey = - IndexLengthRestrictionEnforcer.MAX_FEATURE_PROPERTY_VALUE_LENGTH - - ("features".length() - + PersistenceConstants.SLASH.length() - + featureId.length() + - + PersistenceConstants.SLASH.length() - + "properties".length() - + PersistenceConstants.SLASH.length() - + "last-message".length()); + IndexLengthRestrictionEnforcer.MAX_INDEX_CONTENT_LENGTH - + (featureId.length() + "last-message".length()); final String value = createString(maxAllowedValueForKey + 1); final Feature feature = Feature.newBuilder() @@ -352,8 +321,7 @@ public void enforceRestrictionsOnAttributes() { public void enforceRestrictionsOnAttributesViolation() { final String key = "description"; final int maxAllowedValueForKey = - IndexLengthRestrictionEnforcer.MAX_ATTRIBUTE_VALUE_LENGTH - - ("attributes".length() + PersistenceConstants.SLASH.length() + key.length()); + IndexLengthRestrictionEnforcer.MAX_INDEX_CONTENT_LENGTH - key.length(); final String value = createString(maxAllowedValueForKey + 1); final String expected = value.substring(0, maxAllowedValueForKey); @@ -379,8 +347,7 @@ public void enforceRestrictionsOnAttributeValue() { public void enforceRestrictionsOnAttributeValueViolation() { final JsonPointer key = JsonPointer.of("description"); final int maxAllowedValueForKey = - IndexLengthRestrictionEnforcer.MAX_ATTRIBUTE_VALUE_LENGTH - - ("attributes".length() + key.toString().length()); + IndexLengthRestrictionEnforcer.MAX_INDEX_CONTENT_LENGTH - "description".length(); final JsonValue value = JsonValue.of(createString(maxAllowedValueForKey + 1)); final JsonValue expected = JsonValue.of(value.asString().substring(0, maxAllowedValueForKey)); assertThat(value).isNotEqualTo(expected); diff --git a/services/thingsearch/persistence/src/test/java/org/eclipse/ditto/services/thingsearch/persistence/write/impl/AbstractMongoEventToPersistenceStrategyTest.java b/services/thingsearch/persistence/src/test/java/org/eclipse/ditto/services/thingsearch/persistence/write/impl/AbstractMongoEventToPersistenceStrategyTest.java index 33014f1b6a..a642bd8ee9 100644 --- a/services/thingsearch/persistence/src/test/java/org/eclipse/ditto/services/thingsearch/persistence/write/impl/AbstractMongoEventToPersistenceStrategyTest.java +++ b/services/thingsearch/persistence/src/test/java/org/eclipse/ditto/services/thingsearch/persistence/write/impl/AbstractMongoEventToPersistenceStrategyTest.java @@ -53,7 +53,8 @@ public static List versions() { @Before public void setUpMocks() { policyEnforcer = Mockito.mock(PolicyEnforcer.class); - indexLengthRestrictionEnforcer = IndexLengthRestrictionEnforcer.newInstance(Mockito.mock(LoggingAdapter.class)); + indexLengthRestrictionEnforcer = + IndexLengthRestrictionEnforcer.newInstance(Mockito.mock(LoggingAdapter.class), ""); final EffectedSubjectIds effectedSubjectIds = ImmutableEffectedSubjectIds.of(Collections.emptyList(), Collections.emptyList());