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

jdrueckert · 2022-08-15T21:31:46Z

Contains

detect incorrect map format (object instead of array) early, log expected format
```
"mapName": [
  { "key": "...", "value": "..." }
]
```
add null check to avoid crashing if key "key" not found in case of incorrect map format (no map entry for "key")
add null check to avoid crashing if key "value" not found in case of incorrect map format (no map entry for "value")

How to test

Can be tested in combination with Terasology/PlantPack#20

Outstanding before merging

judge whether refactoring actually improves readability
check that refactoring doesn't break anything
- discuss the return value in case of mismatched key / value handler

- detect incorrect map format (object instead of array) early, log expected format ``` "mapName": [ { "key": "...", "value": "..." } ] ``` - add null check to avoid crashing if key "key" not found in case of incorrect map format (no map entry for "key") - add null check to avoid crashing if key "value" not found in case of incorrect map format (no map entry for "value")

jdrueckert · 2022-08-15T21:56:15Z

...y/src/main/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandler.java

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


@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 😅

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.

keturn

Please add a test for the case that was crashing.

...y/src/main/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandler.java

skaldarnar · 2022-08-18T15:23:09Z

The type handler interface documents that Optional.empty corresponds to a failed deserialization. It also has a couple of convenience methods to return null instead, or use a default value, or throw an exception DeserializationException.

Terasology/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/TypeHandler.java

Lines 39 to 45 in b367a18

    
               /** 
        
                * Deserializes a single value to the type {@link T}. 
        
                * 
        
                * @param data The persisted data to deserialize from. 
        
                * @return The deserialized value. {@link Optional#empty()} if the value could not be deserialized. 
        
                */ 
        
               public abstract Optional<T> deserialize(PersistedData data);

When we talked about this offline, I raised similar concerns and comments to what @keturn mentioned above. Half-parsed components or prefabs are most likely no good to anybody, so being more strict is likely a good choice. To avoid a situation where we change it for one thing but not the others I'd like to take a bit of time to figure out how we actually want this code to behave and look like, and then do a sweep over it to improve all (or most) places where we use type handlers.

In any case, I'd like to be able to assemble errors (and error message) bottom up to be able to present the user with a more meaningful error message on where they have to look to fix broken prefab.

Coming from functional programming in Scala and ReasonML I'm fond of using types to express logic. In this particular case, I'm looking at the Result type, c.f. its variant in Kotlin. I think the "more Java" way is to use plain exceptions...
In both cases, we should avoid logging way down in the stack (very similar to Log and Throw anti-pattern).

I believe for starters we can try to work with what we already have in the type handler interface, and gradually improve it (with a bit of focus on it for the next week(s)). Let's make this type handler more strict, and also apply it to others. Try to make better use of the information that something went wrong higher hup in the stack.

@keturn @jdrueckert what would be your take on this?

jdrueckert · 2022-08-21T09:50:55Z

Please add a test for the case that was crashing.

done ✔️

jdrueckert · 2022-08-21T10:02:54Z

I believe for starters we can try to work with what we already have in the type handler interface, and gradually improve it (with a bit of focus on it for the next week(s)). Let's make this type handler more strict, and also apply it to others. Try to make better use of the information that something went wrong higher hup in the stack.

@keturn @jdrueckert what would be your take on this?

Sounds fair to me. I adjusted the existing test case and added some more incl. a test case to test that a half-way valid map is still not half-way returned according to your "let's make this type handler more strict".
I'm also very much in favor to apply it to other type handlers as well as soon as possible.
I don't see it happening across all type handlers before the next playtest, but I'd like to get as far as possible with the capacity I have.

.idea/kotlinc.xml

skaldarnar · 2022-08-21T16:03:16Z

...y/src/main/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandler.java

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


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 + "'"; }

done ✔️

skaldarnar · 2022-08-21T16:03:43Z

...y/src/main/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandler.java

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


Suggested change

"Expected\n" +

" \"mapName\": [\n" +

" { \"key\": \"...\", \"value\": \"...\" }\n" +

" ]\n" +

"but found \n'{}'", data);

getUsageInfo(data));

done ✔️

skaldarnar · 2022-08-21T16:05:48Z

...c/test/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandlerTest.java

@@ -32,6 +31,39 @@ GenericMapTypeHandler.VALUE, new PersistedLong(TEST_VALUE)
            ))
    ));

+    private final PersistedData testDataMalformatted = new PersistedValueArray(List.of(


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.

done ✔️

😟 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.

I've replaced the triple-quotes.

requested changes added

javadoc does not have triple-quote code blocks.

keturn

Alright, lets go ahead with this.

I appreciate that you've covered a range of cases with the tests, but I'm curious: which case was it that you initially bumped in to and prompted this PR?

keturn · 2022-08-22T06:39:44Z

...c/test/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandlerTest.java

+    @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();


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.

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.

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! ❤️

skaldarnar

@keturn The issue we found was due to the prefab using the JSON object format to specify the mapping after switching the type to be SimpleUri.

{
  "plantpack:radish1": 3000,
  "plantpack:radish2": 2000
}

We currently have two variants for defining maps:

String -> V if the keys are Strings, value can be anything, handled by a different type handler

    {
      "plantpack:radish1": 3000,
      "plantpack:radish2": 2000
    }

K -> V where both keys and values can be arbitrary objects that can be de-/serialized
```
    [
      { "key": "plantpack:radish1": , "value": 3000 },
      { "key": "plantpack:radish2": , "value": 2000 },
    ]  
```
In the future, we probabyl want to add a third option (or replace/enhance the first one) if the key type K can be parsed from a plain String:
K -> V if the keys are Strings which can be parsed to some type K (e.g., all the URIs and Names), value can be anything, handled by a different type handler
```
    {
      "plantpack:radish1": 3000,
      "plantpack:radish2": 2000
    }  
```

...c/test/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandlerTest.java

Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

jdrueckert added 2 commits August 15, 2022 22:33

refactor: improve code readability

c1aba4c

jdrueckert requested review from keturn and skaldarnar August 15, 2022 21:31

github-actions bot added the Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. label Aug 15, 2022

jdrueckert commented Aug 15, 2022

View reviewed changes

This was referenced Aug 15, 2022

Crash on placing a prefab with ChangingBlock property Terasology/ChangingBlocks#14

Closed

fix: make ChangingBlocks case insensitive via BlockUri Terasology/ChangingBlocks#15

Merged

keturn suggested changes Aug 15, 2022

View reviewed changes

keturn reviewed Aug 15, 2022

View reviewed changes

...y/src/main/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandler.java Show resolved Hide resolved

test: add tests for partially and fully malformatted input

f5869a2

chore: remove unused import

cbb6a8c

skaldarnar previously requested changes Aug 21, 2022

View reviewed changes

jdrueckert added 2 commits August 21, 2022 19:13

chore: extract multiple times used map usage info into helper method

a92641b

doc: improve test case documentation

67bca4f

jdrueckert requested review from keturn and skaldarnar August 21, 2022 17:29

jdrueckert mentioned this pull request Aug 21, 2022

Potential bug in PersistedData.isArray() #5069

Open

jdrueckert added 4 commits August 21, 2022 23:29

chore: remove kotlinc.xml

4a8f40a

Merge branch 'develop' into fix/generic-map-typehandler-resilience

ba6b2be

Merge branch 'develop' into fix/generic-map-typehandler-resilience

fbe4e10

fix: add missing return

6e5ed50

keturn added 2 commits August 21, 2022 23:29

test(TypeHandlerLibrary): use javadoc syntax

1673268

javadoc does not have triple-quote code blocks.

chore(TypeHandlerLibrary): add toString for GenericMapTypeHandler

db97071

keturn previously approved these changes Aug 22, 2022

View reviewed changes

skaldarnar previously approved these changes Aug 22, 2022

View reviewed changes

...c/test/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandlerTest.java Outdated Show resolved Hide resolved

jdrueckert dismissed skaldarnar’s stale review via bb8d54e August 22, 2022 13:16

chore: remove superfluous '"'

bb8d54e

Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

jdrueckert dismissed keturn’s stale review via bb8d54e August 22, 2022 13:16

Merge branch 'develop' into fix/generic-map-typehandler-resilience

6f5a1b2

jdrueckert changed the title ~~fix: do not crash when facing unexpected map format~~ fix: do not crash on unexpected map format in GenericMapTypeHandler Aug 22, 2022

jdrueckert merged commit f3f9d92 into develop Aug 22, 2022

jdrueckert deleted the fix/generic-map-typehandler-resilience branch August 22, 2022 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jdrueckert commented Aug 15, 2022 •

edited

Loading

jdrueckert Aug 15, 2022 •

edited

Loading

keturn Aug 15, 2022

keturn left a comment

skaldarnar commented Aug 18, 2022

jdrueckert commented Aug 21, 2022

jdrueckert commented Aug 21, 2022

skaldarnar Aug 21, 2022

jdrueckert Aug 21, 2022

skaldarnar Aug 21, 2022

jdrueckert Aug 21, 2022

skaldarnar Aug 21, 2022

jdrueckert Aug 21, 2022

keturn Aug 21, 2022 •

edited

Loading

keturn Aug 22, 2022

keturn left a comment

keturn Aug 22, 2022

skaldarnar Aug 22, 2022

jdrueckert Aug 22, 2022 •

edited

Loading

skaldarnar left a comment •

edited

Loading

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

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

Conversation

jdrueckert commented Aug 15, 2022 • edited Loading

Contains

How to test

Outstanding before merging

jdrueckert Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keturn left a comment

Choose a reason for hiding this comment

skaldarnar commented Aug 18, 2022

jdrueckert commented Aug 21, 2022

jdrueckert commented Aug 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keturn Aug 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keturn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdrueckert Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

skaldarnar left a comment • edited Loading

Choose a reason for hiding this comment

jdrueckert commented Aug 15, 2022 •

edited

Loading

jdrueckert Aug 15, 2022 •

edited

Loading

keturn Aug 21, 2022 •

edited

Loading

jdrueckert Aug 22, 2022 •

edited

Loading

skaldarnar left a comment •

edited

Loading