NODE-1295 Implement a connection string parser for the core module #269
Conversation
To satisfy the requirements of one of our connection string tests I chose to use `punycode.toUnicode` on the result of parsing a unicode string. This is error prone due to a number of deviations in the punycode standard vs unicode outlined in the RFC. Instead we should do the conversion in the tests, since the node URL parsers output is actually correct. NODE-1295
|
@mbroadst Are there any changes from the |
|
@daprahamian yes, significant changes. The output is completely different, but now up to spec. It turns out we weren't even checking that the unit tests expected output matched the result of parsing previously! |
lib/uri_parser.js
Outdated
|
|
||
| result.domainLength = result.hostname.split('.').length; | ||
| if (result.pathname && result.pathname.match(',')) { | ||
| return callback(new Error('Invalid URI, cannot contain multiple hostnames')); |
There was a problem hiding this comment.
Can we make a new type of MongoError here?
There was a problem hiding this comment.
sure, MongoParseError ?
There was a problem hiding this comment.
Yeah, that sounds good.
lib/uri_parser.js
Outdated
| // Default to SSL true | ||
| if (!options.ssl && !result.search) { | ||
| connectionStringOptions.push('ssl=true'); | ||
| } else if (!options.ssl && result.search && !result.search.match('ssl')) { |
There was a problem hiding this comment.
Is this just a regex lookup on a search string? If so, are we ok with matching an invalid field like myinvalidssl?
There was a problem hiding this comment.
good call, we can easily have node's parser parse the query items and check that way
lib/uri_parser.js
Outdated
| let connectionStringOptions = []; | ||
|
|
||
| // Default to SSL true | ||
| if (!options.ssl && !result.search) { |
There was a problem hiding this comment.
Can we DeMorgan / Karnaugh this and have one if?
if (!options.ssl && (!result.search || !result.search.match('ssl'))) {
...
}Or
if (!(options.ssl || result.search && result.search.match('ssl'))) {
...
}There was a problem hiding this comment.
sure, this section was actually a direct port of Jess' work earlier this month - wonder we didn't catch it then shrug
lib/uri_parser.js
Outdated
| } | ||
|
|
||
| dns.resolveTxt(result.host, (err, record) => { | ||
| if (err && err.code !== 'ENODATA') return callback(err); |
There was a problem hiding this comment.
Can we do:
if (err) {
if (err.code !== 'ENODATA') {
return callback(err);
}
record = null;
}
lib/uri_parser.js
Outdated
| return Object.keys(result).length ? result : null; | ||
| } | ||
|
|
||
| const SUPPORTED_PROTOCOLS = ['mongodb', 'mongodb+srv']; |
There was a problem hiding this comment.
Since we reference 'mongodb+srv' twice, we should probably make these constants
| "options": null | ||
| } | ||
| ] | ||
| } |
Avoid invalid strings matching a `ssl` direct match regex by using the node parser to parse the query items, and determine if an `ssl` key exists instead. NODE-1295
|
@jlord @daprahamian gooood to go |
This is a partial rewrite of the connection string parser from
nativeported tocore. The impetus for this was a need to parse connection strings during the SDAM refactor, but it is also a feature oft-requested from users (in particular Mongoose). The parser does not concern itself with the knowledge of any particular key (outside of the single deprecated key in the connection string spec), but instead serves to do the basic work of connection string parsing, leaving such specific concerns to the porcelain layer.It should be noted this is another handcrafted parser, and it's my strong feel that we should instead develop a grammar for our particular flavor of URI that can be shared amongst mongo drivers. Having said that, I feel like the refactoring here improves and modernizes the original and should satisfy our needs in the field.