Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: only store topology on MongoClient #2594

Merged

Conversation

HanaPearlman
Copy link
Contributor

@HanaPearlman HanaPearlman commented Oct 21, 2020

Description

What changed?
A topology is no longer stored on Db or Collection; it's only on MongoClient. Each class only stores its parent and accesses the topology through that parent.

Methods that require a topology can now run into cases where the topology is undefined; an error is thrown during these cases.

This work is a step towards having a single, immutable topology on MongoClient which can never be null.

NODE-2850

@HanaPearlman HanaPearlman force-pushed the NODE-2850/master/topology-on-client branch from 5797beb to 7a50664 Compare October 21, 2020 21:36
@HanaPearlman HanaPearlman marked this pull request as ready for review October 22, 2020 12:57
Copy link
Contributor

@reggi reggi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one thought about adding getters / public API.

/**
* The current topology of the collection.
*/
get topology(): Topology | undefined {
Copy link
Contributor

@reggi reggi Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This here exposes topology as a top-level API, I'm generally OK with this, but it is something to acknowledge. We're usually conscious about what we allow as apart of the stable API. We can't make it private because we need to call it internally collection.topology. Calling the .s.properties is pretty annoying. Our users have always been able to call .s.topology but I don't think we consider that stable.

If we don't use getter's we can use deep properties this.s.client.s.db.s.topology 😢

Side thought may not need breaking change ! here?

I really like it, and I'd also be interested to add getters in all the places we're using topology / parent / db / client.

  get db () {
    return this.s.db;
  }

 get topology () {
    return this.db.topology;
  }

This would help in that we can always just call this.topology from within a given class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the getters idea is really good! Am happy to add that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm not sure we should take this approach. My main question is: who is this feature helping? Do we expect users to need to refer to the topology or the db of a Collection, or are we adding something here that only helps us (the maintainers)?

For starters, I don't think we should make it easy to access the Topology type, since we want that to be private API that we can change at will. Leaving the topology in the s is unsavory, but at least let's us make decisions later about how we want to continue to shield users from accessing private implementation.

If it's too hard to stomach this.s.db.client.topology, we could consider a new getTopology util method. Given how boilerplate this all is, I'm not sure it's worth the effort to make it look clean though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point. It seems like this would just be for the maintainers' benefit.

One thing to note is that a topology getter already exists on the Db class. Is this something we would consider removing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I have gone with a getTopology util method on the Collection class. Since a topology field exists on MongoClient and a topology getter exists on Db, it's a bit inconsistent. To address this, I think we could remove the topology getter on Db and (potentially) replace it with a getTopology util.

src/collection.ts Outdated Show resolved Hide resolved
@HanaPearlman HanaPearlman force-pushed the NODE-2850/master/topology-on-client branch from 749efd0 to 588cbec Compare November 2, 2020 15:31
src/utils.ts Outdated Show resolved Hide resolved
@HanaPearlman HanaPearlman force-pushed the NODE-2850/master/topology-on-client branch from 9844bb5 to 884c551 Compare November 3, 2020 14:03
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One 🧅 that's optional lgtm!

@@ -57,6 +57,7 @@ import { DestroyOptions, Connection } from '../cmap/connection';
import { RunCommandOperation } from '../operations/run_command';
import type { CursorOptions } from '../cursor/cursor';
import type { MongoClientOptions } from '../mongo_client';
import { Db, Collection, MongoClient } from '..';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import { Db, Collection, MongoClient } from '..';
import { Db } from '../db';
import { Collection } from '../collection';
import { MongoClient } from '../mongo_client';

Just an 🧅 I think internally we should import from the file directly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, also you may run into issues with require "cycles" with this approach (noted below).

} else {
throw new TypeError(
'parent provided to ChangeStream constructor is not an instance of Collection, Db, or MongoClient'
);
}

const topology = getTopology(parent);
if (!topology) throw new MongoClientClosedError();
this.topology = topology;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be able to use getTopology to remove the need to attach a topology property on ChangeStream as well

src/db.ts Outdated
* @param options - Optional settings for Db construction
*/
constructor(databaseName: string, topology: Topology, options?: DbOptions) {
constructor(databaseName: string, client: MongoClient, options?: DbOptions) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
constructor(databaseName: string, client: MongoClient, options?: DbOptions) {
constructor(client: MongoClient, databaseName: string, options?: DbOptions) {

nit, but if we're changing this anyway, let's try to go from most fundamental element to most optional element in the parameter ordering (this mirrors how we store things in our private s object)

src/db.ts Outdated
@@ -194,8 +194,8 @@ export class Db implements OperationParent {
}

// Topology
get topology(): Topology {
return this.s.topology;
get topology(): Topology | undefined {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aren't we removing all topology properties here?

@@ -636,8 +646,10 @@ export class Collection implements OperationParent {
throw new TypeError('`options` parameter must not be function');
}

const topology = getTopology(this);
if (!topology) throw new MongoClientClosedError('Collection.prototype.find');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than create a custom error message and throwing them in place, it looks like you can actually just move the error inside getTopology. getTopology should throw if it's unable to find the topology, with a MongoError which says that the client has been closed. Additionally, there is no need to include the name of the method in the error message string because you'll get that context in the stack trace anyway.

operation: T,
callback?: Callback<TResult>
): Promise<TResult> | void {
if (!topology) throw new MongoClientClosedError();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getTopology should be changed to always return a Topology, not Topology | undefined - that method will throw if one is not found. Likewise, you can remove this topology == null check since we now validate the presence of a Topology in getTopology

@@ -220,8 +220,8 @@ describe('MongoClient', function () {
client.connect(function (err, client) {
expect(err).to.not.exist;
var db = client.db(configuration.db);
expect(db).nested.property('s.topology.s.options.connectTimeoutMS').to.equal(0);
expect(db).nested.property('s.topology.s.options.socketTimeoutMS').to.equal(0);
expect(db).nested.property('s.client.topology.s.options.connectTimeoutMS').to.equal(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reuse getTopology here

test/unit/collection.test.js Show resolved Hide resolved
@@ -1053,3 +1054,10 @@ export class ServerCapabilities {
return this.maxWireVersion >= 5;
}
}

/** @internal */
export function getTopology(provider: MongoClient | Db | Collection): Topology | undefined {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way to implement this is to use ducktyping. It would remove the need for having to import these types into the file directly, which will prevent cycles in module loading:

export function getTopology(provider: MongoClient | Db | Collection): Topology {
  if (`topology` in provider) return provider.topology;
  if ('s' in provider && 'client' in provider.s && 'topology' in provider.s.client) return provider.s.client.topology;
  ...

@@ -57,6 +57,7 @@ import { DestroyOptions, Connection } from '../cmap/connection';
import { RunCommandOperation } from '../operations/run_command';
import type { CursorOptions } from '../cursor/cursor';
import type { MongoClientOptions } from '../mongo_client';
import { Db, Collection, MongoClient } from '..';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, also you may run into issues with require "cycles" with this approach (noted below).

src/error.ts Outdated
* @public
* @category Error
*/
export class MongoClientClosedError extends MongoError {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's uncommon for us to define a single-purpose error like this, and I think (as I said above) we can get away with moving the logic (and creation of the error) to one location inside getTopology. In the future I would generally say that single purpose errors may indicate an issue in code structure

@HanaPearlman HanaPearlman force-pushed the NODE-2850/master/topology-on-client branch 2 times, most recently from c27692b to 80d7565 Compare November 5, 2020 16:20
@HanaPearlman HanaPearlman force-pushed the NODE-2850/master/topology-on-client branch from 80d7565 to e218676 Compare November 5, 2020 17:02
Copy link
Member

@mbroadst mbroadst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 👏 very nice work here, LGTM

@HanaPearlman HanaPearlman merged commit 33fa6b2 into mongodb:master Nov 6, 2020
@HanaPearlman HanaPearlman deleted the NODE-2850/master/topology-on-client branch November 6, 2020 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants