search

pharo-nosql / mongotalk

A Pharo driver for MongoDB
MIT License
19 stars 13 forks source link

Minor fix for improving support for error replies #90

Closed rydnr closed 3 years ago

rydnr commented 3 years ago

Minor fix dealing with error replies.

Tested with AWS DocumentDB and MongoDB Atlas (TLS with no SNI).

tinchodias commented 3 years ago

Hello @rydnr, thanks for the contribution. I added an inline review comment to learn more about the change.

Do you know what's the version of MongoDB running in AWS DocumentDB and MongoDB Atlas, where you tested it? I wonder why there is a new way to detect errors. Maybe Mongo 4.4 has news?

rydnr commented 3 years ago

AWS DocumentDB is not MongoDB, but supports the protocol. Or so they say. In practice, I found some issues and ended up discarding it. But in the mean time, the errors I received from DocumentDB bypassed the existing check so I added this numb change. It seems DocumentDB uses $errmsg instead of $err. The $code and $ok checks are there to exactly match the errors I found. For example, someone in his application uses $errmsg, as long as it doesn't use $code and $ok, this change would be harmless.

On the other hand, someone trying to use AWS DocumentDB might find this useful, if DocumentDB is suitable in his use case.

However, my motivation was two fold:

Since I'm not using DocumentDB, I don't need this change anymore.

tinchodias commented 3 years ago

Since I'm not using DocumentDB, I don't need this change anymore.

Well, we can close the PR. It will remain as a closed PR and can be recovered in the future if needed.

uses $errmsg, as long as it doesn't use $code and $ok

For the record, my question before was still valid anyway... I think there were missing "not"s on your code? something like: (results first includesKey: 'code') not and: [ (results first includesKey: 'ok') not ].