Skip to content

DRIVERS-3155 Add topologyClosedEvent and topologyOpeningEvent #1782

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

prestonvasquez
Copy link
Member

@prestonvasquez prestonvasquez commented Apr 2, 2025

DRIVERS-3155

DRIVERS-2711 added unified spec tests that use topologyClosedEvent and topologyOpeningEvent with observeEvents and SDAM expected events. This usage should be reflected in the unified spec tests.

Notably, the expectedSdamEvent portion of the schema was updated to include topologyClosedEvent and topologyOpeningEvent in PR #1556. However, the observeEvents were not updated, thus the schema bump proposed in this PR.

@prestonvasquez prestonvasquez requested review from a team as code owners April 2, 2025 21:46
@prestonvasquez prestonvasquez requested review from alcaeus and W-A-James and removed request for a team April 2, 2025 21:46
@@ -0,0 +1,1161 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

I don't want to scope creep here but it's a bit of a pain to review UTF changes because with each version we create a entirely new file. What if we add a file called "schema-latest.json"? Then we could actually review the changes to schema-latest.json.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ShaneHarvey Good idea: #1792

@alcaeus
Copy link
Member

alcaeus commented Apr 30, 2025

In general I agree with this change. While wondering how this didn't trigger schema validation, I found multiple issues. For one, the SDAM tests weren't validated at all, and upon further investigation I found this affected more specs. I created #1794 to change the makefile and validate all test files.

Even if the SDAM tests were checked, the files would still be valid as we generally don't validate operations. This was to prevent each additional operation from generating a new schema version, which would've made the schema version go even higher than it already is. Had we created a client in a test-level createEntities directive, we could've caught this error.

As for the change itself, upon second thought I don't think this should be a new schema version. I'd rather see this as an errata for version 1.20, as drivers were generally expected to implement these events, and the only issue with them is if they are used in a test-level createEntities directive. This brings me to the last problem: some CMAP events, like serverHeartbeatFailedEvent first appear in schema version 1.17, but they haven't been added to that event name enum either. I would recommend adding them to the 1.17 (and all subsequent schema files) and add an errata to the changelog for this as well.

I have a rough idea for how to validate that event names are added wherever necessary and should have a pull request up for that shortly. That pull request would likely have to add these missing events already, and ideally I'd wait until #1794 is merged so all test files are properly validated. At the same time, I'll add validation or the createEntities operation since the arguments of that one are properly defined already.

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.

3 participants