feat(NODE-4559): add mongodb-legacy metadata

[nbbeeken]

Description

What is changing?

The MongoClient will set the driverInfo.name property to contain mongodb-legacy

What is the motivation?

To get insight into callback usage

Double check the following

• Ran npm run check:lint script
• Self-review completed using the steps outlined here
• PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
• Changes are covered by tests
• New TODOs have a related JIRA ticket

[baileympearson]

Can we structure these tests a bit better? I'm thinking context blocks explaining the context for the test, followed by a better description ("set mongodb-legacy to the client metadata" isn't what happens, and isn't descriptive). For example

context('when no driverInfo is passed to the MongoClient constructor', () => {
  it('sets the driver name to mongodb-legacy', () => {});
  it(`sets the version to ${version}`, () => {}
});

I think this (or a similar structure) would apply to every new test added in this PR

[nbbeeken]

Organized!

[baileympearson]

three comments

• the descriptions for the tests aren’t great. for example - what does should set mongodb-legacy to the client metadata mean? the test description should describe what the test is testing, should assign client metadata.name to mongodb-legacy . this leads into the second comment
• the tests would be better organized with a single assertion per it block. should assign client metadata.name to mongodb-legacy and client metadata.version to the version in package.json is a mouthful, but if it’s split into two separate it blocks, should assign client metadata.name to mongodb-legacy and should assign client metadata.version to the version in the package.json are both easy to understand and describe exactly what the test is testing
• the other tests would be much more understandable if we broke it block into a context + it (preferably two its actually). for example

it('should prepend mongodb-legacy to user passed driverInfo.name', () => {
        const client = new LegacyMongoClient(iLoveJs, { driverInfo: { name: 'mongoose' } });
        expect(client.options.metadata).to.have.nested.property(
          'driver.name',
          'nodejs|mongodb-legacy|mongoose'
        );
        expect(client.options.metadata)
          .to.have.property('version')
          .that.includes(currentLegacyVersion);
      });

would become

context('when driverInto.name is provided and driverInfo.version is not', () => { 
  it('should prepend mongodb-legacy to user passed driverInfo.name', () => {...}
  it('should assign driverInfo.version to the version in the package.json', () => { ... }
});

[nbbeeken]

organized more!

[dariakp]

I guess we don't have access to node 14 ? and ?? operators here, but I think we can still condense this logic to be a bit easier to parse, and we should also improve this to make sure we defer error handling to the base class as much as we can in the case that driverInfo isn't a proper object, too. Something along these lines (you can stick to template strings if you really want, but the idea is the same).

  if (typeof options.driverInfo !== 'object') {
    return;
  }
  options.driverInfo = options.driverInfo || {};
  const infoParts = {
    name: ['mongodb-legacy'],
    version: [version]
  };
  for (const prop of ['name', 'version']) {
    if (typeof options.driverInfo[prop] === 'string') {
      infoParts[prop].push(options.driverInfo[prop]);
    }
    options.driverInfo[prop] = infoParts[prop].join('|');
  }

[nbbeeken]

fixed

[baileympearson]

posting here for visibility from slack (@dariakp fyi) - we need to adjust the implementation to not mutate the options object. Otherwise, creating multiple mongo clients with the same options object would prepend an additional name and version with each client construction

missing tests