Don't use Serializer context

[russcam]

Relates:elastic/elasticsearch-net#4791

This commit updates the AttributesTableConverter and FeatureConverter to not use the serializer context when deserializing AttributesTable. The serializer context is not safe to use multithreaded, which can be the case when JsonConverters from NetTopologySuite.IO.GeoJson are added to an singleton instance of JsonSerializer not under the control of NetTopologySuite.IO.GeoJson.

[CLAassistant]

All committers have signed the CLA.

[DGuidi]

@FObermaier have you written the JSON code or I'm wrong? can you check this?
I suspect there is problem in this code when a child "AttributesTable" object is used as a property of an "AttributesTable" parent object, but I'm pretty sure there is a test that check this and all tests are ok... so +1 for this PR for me.

[HarelM]

See RP #36 I think it has a test with a complex attribute table which was not merged...
I would advise adding another test to use an attribute table inside an attribute table just in case...

[DGuidi]

@HarelM ah ok, I supposed the test was merged actually I think a similar test that verifies the bug was merged. this is precisely the bug i'm talking about

[HarelM]

@DGuidi I couldn't find a test that serializes a table within a table, I might wrong but most test are deserializing and not serializing tests as far as I could see.
@russcam Can you add a test to your PR that serializes an attribute table that has a child attribute table?

[russcam]

@HarelM the changes in this PR are only to deserialization of Feature and AttributesTable, not to serialization.

[HarelM]

@russcam my bad, you are right.
In any case, I cloned your code and added the following test:

[GeoJsonIssueNumber(59)]
        [Test]
        public void TestGeoJsonWithNestedObjectsInProperties()
        {
            const string geojson =
                @"{
    ""type"": ""Feature"",
    ""geometry"": {
      ""type"": ""Point"",
      ""coordinates"": [1, 2]
    },
    ""properties"": {
      ""complex"": {
        ""a"": [""b"", ""c""],
        ""d"": [""e"", ""f""]
      }
    } 
  }
}";

            Feature f = null;
            Assert.That(() => f = new GeoJsonReader().Read<Feature>(geojson), Throws.Nothing);
            Assert.That(f, Is.Not.Null);
            Assert.That(f.Attributes["complex"], Is.InstanceOf<AttributesTable>());
            var innerTable = f.Attributes["complex"] as AttributesTable;
            Assert.That(innerTable["a"], Is.InstanceOf<List<object>>());
            Assert.That(innerTable["d"], Is.InstanceOf<List<object>>());
        }
    }

Which passed, so it seems that this commit does not break the reading of the properties.
@FObermaier @DGuidi @airbreather Can this be merged and a new package can be created?

[DGuidi]

probably I made some confusion, the but we talk about it isn't directly related to @HarelM problem as far as I remember.
threading issues are related to the fact that randomly some complex attributes, like the child attributes tables, are set to the wrong object.
Hard to test (random error in a multithreaded environment, probably the hardest bug to reproduce), but I think that if we can remove the context entirely from the json codebase, we can be pretty sure the thing is fixed.
let me check asap the code if there are any other usages of the Context property.

[russcam]

@DGuidi I searched for Context and couldn't find any other usages, but a second pair of eyes would be good.

I think removing the use of context is a good move as it'll ensure that the Json converters will work irrespective of how JsonSerializer is constructed, whether as a singleton or a new instance per usage.

[DGuidi]

@russcam give a check and I've found no other usages of the context property.
so +1 for me, but I feel more comfortable waiting for a feedback from @FObermaier and @airbreather that are actually the maintainers of the project.
thanks for your contribution.

[FObermaier]

Thanks, good catch.

[DGuidi]

thanks again for the contribution

[russcam]

No worries!