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.

Sign up for GitHub

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: do not crash on unexpected map format in GenericMapTypeHandler #5062

Merged
merged 14 commits into from
Aug 22, 2022
Merged

fix: do not crash on unexpected map format in GenericMapTypeHandler #5062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9496503
fix: do not crash when facing unexpected map format
jdrueckert Aug 15, 2022
c1aba4c
refactor: improve code readability
jdrueckert Aug 15, 2022
f5869a2
test: add tests for partially and fully malformatted input
jdrueckert Aug 21, 2022
cbb6a8c
chore: remove unused import
jdrueckert Aug 21, 2022
a92641b
chore: extract multiple times used map usage info into helper method
jdrueckert Aug 21, 2022
67bca4f
doc: improve test case documentation
jdrueckert Aug 21, 2022
4a8f40a
chore: remove kotlinc.xml
jdrueckert Aug 21, 2022
ba6b2be
Merge branch 'develop' into fix/generic-map-typehandler-resilience
jdrueckert Aug 21, 2022
fbe4e10
Merge branch 'develop' into fix/generic-map-typehandler-resilience
jdrueckert Aug 21, 2022
6e5ed50
fix: add missing return
jdrueckert Aug 21, 2022
1673268
test(TypeHandlerLibrary): use javadoc syntax
keturn Aug 22, 2022
db97071
chore(TypeHandlerLibrary): add toString for GenericMapTypeHandler
keturn Aug 22, 2022
bb8d54e
chore: remove superfluous '"'
jdrueckert Aug 22, 2022
6f5a1b2
Merge branch 'develop' into fix/generic-map-typehandler-resilience
jdrueckert Aug 22, 2022
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
50 changes: 38 additions & 12 deletions ...rc/main/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

package org.terasology.persistence.typeHandling.coreTypes;

import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
import com.google.common.collect.Maps;
import org.slf4j.Logger;
Expand Down Expand Up @@ -61,28 +62,53 @@ private PersistedData serializeEntry(Map.Entry<K, V> entry, PersistedDataSeriali

@Override
public Optional<Map<K, V>> deserialize(PersistedData data) {
if (!data.isArray()) {
if (!data.isArray() || data.isValueMap()) {
keturn marked this conversation as resolved.
Show resolved Hide resolved
logger.warn("Incorrect map format detected: object instead of array.\n" + getUsageInfo(data));
return Optional.empty();
}

Map<K, V> result = Maps.newLinkedHashMap();

for (PersistedData entry : data.getAsArray()) {
PersistedDataMap kvEntry = entry.getAsValueMap();
final Optional<K> key = keyHandler.deserialize(kvEntry.get(KEY));

if (key.isPresent()) {
final Optional<V> value = valueHandler.deserialize(kvEntry.get(VALUE));
if (value.isPresent()) {
result.put(key.get(), value.get());
} else {
logger.warn("Missing value for key '{}' with {} given entry '{}'", key.get(), valueHandler, kvEntry.get(VALUE));
}
} else {
logger.warn("Missing field '{}' for entry '{}'", KEY, kvEntry);
PersistedData rawKey = kvEntry.get(KEY);
PersistedData rawValue = kvEntry.get(VALUE);
if (rawKey == null || rawValue == null) {
logger.warn("Incorrect map format detected: missing map entry with \"key\" or \"value\" key.\n" + getUsageInfo(data));
return Optional.empty();
}

final Optional<K> key = keyHandler.deserialize(rawKey);
if (key.isEmpty()) {
logger.warn("Could not deserialize key '{}' as '{}'", rawKey, keyHandler.getClass().getSimpleName());
return Optional.empty();
Copy link
Member Author

@jdrueckert jdrueckert Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keturn I saw the test cases in GenericMapTypeHandlerTest.java and wanted to ask why it's desirable to have an empty map returned in case of a mismatched key / value handler...? My current assumption is that we might want that because failing to deserialize the ChangingBlocks component should not result in the whole prefab deserialization failing (although I'm not even sure that would be the case when returning Optional.empty() instead 🤔

Can you confirm this assumption or otherwise explain the reason for expecting an empty map in this case?

My intention with using Optional.empty() here is to indicate that the serialization failed but not return a half-way deserialized map as might happen for instance in case the prefab holds:

myMap: [
  { "key": "myKey", "value": "myValue" },
  { "key2": "myKey2", "value2": "myValue2" }
]

With returning Optional.of(result), I would expect the first map entry to be put into the result map and the deserialization failing for the second entry, resulting in a half-way deserialized map which might lead to weird and potentially hard to debug behavior in game.
Taking the growing plant feature, for example, the plant would never grow to the second stage as the second stage never ended up in game because its deserialization failed.

Please correct me if I got any of the type handling logic etc. wrong leading to incorrect assumptions/expectations 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like I wrote those test cases after @skaldarnar wrote the implementation, so we should ask that guy too.

I'm struggling with how to answer these questions because I think you raise some good points, and—

  • I'm not sure how to give advice on the best use of Optional.empty here, because I feel like Optional is bad at communicating error states and I've felt frustrated with trying to use it here myself, and
  • You don't want the result to be a partly-filled data structure. I empathize, but I think a partial response like that would be more consistent with the way the other deserialization methods are written. If I understand correctly, it seems to be working with a best-effort approach, where it'll do its best to return something even if it might have holes in it.

}

final Optional<V> value = valueHandler.deserialize(kvEntry.get(VALUE));
if (value.isEmpty()) {
logger.warn("Could not deserialize value '{}' as '{}'", rawValue, valueHandler.getClass().getSimpleName());
return Optional.empty();
}

result.put(key.get(), value.get());
}

return Optional.of(result);
}

private String getUsageInfo(PersistedData data) {
return "Expected\n" +
" \"mapName\": [\n" +
" { \"key\": \"...\", \"value\": \"...\" }\n" +
" ]\n" +
"but found \n'{}'" + data + "'";
}

@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("key", keyHandler)
.add("value", valueHandler)
.toString();
}
}
140 changes: 137 additions & 3 deletions ...est/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

package org.terasology.persistence.typeHandling.coreTypes;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.terasology.persistence.typeHandling.PersistedData;
import org.terasology.persistence.typeHandling.PersistedDataSerializer;
Expand All @@ -12,7 +13,6 @@
import org.terasology.persistence.typeHandling.inMemory.PersistedString;
import org.terasology.persistence.typeHandling.inMemory.arrays.PersistedValueArray;

import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand All @@ -25,14 +25,102 @@ class GenericMapTypeHandlerTest {
private static final String TEST_KEY = "health:baseRegen";
private static final long TEST_VALUE = -1;

/**
* JSON equivalent:
* <pre><code>
* [
* {
* "key": "health:baseRegen",
* "value": -1
* }
* ]
* </code></pre>
*/
private final PersistedData testData = new PersistedValueArray(List.of(
new PersistedMap(Map.of(
GenericMapTypeHandler.KEY, new PersistedString(TEST_KEY),
GenericMapTypeHandler.VALUE, new PersistedLong(TEST_VALUE)
))
));

/**
* JSON equivalent:
* <pre><code>
* {
* "health:baseRegen": -1
* }
* </code></pre>
*/
private final PersistedData testDataMalformatted = new PersistedValueArray(List.of(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add the JSON equivalent we want to express here in a comment? Probably helps in both understanding the test case and figuring out if the data strucutre represents the thing we want to test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done ✔️

Copy link
Member

@keturn keturn Aug 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😟 It's also the textbook example of "comment that's out of date as soon as you change the code."

If you think it helps despite that, go ahead and keep it, but I'm a little concerned.

Also note Javadoc does not have triple-quote code blocks. Either change these to javadoc syntax, or do not start the comment with the double-asterisk /** so IntelliJ and javdoc don't treat it as javadoc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've replaced the triple-quotes.

new PersistedMap(Map.of(
TEST_KEY, new PersistedLong(TEST_VALUE)
))
));

/**
* JSON equivalent:
* <pre><code>
* [
* {
* "not key": "health:baseRegen",
* "value": -1
* }
* ]
* </code></pre>
*/
private final PersistedData testDataMissingKeyEntry = new PersistedValueArray(List.of(
new PersistedMap(Map.of(
"not key", new PersistedString(TEST_KEY),
GenericMapTypeHandler.VALUE, new PersistedLong(TEST_VALUE)
))
));

/**
* JSON equivalent:
* <pre><code>
* [
* {
* "key": "health:baseRegen",
* "not value": -1
* }
* ]
* </code></pre>
*/
private final PersistedData testDataMissingValueEntry = new PersistedValueArray(List.of(
new PersistedMap(Map.of(
GenericMapTypeHandler.KEY, new PersistedString(TEST_KEY),
"not value", new PersistedLong(TEST_VALUE)
))
));

/**
* JSON equivalent:
* <pre><code>
* [
* {
* "key": "health:baseRegen",
* "value": -1
* },
* {
* "not key": "health:baseRegen",
* "not value": -1
* },
* ]
* </code></pre>
*/
private final PersistedData testDataValidAndInvalidMix = new PersistedValueArray(List.of(
new PersistedMap(Map.of(
GenericMapTypeHandler.KEY, new PersistedString(TEST_KEY),
GenericMapTypeHandler.VALUE, new PersistedLong(TEST_VALUE)
)),
new PersistedMap(Map.of(
"not key", new PersistedString(TEST_KEY),
"not value", new PersistedLong(TEST_VALUE)
))
));

@Test
@DisplayName("Data with valid formatting can be deserialized successfully.")
void testDeserialize() {
var th = new GenericMapTypeHandler<>(
new StringTypeHandler(),
Expand All @@ -44,23 +132,69 @@ void testDeserialize() {
}

@Test
@DisplayName("Deserializing valid data with a mismatching value type handler fails deserialization (returns empty `Optional`)")
void testDeserializeWithMismatchedValueHandler() {
var th = new GenericMapTypeHandler<>(
new StringTypeHandler(),
new UselessTypeHandler<>()
);

assertThat(th.deserialize(testData)).hasValue(Collections.emptyMap());
assertThat(th.deserialize(testData)).isEmpty();
}

@Test
@DisplayName("Deserializing valid data with a mismatching key type handler fails deserialization (returns empty `Optional`)")
void testDeserializeWithMismatchedKeyHandler() {
var th = new GenericMapTypeHandler<>(
new UselessTypeHandler<>(),
new LongTypeHandler()
);

assertThat(th.deserialize(testData)).hasValue(Collections.emptyMap());
assertThat(th.deserialize(testData)).isEmpty();
}

@Test
@DisplayName("Incorrectly formatted data (without an outer array) fails deserialization (returns empty `Optional`)")
void testDeserializeWithObjectInsteadOfArray() {
var th = new GenericMapTypeHandler<>(
new StringTypeHandler(),
new LongTypeHandler()
);

assertThat(th.deserialize(testDataMalformatted)).isEmpty();
}

@Test
@DisplayName("Incorrectly formatted data (without a map entry with key \"key\") fails deserialization (returns empty `Optional`)")
void testDeserializeWithMissingKeyEntry() {
var th = new GenericMapTypeHandler<>(
new StringTypeHandler(),
new LongTypeHandler()
);

assertThat(th.deserialize(testDataMissingKeyEntry)).isEmpty();
}

@Test
@DisplayName("Incorrectly formatted data (without a map entry with key \"value\") fails deserialization (returns empty `Optional`)")
void testDeserializeWithMissingValueEntry() {
var th = new GenericMapTypeHandler<>(
new StringTypeHandler(),
new LongTypeHandler()
);

assertThat(th.deserialize(testDataMissingValueEntry)).isEmpty();
}

@Test
@DisplayName("A map containing both, correctly and incorrectly formatted data, fails deserialization (returns empty `Optional`)")
void testDeserializeWithValidAndInvalidEntries() {
var th = new GenericMapTypeHandler<>(
new StringTypeHandler(),
new LongTypeHandler()
);

assertThat(th.deserialize(testDataValidAndInvalidMix)).isEmpty();
Comment on lines +190 to +197
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the case I'm a little worried about, since it's a change from the previous behavior. Not that the previous behavior was better, but it's one of those things where something might have been implicitly depending on it.

But this has a test case now, and the hypothetical I'm worrying about doesn't, so this wins.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think (or hope) something was depending on it. Rather, we'll see some things either work out better or even worse than before, hopefully just epxosing the underlying problem better.

Copy link
Member Author

@jdrueckert jdrueckert Aug 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My expectation is that if this makes something work worse than before, then it's mainly making the underlying issue more visible than before.

I'm curious: which case was it that you initially bumped in to and prompted this PR?

I ran into the crash after making ChangingBlocks use uris instead of strings for the map key.
The prefabs in PlantPack that I tested this were working before with the basic string key type map deserialization but didn't have the format required by the generic map type handler (outer array, entries with "key" and "value" keys). So the three malformatting test cases are covering this now.

Edit: Just saw that Skal already answered to this and with way more detail 😅 Thanks for that! ❤️

}

/** Never returns a value. */
Expand Down