[PATCH] Try to convert keys to json values prior to saving

[taylor.smock]

{
    "type": "FeatureCollection",
    "generator": "JOSM",
    "features": [
        {
            "type": "Feature",
            "properties": {
                "array": "[\"obj1\", \"obj2\"]",
                "dict": "{"\key1\": \"value1\"}"
            },
            "geometry": {
                "type": "Point",
                "coordinates": [
                    0,
                    0
                ]
            }
        }
    ]
}

vs.

{
    "type": "FeatureCollection",
    "generator": "JOSM",
    "features": [
        {
            "type": "Feature",
            "properties": {
                "array": ["obj1", "obj2"],
                "dict": {"key1": "value1"}
            },
            "geometry": {
                "type": "Point",
                "coordinates": [
                    0,
                    0
                ]
            }
        }
    ]
}

Possible issues: The value was actually supposed to be a string instead of an array/dict/etc.

[taylor.smock]

Try to use JsonValues when it makes sense (try to parse the tag value, falls back to JsonString of the value)

[taylor.smock]

Is there any interest for this patch?

[stoecker]

Can you please explain what this is about?

[taylor.smock]

It pretty much comes from opening an arbitrary JSON file for use in MapRoulette, deleting some tasks that are below a specific threshold (i.e., not on a major road), and then saving the JSON file again.

What happens is that every tag in JOSM is converted to a JsonString, no matter the initial representation (so {"integer": 0} becomes {"integer": "0"}).

I can upload some samples if that would help explain what I am talking about.
I've uploaded some samples. I did hand edit the original while the two outputs were edited in JOSM. The files were edited to reduce size.

[taylor.smock]

Current output when saving geojson files from JOSM

[taylor.smock]

Output (with patch)

[taylor.smock]

Original

[stoecker]

Hmm, I don't know much about that specific code and agree that your patch produces better output, but somehow I have the feeling that the error is in the JSON input and not the output?

I don't understand the idea behind the patch yet.

[taylor.smock]

I guess I'm doing a bad job explaining the idea behind the patch, but it is supposed to produce more correct output, instead of stringifying everything.

IIRC, I did look at modifying the input, but right now a Tag is a String -> String entry, which means we must lose data if we import GeoJSON's with the standard tagging system.

We could probably modify the tagging system to be a String -> Object entry system, but that would (significantly) increase code complexity elsewhere, and we would have to keep current methods as is (so get(key) -> String instead of get(key) -> Object) for backwards compatibility purposes.

I suppose we could make Tag an interface/superclass and then have:

• StringTag (maybe just keep this one as Tag?)
• NumberTag
• ArrayTag
• ObjectTag
• BooleanTag
• ...

And then modify the GeoJSONReader/Writer to instantiate/write the appropriate class for the appropriate entry.

[simon04]

Hmm, this raises plenty of questions… Your current patch would drop the quotation marks from key="value". Should oneway=yes (-1) be serialized to "oneway": true (-1). What about reading boolean values?

For little impact, we could restrict the feature to key={…} denoting a JSON object?

[taylor.smock]

Replying to simon04:

Hmm, this raises plenty of questions… Your current patch would drop the quotation marks from key="value". Should oneway=yes (-1) be serialized to "oneway": true (-1). What about reading boolean values?

I'd say, probably not. It would make sense for some boolean values, but the intent for this bug/patch was to change an input file as little as possible (in terms of content).

For little impact, we could restrict the feature to key={…} denoting a JSON object?

I've modified the reader to prepend/append { and } when JsonObject or JsonArray is encountered. I need to do some testing with JsonObject, since that was (apparently) thrown away previously. This means that the { and } need to be stripped on conversion. This will still change/modify number, boolean, and null. For those, we may need to create some subclasses of Tag specifically for them.

[taylor.smock]

Use { and } to indicate a json value (note: patch was hand edited since #19624 modifies the one of the files, patch -p0 < ${FILE} worked with it)

[taylor.smock]

Minimal extra tags, untested (essentially a skeleton, but probably 90% functional)

[simon04]

In 16936/josm:

fix #19632 - GeoJSONWriter: write key={value} as JSON object (patch by taylor.smock, modified)

[simon04]

I've extracted the essential change to GeoJSONWriter and added a unit test…

[taylor.smock]

Replying to simon04:

I've extracted the essential change to GeoJSONWriter and added a unit test…

That is fine (thanks for the added unit test).

That being said, is this actually going to work? (I need to test reading, then writing a geojson).

{ and } are used for dicts (which we are currently discarding), so I don't think there is a real difference between the previous state of code and the current state of code. (I was adding { and } around dicts and arrays specifically so that we could just look at the start/end character, and reduce possible issues where someone decides its a good idea to have name=[BracketName], although that won't help if they do name={CurlyBracketName}.)

[simon04]

Replying to taylor.smock:

That being said, is this actually going to work? (I need to test reading, then writing a geojson).

Should we drop the "The GeoJSON contains an object with property 'data' whose value has the unsupported type 'JsonObjectImpl'. That key-value pair is ignored!"?

src/org/openstreetmap/josm/io/GeoJSONReader.java

@@ -348 +348 @@
                     if (value instanceof JsonString) {
                         tags.put(stringJsonValueEntry.getKey(), ((JsonString) value).getString());
                     } else if (value instanceof JsonObject) {
-                        Logging.warn(
-                            "The GeoJSON contains an object with property '" + stringJsonValueEntry.getKey()
-                                + "' whose value has the unsupported type '" + value.getClass().getSimpleName()
-                                + "'. That key-value pair is ignored!"
-                        );
+                        tags.put(stringJsonValueEntry.getKey(), ((JsonObject) value).toString());
                     } else if (value.getValueType() != JsonValue.ValueType.NULL) {
                         tags.put(stringJsonValueEntry.getKey(), value.toString());
                     }

With this patch, the following works:

• Add OSM node with tag data={"foo": 1, "bar": "baz"}
• Save as GeoJSON
• Read GeoJSON

[taylor.smock]

Instead of saying "That being said, is this actually going to work? (I need to test reading, then writing a geojson)." I should have said "Will this work with arrays and dicts?" I'm pretty certain it isn't going to work with arrays.

Also, even shorter (since your proposed change is functionally similar to the following if statement):

src/org/openstreetmap/josm/io/GeoJSONReader.java

@@ -348 +348 @@
                     if (value instanceof JsonString) {
                         tags.put(stringJsonValueEntry.getKey(), ((JsonString) value).getString());
-                    } else if (value instanceof JsonObject) {
-                        Logging.warn(
-                            "The GeoJSON contains an object with property '" + stringJsonValueEntry.getKey()
-                                + "' whose value has the unsupported type '" + value.getClass().getSimpleName()
-                                + "'. That key-value pair is ignored!"
-                        );
                     } else if (value.getValueType() != JsonValue.ValueType.NULL) {
                         tags.put(stringJsonValueEntry.getKey(), value.toString());
                     }

TBH, I don't know why it is currently ignored.