RUST-317 Remove oid::Error::HostnameError

[saghm]

No description provided.

[patrickfreed]

Seems strange to have a different error enum + Result pair just for object id's. It looks like is the same case for encoder errors and decoder errors. These all seem like they could just be different cases on a single top level error type, similar to what we have in the driver.

[saghm]

They could be, but it isn't super uncommon for crates to have more than one error type. Serde itself actually provides two different error types for serialization versus deserialization. The main benefit of having a unified error type is that you can change the set of errors that a given method returns without breaking the API. In this case though, we wouldn't ever need to return a serialization error from deserialization or vice-versa, or return either of them when constructing an ObjectId. More generally, almost all the driver functionality is part of the same "API unit" and share the same internals (i.e. the Client). In the BSON library, the modules are pretty disjoint, with the exception that everything relies on the bson module. Since there's not as much code sharing, there isn't a huge likelihood of the error types showing up in more than one place like there is in the driver.

Given that it doesn't really cost us anything and it makes things easier for the user, I think it's worth keeping them separate.

[patrickfreed]

In light of that, I definitely agree that the encoder/decoder errors should be separate. That said, some of these errors seem like they shouldn't be object id specific, namely ArgumentError and IoError. As a side note, I looked briefly through oid.rs and couldn't actually find anywhere that returned an io::Error. I confirmed this by removing the From implementation.

it makes things easier for the user

Not necessarily. Each new error type results in a new From implementation in every user error type that interfaces with it. This can grow into a lot of boilerplate, especially if a user has several of their own error types which need to contain bson's errors.

[saghm]

I definitely don't want to provide error variants/From implementations that aren't used, so I've removed IoError (which I think is also an artifact of the now-removed hostname lookups). As for argument error, I don't really see the utility of sharing it between the encoder/decoder errors and the oid errors; if a user is putting our errors in their own enum with different variants, I don't think they're likely to be inspecting each of our error types, so the shared cases between them don't matter much (and ditto if they're using some sort of Box<Error>-like trait object for their error types), and if they are manually pattern matching them to perform different actions, they can always use the same case for all of the ArgumentErrors if they want.

This does make me think a bit about whether it's even worth having an oid::Error type though. There's a decent argument to be made that being unable to generate an oid is an unrecoverable error and therefore should just panic rather than forcing using to deal with the failure case every time. I'm not sure if this applies as strongly to use-cases of the BSON library outside the driver though; if it doesn't we could always just have the driver panic without making it an enforced part of the BSON API.

[patrickfreed]

As for argument error, I don't really see the utility of sharing it between the encoder/decoder errors and the oid errors

This wasn't what I was suggesting. I agree that encoder/decoder errors should be entirely separate and left as they are. What I meant was that instead of oid::Error, we have some top level bson::Error that includes common error cases like ArgumentError so we don't have to redefine them multiple times whenever we need to return them. It is conceivable (and likely) that we will want to have some other failable methods where invalid arguments are possible. An easy example I can think of is if we add Regex::new that validates the options are alphabetical and correct.

The benefits of fewer error types cascade to users who then don't have to define From<oid::Error>, From<regex::error>, etc. to make use of the ? operator in their code that interfaces with the bson crate.

There's a decent argument to be made that being unable to generate an oid is an unrecoverable error and therefore should just panic rather than forcing using to deal with the failure case every time.

I disagree with this. Imagine a REST API where users can post JSON that basically maps straight to BSON. We wouldn't want the backend crashing because the frontend accidentally posted a malformed ObjectId hex string.

[saghm]

I disagree with this. Imagine a REST API where users can post JSON that basically maps straight to BSON. We wouldn't want the backend crashing because the frontend accidentally posted a malformed ObjectId hex string.

Sorry for the lack of clarity; I was thinking specifically of ObjectId::new when I mentioned this. If the BSON library isn't able to create a new ObjectId, there isn't much a user can do. From looking at the implementation of this method, it doesn't seem to actually return an error in any circumstances anymore, so we can change that without causing it to panic while leaving the error type intact for the other constructors.

[patrickfreed]

Oh I misunderstood. Yeah, ObjectId::new being non-failable seems like a great choice.