[GODRIVER-1020] Primitive UnmarshalJSON method should return a zero'd objectId when the field is empty instead of a decode error Created: 05/May/19 Updated: 28/Oct/23 Resolved: 23/Apr/20 |
|
| Status: | Closed |
| Project: | Go Driver |
| Component/s: | JSON & ExtJSON |
| Affects Version/s: | 1.0.1 |
| Fix Version/s: | 1.4.0 |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Quest Henkart | Assignee: | Divjot Arora (Inactive) |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Description |
|
Currently, when ObjectIds are unmarshalled from a JSON payload https://github.com/mongodb/mongo-go-driver/blob/master/bson/primitive/objectid.go#L100 , 64 and 12 byte strings are properly converted to an objectId. All others return decoding errors. This is expected functionality.
However an empty string or empty field also returns an error instead of a zero/null valued objectId. This makes using the primitive.ObjectID type within a struct definition very opinionated, and negates the need for IsZero methods. For example, if a service layer is responsible for decoding a struct and then setting ObjectID values based on claims information, the request will hit a decoding error since the payload will not contain the objectIds. It forces the use of struct composition to decode input values, by necessitating the extension to a more complete struct definition to apply the ObjectIds. It could very easily be argued that this is a best practice, but it should not be the responsibility of the mongo-driver to force such a high level design decision
Request: ObjectIds should be decoded for 64/12 bytes and for empty fields it should initialize a zero valued objectId that can then be validated, initialized or handled in the application logic |
| Comments |
| Comment by Githook User [ 23/Apr/20 ] | |||
|
Author: {'name': 'Divjot Arora', 'email': 'divjot.arora@10gen.com', 'username': 'divjotarora'}Message: | |||
| Comment by Divjot Arora (Inactive) [ 22/Apr/20 ] | |||
| Comment by Ed Pelc [ 20/Apr/20 ] | |||
|
Awesome! Hope it makes it through. | |||
| Comment by Divjot Arora (Inactive) [ 20/Apr/20 ] | |||
|
Ok, I've moved the ticket back to "Needs Triage" so the team can discuss it in our meeting later today. | |||
| Comment by Ed Pelc [ 20/Apr/20 ] | |||
|
Yes, that is correct. With json input `{"userId":""}` Foo.UserID should equal `primitive.NilObjectID` instead of erroring out. | |||
| Comment by Divjot Arora (Inactive) [ 20/Apr/20 ] | |||
|
epelc@greatcloak.com From what I understand, the request is to allow a JSON string like {"userId": ""} to unmarshal into a struct defined as
Is this correct? | |||
| Comment by Ed Pelc [ 19/Apr/20 ] | |||
|
Hi Quest, Thanks for the library and info! I looked it over and I don't think it is the right way to go about this. Using a 12 byte array as an object id is more efficient and I agree with the official lib's reasons for switching to this. Also we've converted to this already on the backend so the work is done. The real thing though is that if you convert to the official lib you end up with compilation errors surrounding the old mgo object ids. If you fix all compilation errors it is compatible besides for this small point of not handling empty strings with json. I think this should be added for this upgrade reason and also when dealing with third parties or frontend code with JSON an empty string is very common since it is often not explicit that you set this. Even if you update all frontend code you still have third parties or people using your api which will easily run into errors with minimal info since the std lib json library doesn't give you a field that it failed on. The current implementation already handles 64 and 12 byte representations. I think it's a fair extension to lax this to handle empty strings too. | |||
| Comment by Quest Henkart [ 19/Apr/20 ] | |||
|
Hey Ed, when this ticket was rejected I made a small abstraction that completely resolved the issue and made migrating from mgo very easy. It also made dealing with the new library way less error prone. Feel free to use it | |||
| Comment by Ed Pelc [ 18/Apr/20 ] | |||
|
Could we consider reopening this? Allowing an empty json string to unmarshal to `primitive.NilObjectID` would greatly help with upgrading from mgo. If you are just using mongodb I don't see why you'd need or want an extra abstraction layer especially for such a common scenario. Forcing a client to send 12 0s, or delete a field entirely(no empty string, null, etc ie unset the key) due to such a minor change is a big pain and also hard to do in existing frontend apps. You also can't use a pointer to an object id to workaround this like you can with `omitempty` when marshaling. The only option for upgrading is large code modifications on the frontend/connected services or to rip out all uses of `primitive.ObjectID` with a replacement on the backend. I think this is common enough to include in the official lib though.
| |||
| Comment by Kristofer Brandow (Inactive) [ 30/May/19 ] | |||
|
Hi qhenkart, We do handle JSON and while it's not recommended you can use types like primitive.ObjectID in your APIs. You can use them, however you will be directly coupling your service to MongoDB. It is not supported to use types like primitive.ObjectID while also wanting to be decoupled from MongoDB. We support extended JSON and for convenience we also support decoding ObjectIDs that are properly formatted hex strings. At this time, an empty string does not count as a properly formatted hex string. | |||
| Comment by Quest Henkart [ 18/May/19 ] | |||
|
thank you @krisbrandow I appreciate the response. I think this answer is adequate, but I still think documentation and migration instructions need to reflect this behavior.
It is strange though, that given your response, it seems like the primitive.ObjectID unmarshalJSON method here https://github.com/mongodb/mongo-go-driver/blob/master/bson/primitive/objectid.go#L100 would know how to handle the exact behavior you described (converting string id payloads used in other services, along with empty strings to objectIDs or nil objectIDs in the mongo service). Why have a JSON unmarshal method that doesn't know how to handle JSON payloads properly? It seems to me like the opinionated logic should be confined to the BSON marshallers and validators instead as they are built to interact with BSON directly
Given the context of this answer, I look forward to your additions involving this ticket https://jira.mongodb.org/browse/GODRIVER-1030 | |||
| Comment by Kristofer Brandow (Inactive) [ 17/May/19 ] | |||
|
Hi qhenkart, The types within the Go driver's BSON library are meant to be used directly with the mongo package and aren't general types meant to be used in other APIs. While they may function for that, it is recommended that you create a database abstraction layer, using your own types, that you then convert into types that can be inserted into the database. For example, instead of using the primitive.ObjectID type directly, use a string in the type you decode the HTTP JSON payload into, then convert that string into an objectID when inserting it into the database. | |||
| Comment by Quest Henkart [ 12/May/19 ] | |||
|
@isabella To be clear, I am not talking about representing a BSON document with JSON. I am talking about everyday use of the primitive package in a typical Go Application, in this case simply decoding a JSON payload from an http request, still far removed from interfacing with mongo or creating a bson document. I think this ticket is more related to an important Go-ism that the Go driver team should consider rather than the mongod spec. This behavior will require any go service that is in communication with a go-mongo enabled service to be refactored to fall in line with the mongod spec, due to Go’s zero-valued initialization of strings and other values, and cause services to bleed mongo specifications in their APIs
It creates a lot of coupling to the mongo driver with non-mongo-services. We don’t want our non-mongo-services to be concerned with the mongod bson/json spec, we don’t even want to expose that we are using mongo. We want our go clients to only be concerned with our API reqs. An entire http request will fail anonymously on receipt of a JSON payload with an un-initialized ID field from a Go client without giving us the ability to validate or create our own behaviors/error scenarios. The GO-driver should not be an opinionated framework but simply a way to interface with mongo and bson from a Go service.
In our case, we can just fork the library, but this behavior needs to be clearly documented in the migration guide as it is at odds with all of the current mongo community drivers | |||
| Comment by Isabella Siu (Inactive) [ 10/May/19 ] | |||
|
Hi qhenkart, This request goes against the extended JSON spec, which does not consider an empty string a valid ObjectId. | |||
| Comment by Quest Henkart [ 06/May/19 ] | |||
|
@isabella Thank you for investigating this matter. Here is a gist that will create the undesirable results https://gist.github.com/qhenkart/99ab370875130c6c82e12bdf6594d0ec
The main issue is that if one Golang service is passing a payload to another. But the former doesn't use the mongo library (opting for string representations of ids instead), Golang will automatically initialize the id fields in the struct as zero valued strings `""`. This makes any Go server unable to pass data easily unless they both share the mongo library. The same goes with requests from clientside javascript, although they are less likely to have uninitialized Id fields unless the server has given it to them | |||
| Comment by Isabella Siu (Inactive) [ 06/May/19 ] | |||
|
Hi qhenkart, Can you provide either the json you're using or an example program illustrating this? | |||
| Comment by Quest Henkart [ 06/May/19 ] | |||
|
I've made a gerrithub PR here https://review.gerrithub.io/c/mongodb/mongo-go-driver/+/453160
and a normal github PR here https://github.com/mongodb/mongo-go-driver/pull/155 |