globalsign / mgo

The MongoDB driver for Go
Other
1.97k stars 230 forks source link

Connection URL parsing breaks valid tagsets #213

Closed jmcclell closed 6 years ago

jmcclell commented 6 years ago

The URI parser makes some assumptions that break standard mongo functionality.

In particular, we've encountered two issues for passing tagsets for readPreferenceTags.

Given the following connection URL: mongodb://mongodb-1.example.com,mongodb-2.example.com,mongodb-3.example.com?replicaSet=example-replset&readPreference=nearest&readPreferenceTags=foo%3Abar&readPreferenceTags=

We are passing a URL encoded tagset that prefers servers tagged with foo:bar but will fallback to any other server via the empty tag wildcard, as documented here.

Unfortunately, the URL parser doesn't accept an empty tag pair, at least not with the syntax used with other drivers, ie: readPreferenceTags=. This leads to the following error:

connection option must be key=value: readPreferenceTags=

If there is a valid way to pass an empty tagset, it doesn't appear to be documented?

Additionally, when specifying our tag for foo:bar, we are properly url encoding the colon. Unfortunately, the url decoding stage of the url parser happens after the tag split on colon and, even then, it only decodes the key and value parts from the split. The entire url should be decoded prior to parsing. A url encoded colon in the tag pair results in this error:

bad value for readPreferenceTags: foo%3Abar

domodwyer commented 6 years ago

Hi @jmcclell

The connection string parsing is not the most robust thing in the world - it was inherited from the original MGO code and hasn't really been touched since other than bolting on new bits. It's definitely not to the mongo spec.

Does setting DialInfo.ReadPreference not work for your use case?

Dom

jmcclell commented 6 years ago

Hi @domodwyer,

I'm actually on the devops side of the equation and not the actual developer of the application in question. My team is responsible for passing configuration to applications at runtime and we've standardized on the connection string approach (ie: passing a fully formed connection string with all required options) for our apps that use Mongo.

I will have our developers look at DialInfo.ReadPreference, but it will necessitate that we add an exception to our normal practice and doesn't negate the fact that this is still a bug in the parsing logic. I do appreciate you pointing it out, however, as it may help us move forward nonetheless.

domodwyer commented 6 years ago

Hi @jmcclell

I fully understand the frustration - we've opted to maintain backwards compatibility with the original go-mgo which means we've inherited bits like this connection string parsing being contrary to the mongodb spec.

It is worth looking to see if it's possible to fix the issues you describe in a backwards-compatible way (we would definitely appreciate a PR if it's possible) but I recommend using DialInfo to control mgo behaviour in general (we have a DB config struct we serialise as part our config internally) as it provides the most flexibility.

Sorry!

Dom