Implement extraction of ObjectId parts

Bug #740478 reported by Dmitry Chestnykh
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
gobson
Fix Released
Undecided
Dmitry Chestnykh

Bug Description

Methods to extract parts of ObjectId as described in http://www.mongodb.org/display/DOCS/Object+IDs implemented in the attached patch.

Please suggest doc and code improvements, if any.

Revision history for this message
Dmitry Chestnykh (dmitry-codingrobots) wrote :
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

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.

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.

Revision history for this message
Dmitry Chestnykh (dmitry-codingrobots) wrote :

Agree, will fix. BTW, should I put something like:

// Calling this function with an invalid id will cause a runtime panic.

into doc above every function?

>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.

Python driver puts "os.getpid() % 0xFFFF" into this field, while Mongo's logic is more complex: https://github.com/mongodb/mongo/blob/master/bson/oid.cpp

While documentation says "This is 2 bytes of the process id (or thread id) of the process generating the object id.", it's not what they actually do, so it may not correlate with the process id of mongod. Maybe change the doc string for Pid method to state "unique identifier of the process that generated id" to avoid confusion?

Revision history for this message
Dmitry Chestnykh (dmitry-codingrobots) wrote :

On the first point in my previous comment, I think it's better to just say

// [Method] returns [which] part of the valid id.

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

Sounds good regarding the error message and the comment structure. Please use this wording:

// [Method] returns the [which] part of the id.
// It's a runtime error to call this method with an invalid id.

Regarding the pid, let's just use a big endian uint16 then.

Revision history for this message
Dmitry Chestnykh (dmitry-codingrobots) wrote :

Updated patch.

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

This looks great, thanks Dmitry. I'll integrate it later today.

Changed in gobson:
status: New → In Progress
assignee: nobody → Dmitry Chestnykh (dmitry-codingrobots)
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

That's in!

Changed in gobson:
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.