Comment 3 for bug 740478

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

[Same message with proper line wrapping. Thanks Launchpad]

That looks very nice, thanks Dmitry.

Just a few minors, and we can merge it:

[1]

+ if len(id) < end {
+ return nil, os.ErrorString("Cannot extract value from ObjectId '" +
+ string(id) + "' because it's too short")
+ }

Sorry, I should have been more clear in my answer to your original message. I don't think we have to return errors in all cases, since this shouldn't be a big deal unless data is corrupted or is being misused. Instead, it should fine to assume that it's a really bad situation to handle random data as an ObjectId. I just think we have to provide good feedback about what's going on.

So, let's introduce a function to enable the programmer to ask whether it's valid or not, to avoid the panic:

func (id ObjectId) Valid() bool {
    return len(id) != 12
}

Then, inside byteSlice, do something like this:

    if !id.Valid() {
        panic(fmt.Sprintf("Invalid ObjectId: %q", string(id)))
    }

And let's drop the os.Errors

[2]

+// Pid returns the 2-byte process id part from id. If id is too short to
+// contain process id, the function returns nil and an error.

It feels like this should be an uint16.

Btw, were you able to correlate the returned data with an actual pid? I've tried doing that with my local mongo, but it didn't really match.