#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 )
{ "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)
Change History (20)
by , 5 years ago
Attachment: | 19632.patch added |
---|
comment:1 by , 5 years ago
Description: | modified (diff) |
---|
comment:4 by , 4 years ago
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.
by , 4 years ago
Attachment: | BuildingRoadIntersection.josm.current.geojson added |
---|
Current output when saving geojson files from JOSM
comment:5 by , 4 years ago
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 by , 4 years ago
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 asTag
?)NumberTag
ArrayTag
ObjectTag
BooleanTag
- ...
And then modify the GeoJSONReader/Writer to instantiate/write the appropriate class for the appropriate entry.
follow-up: 9 comment:7 by , 4 years ago
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 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:9 by , 4 years ago
Replying to simon04:
Hmm, this raises plenty of questions… Your current patch would drop the quotation marks from
key="value"
. Shouldoneway=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.
by , 4 years ago
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)
by , 4 years ago
Attachment: | 19632.extra_tag.patch added |
---|
Minimal extra tags, untested (essentially a skeleton, but probably 90% functional)
follow-up: 12 comment:11 by , 4 years ago
Milestone: | → 20.08 |
---|
I've extracted the essential change to GeoJSONWriter
and added a unit test…
follow-up: 13 comment:12 by , 4 years ago
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 by , 4 years ago
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) { 348 348 if (value instanceof JsonString) { 349 349 tags.put(stringJsonValueEntry.getKey(), ((JsonString) value).getString()); 350 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 ); 351 tags.put(stringJsonValueEntry.getKey(), ((JsonObject) value).toString()); 356 352 } else if (value.getValueType() != JsonValue.ValueType.NULL) { 357 353 tags.put(stringJsonValueEntry.getKey(), value.toString()); 358 354 }
With this patch, the following works:
- Add OSM node with tag
data={"foo": 1, "bar": "baz"}
- Save as GeoJSON
- Read GeoJSON
comment:14 by , 4 years ago
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) { 348 348 if (value instanceof JsonString) { 349 349 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 );356 350 } else if (value.getValueType() != JsonValue.ValueType.NULL) { 357 351 tags.put(stringJsonValueEntry.getKey(), value.toString()); 358 352 }
TBH, I don't know why it is currently ignored.
Try to use JsonValues when it makes sense (try to parse the tag value, falls back to JsonString of the value)