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

Jump to bottom

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 4 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
6 changes: 6 additions & 0 deletions .idea/kotlinc.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

43 changes: 31 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 @@ -61,26 +61,45 @@ 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" +
"Expected\n" +
" \"mapName\": [\n" +
" { \"key\": \"...\", \"value\": \"...\" }\n" +
" ]\n" +
"but found \n'{}'", data);
Copy link
Member

Choose a reason for hiding this comment

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

Used in multiple places, maybe create a helper method to be used here and below.

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

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 ✔️

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" +
"Expected\n" +
" \"mapName\": [\n" +
" { \"key\": \"...\", \"value\": \"...\" }\n" +
" ]\n" +
"but found \n'{}'", data);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Expected\n" +
" \"mapName\": [\n" +
" { \"key\": \"...\", \"value\": \"...\" }\n" +
" ]\n" +
"but found \n'{}'", data);
getUsageInfo(data));

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 ✔️

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);
Expand Down
78 changes: 75 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 @@ -12,7 +12,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 @@ -32,6 +31,39 @@ GenericMapTypeHandler.VALUE, new PersistedLong(TEST_VALUE)
))
));

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(
"testmap", new PersistedMap(Map.of(
TEST_KEY, new PersistedLong(TEST_VALUE)
))
))
));

private final PersistedData testDataMissingKeyEntry = new PersistedValueArray(List.of(
new PersistedMap(Map.of(
"not key", new PersistedString(TEST_KEY),
GenericMapTypeHandler.VALUE, new PersistedLong(TEST_VALUE)
))
));

private final PersistedData testDataMissingValueEntry = new PersistedValueArray(List.of(
new PersistedMap(Map.of(
GenericMapTypeHandler.KEY, new PersistedString(TEST_KEY),
"not value", new PersistedLong(TEST_VALUE)
))
));

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
void testDeserialize() {
var th = new GenericMapTypeHandler<>(
Expand All @@ -50,7 +82,7 @@ void testDeserializeWithMismatchedValueHandler() {
new UselessTypeHandler<>()
);

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

@Test
Expand All @@ -60,7 +92,47 @@ void testDeserializeWithMismatchedKeyHandler() {
new LongTypeHandler()
);

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

@Test
void testDeserializeWithObjectInsteadOfArray() {
var th = new GenericMapTypeHandler<>(
new StringTypeHandler(),
new LongTypeHandler()
);

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

@Test
void testDeserializeWithMissingKeyEntry() {
var th = new GenericMapTypeHandler<>(
new StringTypeHandler(),
new LongTypeHandler()
);

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

@Test
void testDeserializeWithMissingValueEntry() {
var th = new GenericMapTypeHandler<>(
new StringTypeHandler(),
new LongTypeHandler()
);

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

@Test
void testDeserializeWithValidAndInvalidEntries() {
var th = new GenericMapTypeHandler<>(
new StringTypeHandler(),
new LongTypeHandler()
);

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

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