Homogenize exceptions in GeoJSON

[FObermaier]

• JsonReaderException for errors relating structure
• ParseException for invalid data/values

refers to #92

[FObermaier]

@atlefren, I know this has not been addressed for a long time. Nonetheless is this what you were asking for?

[FObermaier]

@airbreather, do you think we should adjust Exceptions in GeoJSON4STJ, too?

[atlefren]

@FObermaier Sweet! I had almost forgotten about this, but if I read this correctly I will now be able to scrap a lot of ugly code trying to deal with the case I described!

Thank you!

[airbreather]

@airbreather, do you think we should adjust Exceptions in GeoJSON4STJ, too?

First, some background, looking at the exception types that we throw in GeoJSON4STJ:

1. ArgumentException, InvalidOperationException, and NotSupportedException should be used for programming errors (e.g., user tried to modify a read-only object): someone made a mistake, and so the only appropriate things to do are to either fix the mistake (if the mistake was made in code that you own), or (if you don't own the code that made the mistake) log a bug and try really hard to avoid that code path in the meantime.
   • If you don't own the code that made the mistake, then the best-case scenario is if the "mistake" was just that we threw one of these exception types instead of some other exception type, because then at least you can catch the exception and handle it like you would handle the exception that we "should" have thrown.
2. NotImplementedException should almost never be used in production code. In fact, if it ever actually gets thrown, then the line that threw it should always be changed to do something else instead.
   • I believe that our one usage is correct. C# doesn't have compiler-verifiable discriminated unions, so even if the developers of System.Text.Json promised that these would be the only three subclasses that JsonNode could ever have, this line would still need to do something to satisfy the compiler's rules.
   • Therefore, if this ever actually gets thrown, then we're being called with a later version of the library that has something more than the three subclasses that exist today, and so there's a change that we can and must make.
3. JsonException is supposed to mean that everything was working out just fine up until the system tried to interpret the stream of bytes as a JSON document that models some higher-level concept.

I've looked through the 29 lines under src/NetTopologySuite.IO.GeoJSON4STJ that include "throw new" with the above items in mind, and here's what I've come up with (first one should probably be changed, second one is fine either way, everything else is informational to explain why I don't want to change anything else):

1. This NotSupportedException should probably be changed to JsonException, because our rules for what can go into a "Feature"'s "id" depend only on the contents of the JSON tokens that we see, nothing that the caller can do.
   • In contrast, I think that this one is fine as NotSupportedException. If the caller needs something that doesn't fit into decimal, then they can use TryDeserializeJsonObject<T> or TryGetJsonObjectPropertyValue<T>.
2. This NotSupportedException in StjGeometryConverter.cs should be JsonException, but it's not a big deal either way because we will always throw the JsonException for EX_CoordinatesIncompatibleWithType in all cases that would otherwise hit this line today. Tested by trying to deserialize { "type": "123456" } as a Geometry.
3. This NotSupportedException could probably technically be a NotImplementedException by the same logic that I used to justify the one place where we currently throw that type of exception, but I think it's not as much of a stretch to imagine a new value being added in the future to discriminate between different types of tokens that would currently be String (even though I can't see the team actually making such a big behavior change), so I prefer it as-is.
4. I don't want to change any JsonException to ParseException in here. JsonException already correctly covers format issues, and so any user code that currently handles JsonException should not be forced to change.
5. All InvalidOperationException in StjParsedCoordinates.cs are correct.

[airbreather]

I made the previous comment very long because all of my GitHub notification e-mails have been getting sent to spam for several months now, so I'm extra slow to respond on top of my ordinary slowness, so I wanted to get all the information out there at once just in case I miss things for a really long time again.

[airbreather]

I've looked through the 29 lines under src/NetTopologySuite.IO.GeoJSON4STJ that include "throw new" with the above items in mind, and here's what I've come up with (first one should probably be changed, second one is fine either way, everything else is informational to explain why I don't want to change anything else):

For the two that I suggested should be fixed, I have opened #134 to fix them by themselves. No need to hold this PR over it, I think this can be merged.

I made the previous comment very long because all of my GitHub notification e-mails have been getting sent to spam for several months now

Repeated clicks of the "this is not spam" button have not resolved this, so I've found a way to add some manual filter to make sure that this stops happening. I should now be back to just my usual long periods of radio silence, instead of the more advanced silence I've been exhibiting over the past several months...

[FObermaier]

@airbreather , I think the within the NewtonSoft.Json based project we should replace ParseException with JsonReaderExceptions. I will do that later and do a merge afterwards.

Should we release a v4 or will v3.1 do?

[airbreather]

Should we release a v4 or will v3.1 do?

Per SemVer, it should really be a v4. Callers that catch a certain type of exception today will need to change to catch a different type of exception in order to be correct.

When in doubt, I check the rules on this page: if it's not a sort of change that the .NET development team could accept into the core libraries with their strict compatibility requirements, then it's a breaking change for us. This time, the "Exceptions" section confirms it.