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

GODRIVER-852 Add documentation warning against the use of duplicate key names #614

Merged
merged 5 commits into from
Apr 16, 2021
Merged

GODRIVER-852 Add documentation warning against the use of duplicate key names #614

merged 5 commits into from
Apr 16, 2021

Conversation

benjirewis
Copy link
Contributor

No description provided.

bson/bson.go Outdated Show resolved Hide resolved
@benjirewis benjirewis requested a review from iwysiu March 25, 2021 14:41
mongo/collection.go Outdated Show resolved Hide resolved
@benjirewis benjirewis requested a review from divjotarora March 25, 2021 18:56
Copy link
Contributor

@divjotarora divjotarora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's also backport this to release/1.5 so the documentation shows up for the next released version in pkg.go.dev.

@@ -319,7 +319,9 @@ func (coll *Collection) insert(ctx context.Context, documents []interface{},
//
// The document parameter must be the document to be inserted. It cannot be nil. If the document does not have an _id
// field when transformed into BSON, one will be added automatically to the marshalled document. The original document
// will not be modified. The _id can be retrieved from the InsertedID field of the returned InsertOneResult.
// will not be modified. The _id can be retrieved from the InsertedID field of the returned InsertOneResult. The document
// parameter should NOT contain duplicate key names, as that can result in undefined behavior from the server.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this apply to any CRUD function that results in a document getting inserted or replaced? (FindOneAndReplace, ReplaceOne, Replace). As well as the insert models for bulk write.

If that is right, then I think we'd want to document them all, or just document bson.D. My vote goes for just bson.D.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline: it seems like overkill to document every CRUD function that results in a document getting inserted or replaced. Instead, we've opted to only warn against duplicate keys on bson.D and in bson/doc.go. Let me know what you think...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this for consistency.

@benjirewis benjirewis requested review from iwysiu and kevinAlbs April 2, 2021 14:58
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delayed review. LGTM with a whitespace change!

mongo/collection.go Outdated Show resolved Hide resolved
@benjirewis benjirewis requested a review from divjotarora April 16, 2021 15:56
@benjirewis benjirewis merged commit 90d8c8e into mongodb:master Apr 16, 2021
@benjirewis benjirewis deleted the addDupKeyDocumentation.852 branch April 16, 2021 15:57
tsedgwick pushed a commit to mongodb-forks/mongo-go-driver that referenced this pull request May 27, 2021
tsedgwick pushed a commit to mongodb-forks/mongo-go-driver that referenced this pull request Jun 1, 2021
faem pushed a commit to kubedb/mongo-go-driver that referenced this pull request Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants