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

Upgrade MongoDB driver version for compatibility with MongoDB 5 #7344

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

hognefossland
Copy link

Like others, I've received emails from MongoDB about the forthcoming automatic upgrade of the free offering to version 5.0. As mentioned in issue #7294, MongoDB version 5.0 requires a driver version of 4.0 or above. We currently use 3.6.

This is an attempt to upgrade the driver version. I've tried to make as few changes as possible, meaning I have not attempted to swap out deprecated functions that still haven't been removed from the driver.

The unit tests all pass and I have (smoke) tested the changes as much as I've been able to (I have deployed to my own instance, and so far I haven't experienced any issues), but I can't say I've performed exhaustive testing.

As MongoDB's promised upgrade has not yet taken place, I also haven't actually performed any testing against version 5.0 of the database.


if (err) console.error('Problem upserting treatment', err);

if (!err) {
if (updateResults.result.upserted) {
obj._id = updateResults.result.upserted[0]._id
if (updateResults.modifiedCount == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this identical behavior to the old?

Copy link
Author

Choose a reason for hiding this comment

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

My understanding of the old code is that it it returns upserted = true if the update() ended up doing an insert rather than an update. In this case, it updates the _id of obj with the _id generated by and returned from the call.

Similarly, my understanding of the "new" code is that modifiedCount == 0 (from a successful replaceOne() call) indicates nothing was was modified, hence it must have been an insert rather than an update. In this case we retrieve the generated _id from the returned upsertedId field. I'm certainly open to correction on this.

Btw - I replaced update() with replaceOne() as update() in driver version 4 seems to require specifying individual fields to update rather than being able to update the entire document. replaceOne() looks like the closest match to the old update().

The documentation has the following to say about the return value from replaceOne():

Returns: A document containing:

  • A boolean acknowledged as true if the operation ran with write concern or false if write concern was disabled
  • matchedCount containing the number of matched documents
  • modifiedCount containing the number of modified documents
  • upsertedId containing the _id for the upserted document

Perhaps matchedCount might be a better value to compare with? I'm guessing it too would be 0 for an insert.

@hognefossland hognefossland changed the title Wip/mongodb driver upgrade Upgrade MongoDB driver version for compatibility with MongoDB 5 Feb 17, 2022
@yp1ripe
Copy link

yp1ripe commented Feb 18, 2022

I've fetched your hognefossland:wip/mongodb-driver-upgrade and attempted to deploy it as 14.2.5 is very unstable last week after mongodb upgrade to 5.0.6 ( several crashes per day versus virtually no issues at all last year ).

So far, no luck. Your branch crashes on the first devicestatus upload from my daughter's phone after deploy,

Please check the attached log fragment:

err.txt

@hognefossland
Copy link
Author

hognefossland commented Feb 18, 2022

Hmmm... Thanks for checking and for reporting back. I haven't seen anything like that here yet, but I've just noticed my instance of MongoDB has also been upgraded to 5.0.6 now, so perhaps my joy won't last too long...

I'm away for a week now (which is actually why I attempted to fix this first), so won't be able to look at it until I'm back. But I'll try to get to it as soon as I can unless someone else manages to make progress on the meantime.

@hognefossland
Copy link
Author

@yp1ripe I see you're using OpenAPS, which I am not. I still haven't had any issues with this version--even with MongoDB upgraded to v5.0.6--so I suspect your setup simply uses features that mine doesn't. As I don't know of a simple way to test this particular section of the code, hopefully somebody more knowledgeable might be able to assist.

Could you please confirm that you're getting this particular issue only with my version?

@yp1ripe
Copy link

yp1ripe commented Feb 28, 2022

@yp1ripe I see you're using OpenAPS, which I am not. I still haven't had any issues with this version--even with MongoDB upgraded to v5.0.6--so I suspect your setup simply uses features that mine doesn't. As I don't know of a simple way to test this particular section of the code, hopefully somebody more knowledgeable might be able to assist.

Could you please confirm that you're getting this particular issue only with my version?

I am using AndroidAPS, actually. Anyway, AndroidAPS is also based on OpenAPS algorithm. Yes, this particular issue exists only with your version. Perhaps, the reason is that the websocket.js is only used for data upload by AndroidAPS, according to Milos Kozak, the author.

@PieterGit PieterGit mentioned this pull request Apr 30, 2022
@bewest
Copy link
Member

bewest commented Apr 30, 2022

I've fetched your hognefossland:wip/mongodb-driver-upgrade and attempted to deploy it as 14.2.5 is very unstable last week after mongodb upgrade to 5.0.6 ( several crashes per day versus virtually no issues at all last year ).

So far, no luck. Your branch crashes on the first devicestatus upload from my daughter's phone after deploy,

Please check the attached log fragment:

err.txt

@yp1ripe Thanks for this information. Can you explain a bit more about when you receive errors like this? Is it when using these patches with the same database? What version mongodb database were you using to get these errors? Did the errors start after changing database versions or using the patch?
@MilosKozak Is this the kind of behavior that led to your NPE PR? #7284 Is the NPE related to devicestatus or to mongodb changes?

FWIW, lines 15 - 42 of lib/server/devicestatus.js might be worth investigating. I see that a const is used but don't quite understand the behavior here. Without use of a const this could result in referencing an incorrect object; with a const I would expect some kind of error assigning to const.

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