Implemented Default for Bson, Document, and ObjectId

[NathanFlurry]

Implemented Default for Bson, Document, and ObjectId. I also fixed some warnings with the tests.

[zonyitoo]

Looks great actually. I will merge now. cc @kyeah

[kyeah]

OID default should generate a new id via .new, i believe; what do yall think?

[zonyitoo]

Well, it depends on what you need for Default. Could @NathanFlurry tell us why you need Default for those structs?

[saghm]

I'm actually a bit unsure that having Default for ObjectId is a good idea. At least in terms of usage with MongoDB, an all-zero value of ObjectId seems like a very bad idea (you can just use regular integers as your _id fields if you want to have control over the value), and having it generate a new one seems a little weird also given that two calls to ObjectId::default() won't return the same thing, which I feel like violates how Default is expected to behave.

[NathanFlurry]

@zonyitoo I implemented Default for these structures so I can use methods like unwrap_or_default() on them.
I agree that the implementation for ObjectId is a bit odd, but I did it that way because I believe that the result of default() should always be the same and not generate different values every time, like ObjectId::new() does. Maybe Default just doesn't fit on ObjectId at all.
Edit: After looking at the implementation for RandomState, I guess it's fine for Default to have a random value.

[saghm]

I'm not certain it's really worth it just to avoid having to do opt_oid.unwrap_or_else(ObjectId::new()). Another common use of default I can think of is to save having to explicitly initialize every field of a struct (e.g. Foo { x: 1, y: 2, .. Default::default() }, but that doesn't apply here either. I think it's fairly obvious to a user that something called RandomState won't be the same every time it's created, whereas ObjectId might not be as obvious to someone who isn't familiar with the implementation (which is by design not required knowledge for using BSON/MongoDB).

The only argument in favor of implementing ObjectId::default I can think of is so that users can derive Default on structs that they want to serialize/deserialize to and from the database. This is also a pretty clear argument against having it being initialized to all zeroes. I don't think it would actually be terrible to implement it as a wrapper around ObjectId::new for this purpose, but I think it's a good idea to add a (doc) comment mentioning that it won't always return the same value.

[zonyitoo]

unwrap_or_else would be better

[saghm]

Oh geez, I had forgotten that ObjectId::new returns a Result. Given that the only reason I can think of to even have Default on ObjectId is to be able to serialize/deserialize structs, it seems extremely risky to just have silent failures; I can't imagine that many people would bother checking the resulting struct/bson doc to see if the ObjectId is all zeros even if we told them to, especially given that there isn't any easy way to do so, as the array field is (correctly) non-public. I think I'm back to recommending against having Default implemented for ObjectId, only more strongly than before.

[NathanFlurry]

I agree, too much unexpected behavior and I guess it doesn't really fit in this situation.

[zonyitoo]

I am Ok with that. How about @saghm @kyeah

[saghm]

LGTM. Thanks for bearing with me when I kept changing my mind!

[kyeah]

Awesome! Completely agree with the conversation today -- too much unpredictable behavior for a Default implementation. Was out today, so appeeciate y'all talking this one through to it's conclusion.

[zonyitoo]

Will be released tonight (GMT+8)