search

NetTopologySuite / NetTopologySuite.Features

An implementation of Feature and FeatureCollection
BSD 3-Clause "New" or "Revised" License
7 stars 14 forks source link

2.0 breaking change in AttributesTable ctor #10

Closed stijnherreman closed 4 years ago

stijnherreman commented 4 years ago

In 1.x AttributesTable had a IDictionary<string, object>/HashTable constructor. In 2.x it was replaced by a Dictionary<string, object> constructor.

This change causes IDictionary<string, object> to use the IEnumerable<KeyValuePair<string, object>> constructor instead, so the Comparer of the IDictionary instance is lost. In my case, this is a comparer that ignores casing.

I've tried changing the constructor to accept an IDictionary again, but this breaks the Dictionary<string, object>.Enumerator GetEnumerator() method. That enumerator has a Current property, so changing it would also be a breaking change.

It's probably too late at this point to try and fix this, so perhaps the best thing to do here is document this change, what do you think?

airbreather commented 4 years ago

the Comparer of the IDictionary instance

IDictionary<TKey, TValue> does not have an IEqualityComparer<TKey> Comparer { get; }.

Dictionary<TKey, TValue> does, and there is a constructor that lets you give us the exact instance of Dictionary<string, object> to use, so if you pass in one of those, then we should use its comparer.

I've tried changing the constructor to accept an IDictionary again It's probably too late at this point to try and fix this

It kind-of is, but do you actually have a custom implementation of IDictionary<string, object>, and does it handle serialization / deserialization properly? There's a lot of hidden complexity in the world where we let you inject any old IDictionary<string, object> into AttributesTable, and my judgment was that:

  1. This complexity is unwarranted: probably the overwhelming majority of callers would be perfectly fine with Dictionary<string, object>, and for the rare folks with custom IDictionary<string, object> implementations, they can still make a custom IAttributesTable implementation that may work even better than this.
  2. Given that, the stability of using a built-in type for this serializable member seemed very attractive: the worst-case scenario when copying an instance of AttributesTable from one version of an application to another is if the Comparer that you inject can't be deserialized. And even there, probably the overwhelming majority of people who use a non-default Comparer would probably just give a StringComparer anyway (e.g., StringComparer.OrdinalIgnoreCase like it sounds like you're doing) which should serialize just fine.

perhaps the best thing to do here is document this change, what do you think?

I don't see anything wrong with that. I've added a section to the NTS wiki page for 1.x-to-2.0 upgrades that deals with the breaking changes in this package.

stijnherreman commented 4 years ago

IDictionary<TKey, TValue> does not have an IEqualityComparer<TKey> Comparer { get; }.

I missed this while debugging, it's actually a SortedList<TKey,TValue> exposed as an IDictionary<TKey, TValue> (with StringComparer.OrdinalIgnoreCase as you guessed). I think we can get away with converting it to a Dictionary, and if not I'll write a custom IAttributesTable implementation as per your suggestion.

Thank you for your input and for expanding the documentation.

airbreather commented 4 years ago

SortedList<TKey,TValue> exposed as an IDictionary<TKey, TValue>

Hmm, I should have considered that kind of case before making my reply. Sorry about that.

One thing that we could do is allow callers to inject the IEqualityComparer<string> directly. This doesn't really add any of that "hidden complexity" that I had mentioned before, since it's actually one of the few pieces of "hidden complexity" that this model keeps around.

stijnherreman commented 4 years ago

No problem at all 😄

I'm dealing with conversions of types (related to https://github.com/NetTopologySuite/NetTopologySuite.IO.SqlServerBytes/issues/10) and the source data is a Feature implementation in an in-house library.

We convert from the in-house Feature to NTS and back again (with some operations in-between), and after another review I found that I can simply use new Dictionary<string, object>(source.Attributes, StringComparer.OrdinalIgnoreCase) with NTS as-is, without causing unintended side effects. Or we can make use of the commit you just pushed, that'll work too.