Modify

Opened 7 weeks ago

Closed 4 weeks ago

Last modified 4 weeks ago

#19632 closed enhancement (fixed)

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

Reported by: taylor.smock Owned by: simon04
Priority: normal Milestone: 20.08
Component: Core Version:
Keywords: Cc:

Description (last modified by 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.

Attachments (6)

19632.patch (1.7 KB) - added by taylor.smock 7 weeks ago.
Try to use JsonValues when it makes sense (try to parse the tag value, falls back to JsonString of the value)
BuildingRoadIntersection.josm.current.geojson (7.9 KB) - added by taylor.smock 4 weeks ago.
Current output when saving geojson files from JOSM
BuildingRoadIntersection.josm.patch.geojson (10.5 KB) - added by taylor.smock 4 weeks ago.
Output (with patch)
BuildingRoadIntersection.formatted.geojson (6.7 KB) - added by taylor.smock 4 weeks ago.
Original
19632.1.patch (4.3 KB) - added by taylor.smock 4 weeks ago.
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)
19632.extra_tag.patch (5.1 KB) - added by taylor.smock 4 weeks ago.
Minimal extra tags, untested (essentially a skeleton, but probably 90% functional)

Download all attachments as: .zip

Change History (20)

Changed 7 weeks ago by taylor.smock

Attachment: 19632.patch added

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

comment:1 Changed 7 weeks ago by taylor.smock

Description: modified (diff)

comment:2 Changed 4 weeks ago by taylor.smock

Is there any interest for this patch?

comment:3 Changed 4 weeks ago by stoecker

Can you please explain what this is about?

comment:4 Changed 4 weeks ago by 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.

Last edited 4 weeks ago by taylor.smock (previous) (diff)

Changed 4 weeks ago by taylor.smock

Current output when saving geojson files from JOSM

Changed 4 weeks ago by taylor.smock

Output (with patch)

Changed 4 weeks ago by taylor.smock

Original

comment:5 Changed 4 weeks ago by 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.

comment:6 Changed 4 weeks ago by 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.

comment:7 Changed 4 weeks ago by 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?

comment:8 Changed 4 weeks ago by simon04

Owner: changed from team to simon04
Status: newassigned

comment:9 in reply to:  7 Changed 4 weeks ago by 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.

Changed 4 weeks ago by taylor.smock

Attachment: 19632.1.patch added

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)

Changed 4 weeks ago by taylor.smock

Attachment: 19632.extra_tag.patch added

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

comment:10 Changed 4 weeks ago by simon04

Resolution: fixed
Status: assignedclosed

In 16936/josm:

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

comment:11 Changed 4 weeks ago by simon04

Milestone: 20.08

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

comment:12 in reply to:  11 ; Changed 4 weeks ago by 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}.)

comment:13 in reply to:  12 Changed 4 weeks ago by 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

    diff --git a/src/org/openstreetmap/josm/io/GeoJSONReader.java b/src/org/openstreetmap/josm/io/GeoJSONReader.java
    index e0e162e37..1e4f867a1 100644
    a b private static void parseNullGeometry(JsonObject feature) { 
    348348                    if (value instanceof JsonString) {
    349349                        tags.put(stringJsonValueEntry.getKey(), ((JsonString) value).getString());
    350350                    } else if (value instanceof JsonObject) {
    351                         Logging.warn(
    352                             "The GeoJSON contains an object with property '" + stringJsonValueEntry.getKey()
    353                                 + "' whose value has the unsupported type '" + value.getClass().getSimpleName()
    354                                 + "'. That key-value pair is ignored!"
    355                         );
     351                        tags.put(stringJsonValueEntry.getKey(), ((JsonObject) value).toString());
    356352                    } else if (value.getValueType() != JsonValue.ValueType.NULL) {
    357353                        tags.put(stringJsonValueEntry.getKey(), value.toString());
    358354                    }

With this patch, the following works:

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

comment:14 Changed 4 weeks ago by 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

    diff --git a/src/org/openstreetmap/josm/io/GeoJSONReader.java b/src/org/openstreetmap/josm/io/GeoJSONReader.java
    index e0e162e37..1e4f867a1 100644
    a b private static void parseNullGeometry(JsonObject feature) { 
    348348                    if (value instanceof JsonString) {
    349349                        tags.put(stringJsonValueEntry.getKey(), ((JsonString) value).getString());
    350                     } else if (value instanceof JsonObject) {
    351                         Logging.warn(
    352                             "The GeoJSON contains an object with property '" + stringJsonValueEntry.getKey()
    353                                 + "' whose value has the unsupported type '" + value.getClass().getSimpleName()
    354                                 + "'. That key-value pair is ignored!"
    355                         );
    356350                    } else if (value.getValueType() != JsonValue.ValueType.NULL) {
    357351                        tags.put(stringJsonValueEntry.getKey(), value.toString());
    358352                    }

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain simon04.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.