JSON storage BigDecimal scaling regression

[wborn]

After updating to openHAB 2.1.0 I encounter again BigDecimal scaling issues. I.e. longs are stored as decimals in the jsondb files. Because of this the LIFX binding (and other bindings) revert to default values and log warnings like:

23:04:31.744 [WARN ] [inding.lifx.handler.LifxLightHandler] - Invalid value '300.0' for transition time, using default instead.
23:04:31.790 [WARN ] [inding.lifx.handler.LifxLightHandler] - Invalid value '300.0' for transition time, using default instead.
23:04:31.829 [WARN ] [inding.lifx.handler.LifxLightHandler] - Invalid value '0.0' for transition time, using default instead.
23:04:31.870 [WARN ] [inding.lifx.handler.LifxLightHandler] - Invalid value '300.0' for transition time, using default instead.
23:04:31.991 [WARN ] [inding.lifx.handler.LifxLightHandler] - Invalid value '300.0' for transition time, using default instead.

In org.eclipse.smarthome.core.thing.Thing.json the parameters show up like:

          "fadetime": 300.0

I'm using:

openhab> bundle:list|grep Json
136 | Active   |  80 | 0.9.0.b5               | Eclipse SmartHome Json Storage Service

@cdjackson originally fixed this with PR #2685.

[sjsf]

I just checked the Json DB implementation - the mentioned workaround indeed got lost with #3225.

The database implementation itself actually handles precisions "correctly", i.e. it does not add the trailing .0 by itself. However, if it got in there (for whatever reasons), it keeps them.

The config normalization also ensures that INTEGER configuration values are converted to a a BigDecimal with 0 scale. So while I'd love to find out how these values originally get into the database, it still can't hurt to apply the same workaround again.

I will create a PR to make it more robust in this area.

[sjsf]

Oh, that's indeed a very ugly turn which I missed. Interesting... Very good analysis!

I looks like so far the put(...) actually did not serialize the object, but left it in the map as a plain java object. As a result, the "value" object gets serialized (and later deserialized) together with the "meta object" which is a Map. And by doing so, our humble StringObjectMapDeserializer delegates to Object type and therefore won't be considered for inner maps by Gson anymore. This is how some doubles/floats get in, which lead to the trailing .0. Therefore it makes a difference whether you request an object from the cache (i.e. what you had put in before) or you get it deserialized from the file. 😞

The proper fix of course would be to serialize the object to a strong before we put it into the outer map. However, that would lead to all objects being persisted as strings which include escaped json. Absolutely unreadable. That would contradict the original idea of the JsonDB: having a persistence format which is easily human-readable. Also it would render all existing JsonDBs useless.

Therefore I'll try to fix the StringObjectMapDeserializer in a more convenient way.

[sjsf]

I have updated the PR. As thanks to your help we now found the root-cause, I removed the workaround again for now.

I'd leave it open for discussion whether we should prefer rather stability (i.e. read the same what you wrote before) or automated error correction (i.e. cut off trailing .0 in case they got written into it).

@wborn @cdjackson @maggu2810 WDYT?

[wborn]

Thanks for having another look at the code @SJKA. After doing some more testing I again run into deserialization issues with the updated PR code. My "300" values again get deserialized into "300.0".

I've tested a new OH 2.2.0-SNAPSHOT, empty initial database and updated the Json Storage bundle so it contains the PR code.

I can actually reproduce this in the new unit tests by making the DummyObject a bit more complex so it looks more like the actual ThingImpls that are stored in the database. The following DummyObject makes testStableOutput() fail:

    private class DummyObject {

        public Map<String, Object> myMap = new HashMap<String, Object>();

        public List<Object> channels = new ArrayList<>();

        public DummyObject() {
            myMap.put("testShort", Short.valueOf("12"));
            myMap.put("testInt", Integer.valueOf("12"));
            myMap.put("testLong", Long.valueOf("12"));
            myMap.put("testDouble", Double.valueOf("12.12"));
            myMap.put("testFloat", Float.valueOf("12.12"));
            myMap.put("testBigDecimal", new BigDecimal(12));
            myMap.put("testBoolean", true);

            myMap.put("testString", "hello world");

            Map<String, Object> childMap = new HashMap<String, Object>();
            childMap.put("testChildLong", Long.valueOf("12"));
            channels.add(childMap);
        }
    }

The test fails because storageString2 then contains "testChildLong": 12.0.