Skip to content

GODRIVER-1953 Refactor to (almost) not use bsonx package #692

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

GODRIVER-1953 Refactor to (almost) not use bsonx package #692

wants to merge 1 commit into from

Conversation

RammasEchor
Copy link
Contributor

Hi! I created this draft to ask some questions. I'm new to the mongodb driver, and I'm really trying to not break any rules, so apologies in advance. (I tried to talk to someone via a question ticket in Jira, and with comments on the issue, but nobody answered, so I'm creating this draft as a last resort. Again, apologies in advance if I'm breaking any rules).

I'm trying to work in a Jira issue ,but I have some questions:

  • Is this the correct approach? I investigated and read about bsonx and bson/bsoncore packages, and refactoring from bsonx to bson/bsoncore seems pretty straightfoward, since both have almost equivalent types. Running tests with everything in this commit passes (except some errors present both in the master branch and in this branch).
  • I'm having problems with two parts of code:

    • convertFileId() : In this section, I cannot return a bsoncore.Value in any form that does not break any test. My approach was focused on returning bsoncore.Value instead of bsonx.Val, and the things I tried were:

      • Marshaling before returning with Marshal
      • No Marshaling and just returning val
      • Constructing a bsoncore.Value with the corresponding type, and returning that.

    • bsonx.Binary() : This types of conversions. I could not find something equivalent in bsoncore. Some things I tried:

Thanks, any help is greatly appreciated!

@RammasEchor
Refactor to (almost) not use bsonx package
25f1995 
Removed the cases where it checks/creates a bsonx.Doc, and replaced
them with bson.D type if necessary. Removed functions that seemed only
to transform bsonx to another type. Removed a test for an specific
function for the same reason.

The bsonx package is largely replaced by bson and bsoncore. bsonx
was a proof of concept, and it should be completely replaced.

There is still various places that use bsonx:

	- benchmark directory
	- mongo/gridfs/bucket.go
	- mongo/gridfs/upload_stream.go
	- mongo/integration/collection_test.go
	- mongo/integration/gridfs_spec_test.go

And a file inside the same bsonx package uses bsonx package:
	- x/bsonx/array.go
@matthewdale matthewdale self-requested a review September 7, 2021 16:52
@matthewdale
Copy link
Collaborator

Hey @RammasEchor, sorry for the hilariously slow response and thank you for the PR! I'm looking into the two questions you have about the refactor. In the mean time, are you able to rebase this on the master branch to get the latest CI configuration?

@matthewdale matthewdale mentioned this pull request Mar 30, 2022
Merged
@matthewdale
Copy link
Collaborator

Hey @RammasEchor, I rebased these changes on the master branch and removed the remaining bsonx references from the "production" code paths in a new PR: #893. I believe you'll still get contributor credit when that PR is merged since one of the commits is yours. Please feel free to review or comment on the new PR. I'm closing this one in favor of the new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewdale matthewdale Awaiting requested review from matthewdale

Assignees

@matthewdale matthewdale

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RammasEchor @matthewdale