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

fix(NODE-5999): Change TopologyDescription.error type to MongoError #4028

Merged
merged 8 commits into from
Mar 14, 2024

Conversation

aditi-khare-mongoDB
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB commented Mar 8, 2024

Description

Expand type descriptions for certain uses of MongoServerError to MongoError and remove casting.

What is changing?

  • Change the type of Server.error, ServerDescription.error, ServerDescriptionOptions.error and TopologyDescription.error to be MongoError | null instead of MongoServerError | null.
  • Change the type of the argument to markServerUnknown method to be MongoError instead of MongoServerError
Is there new documentation needed for these changes?

Yes, there is a change being made to the public TopologyDescription class (the error property of that class specifically).

What is the motivation for this change?

Before NODE-5988 merged, the type signature of MongoServerError was identical to MongoError, and prior to this PR, multiple places in the driver, MongoServerError was used imprecisely.

In the server.handleError method and its test file, it's noted that the following are errors the server can "handle" or mark as unknown properly:

  • any MongoServerError
  • a MongoError with a MongoErrorLabel.HandshakeError
  • a MongoNetworkError that's not a MongoNetworkRuntimeError.

Therefore, MongoServerError is the incorrect type for the markServerUnknown method called within server.handleError method, and the type was expanded to MongoError. This trickles up to Server, ServerDescription, ServerDescriptionOptions, and TopologyDescription.error.

Release Highlight

TopologyDescription.error type is MongoError

Important

The TopologyDescription.error property type is now MongoError rather than MongoServerError.

This type change is a correctness fix.

Before this change, the following errors that were not instances of MongoServerError were already passed into TopologyDescription.error at runtime:

  • MongoNetworkError (excluding MongoNetworkRuntimeError)
  • MongoError with a MongoErrorLabel.HandshakeError label

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
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(Node 5999): Fix mongoServerError casting fix(Node 5999): Fix MongoServerError casting Mar 8, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB force-pushed the NODE-5999/fix-MongoServerError-casting branch 3 times, most recently from 82fa338 to 5f87446 Compare March 8, 2024 21:50
@mongodb mongodb deleted a comment from evergreen-ci-prod bot Mar 8, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB marked this pull request as ready for review March 11, 2024 17:50
@baileympearson baileympearson self-requested a review March 11, 2024 20:20
@baileympearson baileympearson self-assigned this Mar 11, 2024
@@ -307,13 +307,13 @@ export class TopologyDescription {
);
}

get error(): MongoServerError | null {
get error(): MongoError | null {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically public. I think this change is okay to make, because we usually permit breaking TS fixes in non-major releases. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is an expanded type considered a breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm on second thought - if we had made this change before c023242, I don't think this would have been breaking. but since MongoServerError is no longer assignable to MongoError, it's now breaking to make this change? But I still think that's okay. This is a correctness fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MongoServerError inherits from MongoError, so MongoServerError can be assigned to MongoError, but not the other way around. This is why we had to make this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

interface Foo {
  name: string;
}

interface Bar extends Foo {
  age: number;
}

const document: Bar = { name: 'bumpy', age: 7 };

If Bar is changed to Foo, this code fails to compile. That's why this change is potentially problematic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm I see. Should we wait until the next major version to make this change?

@nbbeeken nbbeeken self-requested a review March 11, 2024 21:51
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Mar 12, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(Node 5999): Fix MongoServerError casting fix(NODE-5999): Fix MongoServerError casting Mar 13, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(NODE-5999): Fix MongoServerError casting fix(NODE-5999): Change MongoServerError to MongoError for markServerUnknown function and its dependencies Mar 13, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(NODE-5999): Change MongoServerError to MongoError for markServerUnknown function and its dependencies fix(NODE-5999): Fix to TopologyDescription.error type Mar 13, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(NODE-5999): Fix to TopologyDescription.error type fix(NODE-5999): Fix TopologyDescription.error type to be MongoError Mar 13, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(NODE-5999): Fix TopologyDescription.error type to be MongoError fix(NODE-5999): Change TopologyDescription.error type to MongoError Mar 13, 2024
@nbbeeken nbbeeken removed their request for review March 13, 2024 21:36
@baileympearson baileympearson merged commit 30432e8 into main Mar 14, 2024
19 of 26 checks passed
@baileympearson baileympearson deleted the NODE-5999/fix-MongoServerError-casting branch March 14, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants