chore(deps): bump mongodb-build-info
#2563
Conversation
| dl_version: null, | ||
| atlas_version: null, |
There was a problem hiding this comment.
This shows that the types were actually wrong. mongodb-build-info has always been sending null for missing values, I just updated the types to match this in mongodb-build-info.
The signature of ConnectionExtraInfo prevents this (these properties are ? optional). I could either update the type here to accept null or map null to undefined (what I propose - to avoid propagating the breakage in types).
packages/service-provider-node-driver/src/node-driver-service-provider.ts
Show resolved
Hide resolved
kraenhansen
force-pushed
the
kh/bump-mongodb-build-info
branch
from
ab42194 to
83ff11d
Compare
| // ^ segment data is in snake_case: forgive me javascript, for i have sinned. | ||
|
|
||
| import getBuildInfo from 'mongodb-build-info'; | ||
| import * as getBuildInfo from 'mongodb-build-info'; |
There was a problem hiding this comment.
All the 💙 for moving towards correct imports!
| buildInfo: buildInfoPromise.then((bi) => bi ?? {}), | ||
| adminCommand: (command) => | ||
| this.runCommandWithCheck('admin', command, this.baseCmdOptions), |
There was a problem hiding this comment.
Here's what I'd have in mind for mongodb-js/devtools-shared#587 (review):
| buildInfo: buildInfoPromise.then((bi) => bi ?? {}), | |
| adminCommand: (command) => | |
| this.runCommandWithCheck('admin', command, this.baseCmdOptions), | |
| adminCommand: async (command) => { | |
| if (command.buildInfo) | |
| return (await buildInfoPromise) ?? {}; | |
| return await this.runCommandWithCheck('admin', command, this.baseCmdOptions); | |
| }, |
This is optional but I do feel like this is the right way to go, identifyServerName() shouldn't have to care. about whether the call to adminCommand actually runs a command or returns cached data
There was a problem hiding this comment.
You're right, this is a great idea 👍
It's more flexible and scalable as we might want to call more commands in the future.
There was a problem hiding this comment.
Pushed a fix doing this 👍
kraenhansen
force-pushed
the
kh/bump-mongodb-build-info
branch
from
83ff11d to
0658657
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR bumps the mongodb-build-info dependency to version ^1.8.1 and integrates the new identifyServerName function to improve server identification. The change replaces the previous approach of using getGenuineMongoDB with a more comprehensive server name identification that leverages the identifyServerName API.
Key changes:
- Integration of
identifyServerNamefrommongodb-build-infoto identify server types more accurately - Refactored connection info gathering to use the new API
- Updated import style from default to namespace import for
mongodb-build-info
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/service-provider-node-driver/src/node-driver-service-provider.ts | Added identifyServerName call in getConnectionInfo() and restructured promise handling |
| packages/service-provider-node-driver/src/node-driver-service-provider.spec.ts | Updated test expectations to account for additional command call |
| packages/service-provider-node-driver/package.json | Bumped mongodb-build-info dependency to ^1.8.1 |
| packages/service-provider-core/src/connect-info.ts | Changed import style and refactored to use serverName parameter instead of calling getGenuineMongoDB |
| packages/service-provider-core/src/connect-info.spec.ts | Updated test expectations and added serverName parameter to test calls |
| packages/service-provider-core/package.json | Bumped mongodb-build-info dependency to ^1.8.1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [ | ||
| buildInfo, | ||
| atlasVersion = null, | ||
| fcv = null, | ||
| atlascliInfo, | ||
| serverName, | ||
| ] = await Promise.all([ |
There was a problem hiding this comment.
The buildInfo destructured from the Promise.all result has no default value, but buildInfoPromise can resolve to an empty object {} when the catch handler executes. This inconsistency means buildInfo could be {} instead of the expected buildInfo response structure. Consider adding a default value of {} to match the catch handler's return value.
| atlas_version: atlasVersion?.atlasVersion ?? null, | ||
| is_genuine, | ||
| non_genuine_server_name, | ||
| is_genuine: serverName === 'mongodb' || serverName === 'unknown', |
There was a problem hiding this comment.
[nitpick] The logic for determining is_genuine treats 'unknown' as genuine, which may be misleading. If server identification fails and returns 'unknown', it's unclear whether the server is genuine or not. Consider documenting this behavior or adjusting the logic to handle the 'unknown' case more explicitly.
| is_genuine: serverName === 'mongodb' || serverName === 'unknown', | |
| is_genuine: serverName === 'unknown' ? undefined : serverName === 'mongodb', |
kraenhansen
changed the title
mongodb-build-infomongodb-build-info
|
The failure on this seem unrelated to the changes I'm introducing and it looks like they're matching those raised in #2562 |
This PR depends on mongodb-js/devtools-shared#587 being merged and released: I wanted to get this up as a draft to get some early feedback.