Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why this coupling to the serializer? #15

Open
SebastianStehle opened this issue Jan 28, 2022 · 9 comments
Open

Why this coupling to the serializer? #15

SebastianStehle opened this issue Jan 28, 2022 · 9 comments

Comments

@SebastianStehle
Copy link

I am already using GeoJSON.NET and I am planning a migration to System.Text.JSON. But I also serialize to other formats, e.g. Mongo-BSON.

What I don't understand is the coupling to the serializer. The fields with GeoJSON objects are part of my domain and I think it would be great to have it decoupled from the serializer. GeoJSON is so widely used that it is almost the standard for all representation of geo objects.

@matt-lethargic
Copy link
Member

The initial motivation for this project was always to facilitate serialization and deserialization of GeoJSON using Newtonsoft.Json, hence the name! It was never built to have the objects incorporated inside of a domain model.

If we imagine that we were to split out our model to be "shared", we would then have to reference both Newtonsoft and System.Text in those models as we use property attribute decoration to assign converters etc.

Do you have any suggestions how we could achieve both the goals that you talk of and our goals to support Newton and System.Text? We didn't like the solution of model duplication between both of the packages, but it was the simplest and cleanest solution at the time.

@SebastianStehle
Copy link
Author

I thought the initial motiviation was to provide a GeoJSON for .NET because of the name ;).

If we imagine that we were to split out our model to be "shared", we would then have to reference both Newtonsoft and System.Text in those models as we use property attribute decoration to assign converters etc.

  • Some attributes are universal, e.g. DataMemberAttribute should work for both.
  • Most other converters can be also registered in the serializer, e.g. with an extension method.

I think it should not too difficult.

@MartinCarpentier
Copy link
Contributor

Sadly it's not possible. Lets talk about why.

Lets answer your points.

  • Most attributes are not common
  • Converters do not follow the same logic

GeoJson.NET and GeoJson.Texts job is to give a developer a set of classes, which can automatically be converted to valid geojson per the specification. This is not possible directly by using default serialization logic, and requires this package to configure and work directly with the serializer.

Since the 2 serializers, newtonsoft and system.text, do not follow the same logic, or allow the same things, different logic have to be implemented when communicating with both.
So what can be done?

We could implement both in one god nuget package, which would require all using projects to both reference system.text and also newtonsoft, it would also require a lot of attributes from both NewtonSoft and System.Text on all the classes, since they don't use the same attribute sets. This would be something like what you suggest.
We could split it into 3 packages. a core package, 1 for newtonsoft, and 1 for system.text. This would create a lot of complexity for what is in essence a very small solution. Furthermore, it would still require users to select either GeoJson.Text or GeoJson.Net when adding the nuget package.
Or we could create an entirely new package for GeoJson.Text which would not be bound by the old code, dependencies on very old .NET versions like GeoJson.Net does, and would be entirely based on System.Text.Json.

We chose the last solution.

If you can find a better solution that enables both serializers to work together, without forcing users to have references to both serializers, then you're very welcome to implement it, and send a pull request.

@SebastianStehle
Copy link
Author

It is definitely possible. You could convert everything manually of course, but I understand that it is a question about how the goals are defined.

But NetTopologySuite does exactly that: https://github.com/NetTopologySuite/NetTopologySuite.IO.GeoJSON/blob/develop/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/GeoJsonConverterFactory.cs

@MartinCarpentier
Copy link
Contributor

Even the link you just send requires a dependency to the serializer.
As stated in their readme.

GeoJSON4STJ Usage
This is the package for System.Text.Json serialization and deserialization

@MartinCarpentier
Copy link
Contributor

Furthermore, you do realise that the geojson is the result of the serialization and not the c# objects?

@SebastianStehle
Copy link
Author

Of course you have a dependency to the serializer, but it depends where you have them. Perhaps it depends on the understanding of this library. For me it was always something that I would add to the domain layer, because GeoJSON is so widely used. But if this library is a transport model for you the coupling as it is right now, makes sense.

If you have a model that is agnostic of the serializer you can build extensions to support different serialization methods. GeoJSON is also used by database for example such as MongoDB.

It is the same approach that is also used by other libraries such as NodaTime: https://github.com/nodatime/nodatime.serialization/tree/main/src

@MartinCarpentier
Copy link
Contributor

Ok, so if i understand you correctly, what you would have liked was a set of Gis domain objects that could easily be serialized using different serializers including a geojson representation.
The idea of this project is not as such, and is bound 100% to geojson, and therefore also a json serializer.

@matt-lethargic
Copy link
Member

Can / should we close this issue now? Seems like a happy understanding has come to pass!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants