Move new errors to extendable struct-based SwiftProtobufError

[gjcairo]

This is part of a wider effort to make breaking changes introduced in main non-breaking.

Adding or removing cases to a public enum is an API-breaking change, and we've done both on main.

To get around this, it's common practice to have a struct-based Error type instead. A useful pattern is to have this error contain a Code (which defines the domain of the error), and a message explaining the error in more detail. This message can be a dynamic string, meaning it can contain additional information about the error, for example, the name of an unknown JSON field when decoding.

This PR adds a new SwiftProtobufError, which follows the above pattern.

Common errors are defined as public static lets, which means new cases can be added without breaking API. Errors can also be defined without the need for exposing a new constant - I have simply moved all of the existing errors (grouped by domain) to static constants for consistency, but this doesn't have to stay this way in the future.

Existing (i.e. already released) enums have been left as-is to avoid any potential source-breaking changes where users are catching specific errors. New errors have been moved to SwiftProtobufErrors - and so should any new errors moving forward. Docs have also been updated.

[thomasvl]

This feels like a layering violation. This is for the plugin library, only a plugin would ever need these errors, so it doesn't seem like it should be in the core library.

[gjcairo]

You're right. I've changed this around a bit and got rid of anyUnpackError and protoFileToModuleMappingError an replaced them with invalidArgument and internalError. I think they're better as general error domains. What do you think?

[gjcairo]

Made this change

[thomasvl]

Sorry, missed this reply -

anyUnpackError is likely still ok, as unpacking any is still within Source/SwiftProtobuf, it's the modulemapping that is only within the scope of another package, which is what seemed odd to me. For the modulemapping, why not have a PluginLibError? i.e. - have one error scope per package as you go up the stack?

[gjcairo]

No problem. I ended up removing anyUnpackError because:

• for malformedWellKnownTypeJSON error I thought it made more sense to use a jsonDecodingError code
• for malformedAnyField, internalError seemed like a good fit, since it's unrecoverable and we don't really know what caused the unpacking to fail
• for typeMismatch, invalidArgument seems to be descriptive enough since the mismatch comes from the provided type not matching the actual type.

We could have an anyUnpackError still but I felt like as a domain it was a bit too specific maybe and could be described as well with other codes that capture larger domains. However this is all very subjective, I'm happy to switch this back if you think it would be useful for callers to catch unpacking errors specifically - I think from your time working on protobuf you have more experience on user use cases than me :)

As for having an error code per package, I personally think that would be a bit too generic: we'd be "forced" (for consistency) to have all errors thrown from (following your example) the plugin throw PluginLibError, even if there was a better error code for that specific case. But again, all very subjective.

[thomasvl]

Might not need this is everything moves to not making the error directly.

[FranzBusch]

We really shouldn't use a single code for more than one error otherwise users can't match on the code. This means introducing a ton more cases in the underlying code enum but giving developers back better error matching.

Also all these let's should probably be funcs if we want to pass through the file/line/column since these need to be created at the place where the error is created and not here where it is statically defined

[gjcairo]

Ah, I was using NIOFileSystem/FileSystemError as inspiration, where we only have a few Codes that group the error types in different categories, and then throw errors with different messages but the appropriate code.

Not sure I follow why users wouldn't be able to match on the code. This is possible:

do {
  try someOperationThatThrows()
} catch let error as SwiftProtobufError where error.code == .binaryEncodingError {
  // Do something
}

And so is this:

do {
  try someOperationThatThrows()
} catch let error as SwiftProtobufError {
  switch error.code {
  case .binaryEncodingError:
    // Do something
  case ...
  }
}

Regarding these having to be funcs though, you're right, I was unsure about keeping the SourceLocation and did a refactor and I'm not really using it anyways but didn't properly clean up, my bad. I see there's another thread above discussing whether we want to keep it or not. If we do, I'll convert all of these into funcs and add the location to the message. Otherwise I'll just get rid of it and keep these as lets.

[gjcairo]

FYI @thomasvl just rebased on main and fixed a conflict after your changes on the JSONDecoder, that's what the force push is about.

[thomasvl]

FYI @thomasvl just rebased on main and fixed a conflict after your changes on the JSONDecoder, that's what the force push is about.

ack.

@FranzBusch hows this looking from your POV.

@tbkka @Lukasa any thoughts/comments on this?

[FranzBusch]

This looks good to me know! I like the name spacing that you introduced @gjcairo

[thomasvl]

I was going back through some of our internal code, and realized this new model for errors has a limitation when it comes to the general idea of details about an error. So in the enum form, the typeURL was an associated value, so a developer can write code to extract what the type was and then act on it (log it, attempt to do something else about it, etc.)

In this new form, there is no way a developer can write code to actually get at this detail to do anything with it. If they describe the error all they get is the Code. So if someone wanted some degree of logging for how an application does in the field, how would they get these sorta information? Snd I don't think we want the message expose for the previously mentioned reasons, as then folks will parse it making the string contents part of the api contract.

Are we headed down a path where we need something like NSError's userInfo to do this completely generically to avoid future api issues? 😞

[thomasvl]

Quick follow up - I'm thinking cases like where say you have a server accepting connection, being able to get specific details about all the different decoding failures that are happening may help you better understand what if someone is trying to hack into your server vs. say just doing a DOS attack, etc. So I'm starting to wonder if these "domains" are just too limited of info to provide something detailed enough to build monitoring on top of.

[gjcairo]

If users wanted to log the typeURL or any other information associated with the error, they can just get the error description and the message will be a part of it (which now makes me think it may make having made message internal moot, but let's stay focused for a minute :)).
So let's assume that the "something else" users want to do with these associated values is try to recover from the error in some way, and that they need more information to do so. It could be argued that maybe throwing errors to drive the program's logic isn't necessarily the best pattern. Using this anyTypeURLNotRegistered as an example: if it was a recoverable issue, you might want to add API to allow users to set a closure/have a delegate to handle "unregistered URLs" for instance.

However, I do understand the concern and I think it's still valid (seeing how changing the APIs in a more significant way would either be breaking or definitely out of scope).

So thinking of solutions that would work for this... Having struct-based errors does allow for associated values, but it also requires having a more specific wrapped private enum instead of this more generic "domain" approach. We could add more specific error codes as a compromise for errors that definitely require associated values.
For instance, we could have something like:

struct SwiftProtobufError: Error {
  private enum _Error {
    case malformedAnyField
    case anyTypeURLNotRegistered(typeURL: String)
    // There would obviously be more cases here, just writing these two for simplicity
  }

  private let _wrapped: _Error
  private init(_ wrapped: _Error) {
    self._wrapped = wrapped
  }

  public static let malformedAnyField = SwiftProtobufError(.malformedAnyField)

  public static func anyTypeURLNotRegistered(typeURL: String) -> SwiftProtobufError {
    SwiftProtobufError(.anyTypeURLNotRegistered(typeURL: typeURL))
  }

  public var unregisteredTypeURL: String? {
    switch self._wrapped {
    case .malformedAnyField:
      return nil
    case .anyTypeURLNotRegistered(let typeURL):
      return typeURL
    }
  }
}

Callers could then match the error and extract the associated value like this:

do {
  try somethingThatThrows()
} catch let error as SwiftProtobufError {
  if let unregisteredTypeURL = error.unregisteredTypeURL {
    // This error was thrown because there was an issue with an unregistered type URL:
    // do something with it.
  }
}

I know this isn't as ideal, but I think it's a good compromise. We've reached this current iteration of errors as structs after quite a lot of experimentation on our other libraries, and we think it's the best we can currently do.

What do you think? Would you want to have a getter for all associated values that were part of some error in the old model?

[thomasvl]

This seems promising. I don't have any experience with the struct-based errors (aside: it seems like Swift as a language needs something to deal with error/associated values/non-breaking apis with new errors); so I'll defer to other on how to best model this. The loss of functionality/visibility only being the description/message does feel wrong for all the reasons we'd mentioned. So hopefully there is a right balance to strike for something like this.

[gjcairo]

So it seems we'd only need two cases with associated values (and thus two getters): anyTypeURLNotRegistered and unknownField. Given the context, I think it's fine to compromise and go down this route so I'll just implement it, it's quick anyways. Others can say what they think once it's done and we can change it once again if need be.

[thomasvl]

I think I'm good with things, but I'd like Tim to look also as original author of a bunch of the library.

[gjcairo]

Thanks @thomasvl, that's fair.

[gjcairo]

Had to rebase after the latest changes.

@tbkka mind looking at this when you have a minute?

[tbkka]

Finally getting to this, apologies for the delay.

There are a lot of details about the precise structure of the new errors ... it looks like Thomas and Gus have discussed those pretty thoroughly and I don't think there's much I could contribute. I just have two fairly high-level issues:

1. This is a source-breaking change so should not be merged to 1.x. (I presume that's obvious to everyone, just wanted to say it out loud.)

2. The changes to the throw sites within the library look pretty straightforward. But I'm less clear on what we expect clients to do with this new structure. Right now, I see this in the test suite:

do {
    ... stuff ...
 } catch let error as SwiftProtobufError where self.isSwiftProtobufErrorEqual(error, .BinaryDecoding.messageDepthLimit()) {
    ... deal with error ...

What should a client's catch statement look like in this new world?

[FranzBusch]

This is a source-breaking change so should not be merged to 1.x. (I presume that's obvious to everyone, just wanted to say it out loud.)

This shouldn't be a source breaking change. Why do you think it is? It reverts one source break that we merged into main so net-net this should be non source breaking compared to our last release

[tbkka]

This shouldn't be a source breaking change. Why do you think it is?

So the systematic changes to the test suite weren't actually necessary?

[gjcairo]

@tbkka we discussed this some more offline, and we decided to only move new errors (i.e. those that were only in main and not part of any tags) to the new, extensible SwiftProtobufError type, but kept the old enums. This is less tidy, but non-breaking. Hope we're all okay with this approach.

[Lukasa]

Ok, I think I'd like us to chat about this a bit. @thomasvl and @tbkka your opinions would be valuable.

In Protobuf 1.26, at this call site we used to throw BinaryEncodingError.anyTranscodeFailure. I'm inclined to say that where we don't need to change the thrown error type, we probably shouldn't. It's not an API break, so semver doesn't prevent us from making this change, but it does risk causing some trouble with users who actually were trying to catch this error.

However, this would mean these new errors are essentially removed from the tree. How do people feel about that?

[thomasvl]

You might need to go back to the PR, I believe this got added to fix a bug where someone wanted the better visibility into the failure. So reverting it would likely also mean reopening the bug as we still wouldn't be providing the requested information.

[Lukasa]

I think that's exactly the question I'm asking you to weigh in on: do you feel like that's acceptable?

[thomasvl]

Ah, ok, now I follow (I guess my ☕ needed more time 😄).

Historically (in the 1.x releases), we have added error codes, but I don't know that we ever changed a code, and have the existing enums, folks could catch all the errors of that type easily.

But yea, I guess this is different, not only could someone trying to catch this fail due to the change, I guess since it is a new type throw, it would completely escape their catch based on the enum. But if we stick to that train of thought pedantically, it means even throwing the new error types is problematic.

I guess I can(am) arguing both outcomes. I'd like to be able to correct errors, add better errors without having to go to a new major version, but I'm not sure where exactly to set the bar.

When/How did Nio decide to change the errors over and what was the reasoning behind that? I don't think you all did a major bump when it landed, correct?

[FranzBusch]

Yeah from my recollection the users wanted to get a better error here for troubleshooting on their end. We have to decide now. Do we want to strictly stick with throwing the errors that we are currently throwing and never change that until a new major. Or can we move individual errors to the new struct if it makes sense e.g. here because we can provide more details.

[thomasvl]

Should we open a new issue to reference all the things being reverted to provide a clean clearing house of all the errors we'll want to ensure we change whenever we finally do this change? i.e. - I don't want to accidentally lose a minor detail that we did work out and fail to recreate it whenever we do make the changes.

[Lukasa]

Seems sensible to me.

[gjcairo]

Okay, just made the change we discussed. I believe we're fully non-breaking now.

I'll open an issue listing the things we've reverted from this and other PRs.

[gjcairo]

#1657

[thomasvl]

@Lukasa when you get a chance, want to take another look?

[gjcairo]

@thomasvl are you okay with getting this merged since it's been approved on our end?

[thomasvl]

@thomasvl are you okay with getting this merged since it's been approved on our end?

Yea, I'm good. I wasn't sure if @tbkka should look again since he'd made some comment in the past and things were adjusted based on those comments.

[tbkka]

I'm good. 👍