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

HELP-68823 Fix invalid_me comparison and normalize host strings #1319

Merged
merged 1 commit into from
Mar 5, 2025

Conversation

abr-egn
Copy link
Contributor

@abr-egn abr-egn commented Mar 4, 2025

HELP-68823

This wasn't the fix that I was intending to put into mainline, but that turns out to have a lot more in the way of wrinkles than I expected. In particular, we have some public APIs (e.g. ToplogyDescription::servers) that hand out &ServerDescription values, which locks us into having the underlying data structure storing that type either as the primary (how it's currently done) or as some kind of internally-mutable cache (ew).

There may be some clever path forward for split types that maintains API compatibility that I didn't see, but for the moment I think it's best to get the simple fix in and unblock downstream users. This PR, while minimal, fixes the core issue and explicitly normalizes anything that parses as an IP address, so it should be reasonably robust, and I've gone through and audited all the other uses of ServerAddress's Display impl and they're all test code or error messages so I think the chance of another recurrence of this problem is low.

@abr-egn abr-egn marked this pull request as ready for review March 5, 2025 14:39
@abr-egn abr-egn requested a review from isabelatkinson March 5, 2025 14:39
Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

LGTM! This seems like a good fix for now.

In particular, we have some public APIs (e.g. ToplogyDescription::servers) that hand out &ServerDescription values

Would this make sense to revisit in 4.0? i.e. return owned ServerDescriptions in the public API and convert from the internal type in those methods

@abr-egn
Copy link
Contributor Author

abr-egn commented Mar 5, 2025

Would this make sense to revisit in 4.0? i.e. return owned ServerDescriptions in the public API and convert from the internal type in those methods

Yeah, especially for places where the overhead of a copy isn't likely to be significant like events I think we should do a pass and bias towards returning owned values. Filed RUST-2170 to track.

@abr-egn abr-egn merged commit 1acffc4 into mongodb:main Mar 5, 2025
13 of 17 checks passed
abr-egn added a commit to abr-egn/mongo-rust-driver that referenced this pull request Mar 5, 2025
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.

2 participants