Support number type for "Feature.Id" property

[dalbani]

As per the RFC:

3.2.  Feature Object
   o  If a Feature has a commonly used identifier, that identifier
      SHOULD be included as a member of the Feature object with the name
      "id", and the value of this member is either a JSON string or
      number.

Would it possible for GeoJSON.Net to support a number value for Feature.Id, in addition to a string value?

[matt-lethargic]

Do you have any good ideas or even a pull request with this in? Personally I think that leaving as a string gives the most options to everyone as you can easily put a number into a string but not the other way around.

[dalbani]

It's indeed possible to somewhat "stringify" a number, but that's not very reliable — and breaks for example the identity === operator in JavaScript.

As for a solution in GeoJSON.Net, I don't really know, I'm no C# expert to be honest.
A discussion on StackOverflow on this topic mentions 2 solutions, I quote:

• Make your property type object (or dynamic if you want to get even worse design) and transform values within the setter as you stated in the question - i strongly recommend to avoid this approach.
• Get away from property concept and create separate methods to get value of the field and assign from different types. This approach will allow you to assign value if you dont know the type at compile-time while getter-method will be typed still correctly. But generally it still looks like bad design.

Or would you see another solution?

[matt-lethargic]

So being that c# is a strongly typed language it wouldn't have the issues of typing like you get in Javascript so the === operator issue isn't a problem.

I would suggest that the consumer of GoeJSON.Net could cast or parse the Id property to an int if needed using one of the many int methods

int.TryParse(feature.Id, out featureId);

or

var featureId = Convert.ToInt32(feature.Id);

or

var featureId = Int32.Parse(feature.Id);

This is what I'd have to do under the hood anyway.
You could also create an extension method

public static class FeatureExtensions
{
    public static int NumberId(this Net.Feature.Feature feature)
    {
        int featureId = 0;
        if (int.TryParse(feature.Id, out featureId))
        {
            return featureId;
        }

        throw new ArgumentException("Feature Id is not an integer.");
    }
}

[dalbani]

Indeed, when you have control over the consumer of GeoJSON.Net, you can manually (try to) convert the string ID to a number ID.
But the use I had in mind was to use the built-in JSON serialization functionality, for an HTTP response for example.
In this case, feature IDs will always be returned as string — even in the case that it was originally parsed from a number ID!

See what another C++ library does for the feature ID: https://github.com/mapbox/geometry.hpp/blob/master/include/mapbox/geometry/feature.hpp#L47.

using identifier = mapbox::util::variant<uint64_t, int64_t, double, std::string>;

Is it out of the question to have the Id property as object then, and implement a check on string or number value?

[xfischer]

Hi @dalbani and @matt-lethargic !

Could it be possible to say that

• Id is an object property or whatever IEquatable thing
• The writeJson could handle if it is convertible to a numeric type and then serialize it as a numeric type ?

The problem here is that now Id MUST be a string, even if it represents a number.
It could be changed to object, and we tell NewtonSoft.Json to serialize it the way it should.

[matt-lethargic]

Hey guys, had some thinking time.

C++ supports variant types which much better than C# by the seems of it.

We can easily serialize to a number value if the Id has been set to one. I've created a banch 'Issue100' that addresses this. I'm not keen on setting the Id to type 'object' as it can be set to anything not just a number or a string.

I am concerned about putting this straight in as it could be classed as a breaking change so would need a version bump. @xfischer any thoughts on that?

[matt-lethargic]

.... I'm happy to put this into version 2.x though

[dalbani]

Thanks a lot @matt-lethargic for the time spent in writing the code of the pull request.
But I'm not so sure users of the library will find it logical to get a number back as serialized value when they put a string in.
To get back to my original proposal: how problematic would it be to turn the Id property into an object, as @xfischer also mentioned?
With of course a check at runtime that the valued passed is either a string or a number.

[xfischer]

@matt-lethargic's PR is neat and works. OK with @dalbani for the downside of getting back a string.

I wish I had more time to test the Issue100 branch, but I don't !
I don't know enough pure GeoJSON.Net lib (I'm more in the spatial contribs) but for sure it's a breaking change. (Imagine someone uses string manipulation on the Id property in its code (like Id.Length), changing Id to object would surely destroy (at least) the planet.