Exception when deserializing a feature with nested object in properties (I think)

[HarelM]

I've created the following test for geojson4stj (I wanted to migrate from newtonsoft to STJ):

        [TestMethod]
        public void DeserializationFailure()
        {
            string str = @"
                 {
            ""type"" : ""Feature"",
            ""bbox"" : [
            35.0666687,
            32.5499986,
            35.0666687,
            32.5499986
                ],
            ""geometry"" : {
                ""type"" : ""Point"",
                ""coordinates"" : [
                35.0666687,
                32.5499986
                    ]
            },
            ""properties"" : {
                ""accuracy"" : ""minutes"",
                ""description"" : ""מעיין בסמוך לעץ אשל גדול \n"",
                ""description:he"" : ""מעיין בסמוך לעץ אשל גדול \n"",
                ""gns_ref"" : ""-780914"",
                ""name"" : ""עין ניצה"",
                ""name:en"" : ""Ein Nizza"",
                ""name:he"" : ""עין ניצה"",
                ""natural"" : ""spring"",
                ""source"" : ""GEOnet Names Server (gns)"",
                ""identifier"" : ""node_278470424"",
                ""poiLastModified"" : ""2021-10-08T18:06:37.0000000"",
                ""poiVersion"" : 5,
                ""poiSearchFactor"" : 1.0,
                ""poiIcon"" : ""icon-tint"",
                ""poiIconColor"" : ""blue"",
                ""poiCategory"" : ""Water"",
                ""poiSource"" : ""OSM"",
                ""poiLanguage"" : ""all"",
                ""poiContainer"" : false,
                ""poiNames"" : {
                    ""he"" : [
                    ""עין ניצה""
                        ],
                    ""en"" : [
                    ""Ein Nizza""
                        ],
                    ""all"" : [
                    ""עין ניצה""
                        ]
                },
                ""poiId"" : ""OSM_node_278470424"",
                ""poiGeolocation"" : {
                    ""lat"" : 32.5499986,
                    ""lon"" : 35.0666687
                },
                ""poiAlt"" : 172.0123568908041
            }
        }
                 ";
            var factory = new GeoJsonConverterFactory();
            var indentedOptions = new JsonSerializerOptions
            {
                DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
                WriteIndented = true
            };
            indentedOptions.Converters.Add(factory);

                 var f = JsonSerializer.Deserialize<Feature>(str, indentedOptions);
                 Assert.IsNotNull(f);
        }

This test is failing with an exception about unableing to convert from IFeature to Feature or something similar.
I think this was also a problem with the previous non STJ implementation, but I can't find the relevant issue :-(

I think the problem is with the nested object inside the properties, I haven't tried to narrow down the geojson feature.
Let me know if there's anything I can do to help out.

[airbreather]

Deserialize<Feature> should be Deserialize<IFeature>

[HarelM]

Thanks for the super fast response! But why can't I deserialize to a concrete object?
I changed my APIs from IFeature to Feature when I migrated from NTS 1 to 2, not sure I understand why the need to revert that change (or some if it).
Also non STJ code is able to pass that without any issues I believe.

[airbreather]

why can't I deserialize to a concrete object?

4STJ deserializes features to its own internal implementation of IFeature that also implements IUnique: RFC 7946 says that a GeoJSON Feature "SHOULD" include any commonly used identifiers as members of the Feature object itself (as opposed to having it be somewhere in its "properties"), and so this is a more faithful and clean way to implement that part of the spec than by stuffing it into its Attributes.

Also non STJ code is able to pass that without any issues I believe.

IIRC, the Newtonsoft.Json implementation has historically handled this by hooking into the feature's Attributes with a special attribute named "id". This creates an ambiguity, since for a feature like:

{
    "type": "Feature",
    "id": 1,
    "geometry": { "type": "Point", "coordinates": [0,0] },
    "properties": {
        "id": "e7b67869-899e-4fee-8ef4-905e4d7ef7d1",
        "name": "Something"
    }
}

only one of the two "id" values would make it onto the Attributes. So I pushed to have this done more faithfully for the new library, since there weren't any existing consumers to worry about breaking.

In theory, what we did for STJ can also be called ambiguous when it comes to writing out a concrete Feature object with an ID, but I think this is mitigated well enough in practice: not only do we look for a property with an unusual name by default:

NetTopologySuite.IO.GeoJSON/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/GeoJsonConverterFactory.cs

Lines 13 to 18 in 368733d

	/// <summary>
	/// The default name that, when seen on <see cref="IAttributesTable"/> or the "properties"
	/// object of a feature, indicates that it should "really" be the feature's "id", not stored
	/// in "properties" as-is.
	/// </summary>
	public static readonly string DefaultIdPropertyName = "_NetTopologySuite_id";

...but we also let you override the property that we look for, just in case you want to use that unusual name in the "properties" for some reason:

NetTopologySuite.IO.GeoJSON/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/GeoJsonConverterFactory.cs

Line 97 in 368733d

_idPropertyName = idPropertyName ?? DefaultIdPropertyName;

[airbreather]

See also: #44 (comment)

[HarelM]

Thanks for the info one again!
As a developer I would expect to get a better error message if that's the case, or at least have this documented in the readme of this repo.
I'll see if I can introduce the ID part although I already have a database of features that don't have the ID outside the properties field, I can try and go into that direction, but it's not an easy fix as these features are cached in the client and can't be easily replaced...
Also, in theory, I can get arbitrary "old" geojson files or requests, and this library needs to be able to support that.
I need to figure out how what you said here affects HTTP API calls that have geojson as part of the request/response...

[airbreather]

As a developer I would expect to get a better error message if that's the case

I think we can actually make this a lot better simply by removing our StjFeatureConverter.CanConvert override and change this block accordingly. The only reason I'm not doing this right now is because I would need to test to see whether or not that would mess with someone's ability to serialize something with a property of type Feature (not IFeature).

If it's REALLY important for us to be able to deserialize to the concrete Feature class, then we can explore some options, but I don't know if that's better than directing people to use IFeature instead.

at least have this documented in the readme of this repo.

The wiki has more complete usage samples for integrating with ASP.NET Core applications.

If you don't have a need for any of the ID-related parts, then the only thing you should have to do is make sure that you're deserializing to IFeature instead of Feature.

[HarelM]

Changing the following line, still doesn't allow deserializing to Feature:
var factory = new GeoJsonConverterFactory(new GeometryFactory(), false, "poiId");
poiId is where I store the element's ID inside the properties.
Deserializing to IFeature works, but this will require me to change substantial parts of the code I think.
Thanks for the link to the wiki, I would consider adding this link to the repo's readme for further reading.

[HarelM]

@airbreather do I need to change my code to use IFeature everywhere? What about the places where I create features and geometries, update attributes table etc? will combining Feature and StjFeature will work?
For example, this one:
https://github.com/IsraelHikingMap/Site/blob/0ed22f041ff402a125f5cb8bf327da93dd49ef99/IsraelHiking.API/Controllers/RoutingController.cs#L128

[airbreather]

When creating / writing features, there's no issue with using Feature. It's only when reading them from an external source that you need to make sure that you accept any instance of the IFeature interface.

[HarelM]

Thanks for the info and the hand holding.
I was able to migrate my code from Feature to IFeature and from AttributeTable to IAttributeTable, it was easier than I thought (mostly just search and replace).
The only place where I did see a change was the underlaying object holding an object in the properties.
i.e. I have properties: { names: { he: ["1", "2"], en: ["3","4"] } the names was converted to List<object> in the past and in STJ it was converted to object[].
In any case, I think this only thing that is needed here is a better exception message and some updates to the docs...

[HarelM]

BTW @airbreather, if I specify a field in the properties for the id (one of the factory parameters), will it be removed when serializing/deserializing with STJ?
I think this is still open according to a test I made, I'll see if I can reproduce it with a test and follow up here.

[mhosman]

Each time I try to serialize an object with a Geometry (GeoJSON) attribute, I get:

ex.Message ".NET number values such as positive and negative infinity cannot be written as valid JSON. To make it work when using 'JsonSerializer', consider specifying 'JsonNumberHandling.AllowNamedFloatingPointLiterals' (see https://docs.microsoft.com/dotnet/api/system.text.json.serialization.jsonnumberhandling)."

I'm using System.Text.Json and 4STJ and I added GeoJsonConverterFactory converter. I also tried adding AllowNamedFloatingPointLiterals but is still not working.