Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for encoding/decoding strings as ObjectIds #98

Closed
dambrisco opened this issue Feb 9, 2018 · 5 comments
Closed

Add support for encoding/decoding strings as ObjectIds #98

dambrisco opened this issue Feb 9, 2018 · 5 comments

Comments

@dambrisco
Copy link

Currently, there's a rather unfortunate side effect of using this library in that you have to go pretty aggressively out of your way to use string-based IDs (especially in the case of microservices leveraging different DBs but using the same model library). One step toward improved support has been implemented in a PR against the originating fork: go-mgo#492

Yet more powerful might be the inclusion of a new tag that indicates that a given string field should be treated as an ObjectId for marshaling purposes.

Is there any resistance to the inclusion of this sort of feature?

@domodwyer
Copy link

Hi @dambrisco

It's not something we've come up against but I can definitely see the use case!

I like the tag idea (more than previous suggestions of simply changing an ObjectID to be a string) but how would you handle edge cases like a struct with multiple "this is an ObjectID" tags applied? Typically compile-time errors are preferred but mgo does provide runtime errors for other cases so it might not be a deal breaker.

Do you have an idea what the tag would look like?

Dom

@dambrisco
Copy link
Author

@domodwyer I believe multiple ObjectID tags would be admissible. As I imagine it, it functions only as a marshaling hint. Simply calling the tag "objectid" would be my first choice - it seems clear and concise enough to prevent confusion. So in the case of a declaration like Field string `bson:"keyname,objectid"` or Field string "keyname,omitempty,objectid", we simply have another step in the encoding and decoding processes.

The largest issue that I'm not sure how we might handle is non-string types. Some types, like bool, should likely either be ignored or throw an error and I'm not sure which is preferable. Other types, like int, pose a bigger challenge in that, if my understanding is correct, they can be represented as ObjectIDs just fine, although that might be incredibly unintuitive in some cases. Additionally, struct types could potentially represent ObjectIDs if we were to include an ObjectID interface that they could meet, but that is likely outside the scope of any initial work here - ignoring or throwing on struct types for now is probably more sensible. I'd especially love any thoughts or insights on these issues.

@domodwyer
Copy link

I like the objectid tag! It's clean and clear.

I hadn't thought about the int case though - you raise a good point. If the user tags it as an objectid I think it would be best to respect it and marshal it as a ObjectID? If an incompatible type (bool, etc as you say) is tagged with objectid I would prefer the marshal call to return an error over silently ignoring - always better to have the developer fix in a way that suits them.

It's a good idea! Including it is contingent on having a negligible performance cost but I can't see why that wouldn't be the case.

@a-pav
Copy link

a-pav commented Dec 7, 2018

Sorry, but what do you think about the String() method. Must it really return ids as ObjectIdHex("xyz")?

@a-pav
Copy link

a-pav commented Dec 9, 2018

@dalu Can you elaborate?
I didn't understand your point, sorry.
And .Hex() == .String()? yes, I want that.
I don't need .String method to wrap id inside ObjectIdHex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants