[581] Enables better error handling for JSONDecodingError

[mustiikhalil]

The following PR provides a better error handling in the JSONDecodingError enum, where it would return part of the string as a part of the error Closes #581

[mustiikhalil]

Within the issue there is a suggestion to improve the following unrecognizedEnumValue my suggestion would be the following: case unrecognized(value: Any, for: Enum.Type) would that be a good way to approach it?

[thomasvl]

We can't remove the old cases as it will break someones code that was trying to catch that. Go ahead and tag it as deprecated with a note about the newer error.

@tbkka Since this is a rename of an error code, it is "breaking", but I'm guessing most folks just catch the errors generically, and don't inspect them. Are you ok with change change or would you rather hold it for a major version bump because of the api change?

[cburrows]

There are a number of cases in this enum that are named in the same way, in the style of malformedType. It seems odd to rename just two of them and only on them provide this diagnostic information.

I also clearly don't understand the language spec well enough to know why it permits you to duplicate the case name with associated data using tuple types that are compatible. It seems very strange. They look like functions with named parameters but in the grammar the associated data is clearly a tuple. I am not even sure how I would expect pattern matching to work against these new cases?

[Lukasa]

Tuple labels are part of the type. That is, (x: Int) and (y: Int) are distinct types. Some literals can be used to refer to either, but they are different types nonetheless.

Pattern matching would work like so:

switch error {
case .malformed(timestamp: let timestamp):
    break
case .malformed(duration: let duration):
    break

If you wanted to pattern match on just the case name, that is no longer possible, you'll need this:

case .malformed(timestamp: _):
    break
case .malformed(duration: _):
    break

This is definitely an API break. However, so is adding new fields, and you've also allowed that in the past (e.g. 3dec79b). How it's handled is kind of up to the project. NIO would forbid this change, if you wanted the context: we'd require a new error enum and move to throwing those.

[tbkka]

In its current form, I don't think this change is valuable enough to risk the breakage.

A more ambitious version of this would be appropriate for SwiftProtobuf 2.0. We don't have a timeline for that yet, but it would be good to start collecting rough drafts of changes we'd like to see there. Here's what I think a good JSON error handling would want to include:

• Field contents that caused the decode to fail.
• Name of the field that failed. Ideally, this would be the full path from the root.
• Tests to verify that correct errors were thrown in a variety of situations.
• Performance measurements to show that we were not sacrificing performance in the common case (no error).

I'm not sure whether it makes sense to combine the current enum cases or not. That's probably best driven by talking to people who are trying to use it and finding out what makes sense.

[tbkka]

@mustiikhalil -- I think for unrecognizedEnumValue it makes the most sense to just record a String with the raw field contents. That's an approach we could use consistently throughout the JSON error enums, it allows us to record invalid or unparseable field contents, and it's a better match for the JSON decoder internals (which are all character-based).

[tbkka]

On further reflection, I think there is a way to significantly expand the error reporting without breaking the API. It would require considerable care, though:

• Introduce a new set of JSON decoding interfaces that returned more comprehensive error information.
• Change the old JSON decoding APIs into wrappers that called the new API and translated the new errors into the old form.

That way, users of the old JSON decoding APIs would continue to see the same errors returned from those APIs; users could selectively choose to use the new APIs to get better error handling.

[mustiikhalil]

Good afternoon, Sorry for the late reply, had a lot of work to do. So your suggestion would be changing the implementation of decoding JSON. Wouldn't that mean that there would be a huge refactoring for the way the Decoders work? Since the JSONDecodeError, would be wrapping the newer JSONDecodeError. And also would mean that if we want to report an error where a failed value would be shown, we would require some refactoring to like: internal mutating func decodeJSON(from decoder: inout JSONDecoder) throws {} in extension Google_Protobuf_Struct: _CustomJSONCodable {}

[tbkka]

If you'd like to continue pursuing this, please rebase against the new main branch and re-submit. Thank you!

[thomasvl]

fyi - we're looking at doing a 2.0 for the next release, so now would be the time to do something that does require an api change if you'd like to pick this back up.

[tbkka]

It would be nice to rethink error handling as part of 2.0.

[thomasvl]

It would be nice to rethink error handling as part of 2.0.

Opened #1223 to track the general issues with errors. But expanding the enum to revive this would be fine (i.e. - new model for errors doesn't have to block reviving this).