Skip to content

DRIVERS-3023 rename test operation to avoid clash with existing op #1793

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

Closed
wants to merge 1 commit into from

Conversation

abr-egn
Copy link
Contributor

@abr-egn abr-egn commented Apr 29, 2025

This renames the gridfs rename operation from "rename" to "renameById" (consistent with the existing "renameByName") to avoid conflicting with the existing change streams "rename" test operation. AFAICT the only driver that picked up the tests with the original name is PHP so churn should be minimal.

@abr-egn abr-egn marked this pull request as ready for review April 29, 2025 18:06
@abr-egn abr-egn requested a review from a team as a code owner April 29, 2025 18:06
@abr-egn abr-egn requested review from dariakp and GromNaN and removed request for a team April 29, 2025 18:06
@alcaeus
Copy link
Member

alcaeus commented Apr 30, 2025

@abr-egn what's the reason for renaming this? The GridFS operation is called rename in the spec, and the rename wording for the test operation has been there since the beginning of the unified test format. Furthermore, we already have a rename operation for the collection entity, so I'm not sure what conflict would occur. Generally, operation names have to be unique for a single entity type, but not across entity types. Other examples for this include the aggregate operation, which exists on the client, database, and collection entity types. As such, I see no reason to change this one operation name and making it inconsistent with the actual operation on a GridFS bucket. One could make an argument for renaming the rename operation to renameById in the GridFS spec itself for consistency with renameByName, but I believe that is a wholly separate discussion.

You also mention the rename operation for change streams, is this something you are adding?

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

AFAICT the only driver that picked up the tests with the original name is PHP so churn should be minimal

We implement the method GridFS::rename() since 2017; as it is specified.
You can't change this test without changing the spec.

@abr-egn
Copy link
Contributor Author

abr-egn commented Apr 30, 2025

Sorry, I misspoke; I did mean the collection entity rename operation (currently used in change streams tests, hence the confusion).

To be honest I didn't anticipate that keeping the name of the method as defined in the API spec and the test operation the same would be as important as it clearly is - I saw keeping the API spec name as rename and updating the test operation name to renameById to be a quick and easy way to save work across drivers and since that's no longer at all clear there's no need for this change.

For context, the Rust unified test runner parses the test files ahead of time into a vector of strongly-typed test operations, including argument structs for each operation. The addition of the test files in DRIVERS-3023 introduced an ambiguity with rename where the shape of the expected arguments is not statically determined by the name of the operation (in contrast with aggregate, for example, which always has the same argument shape no matter which entity type it's targeting). I saw that and had the impulse that other drivers might do the same ahead-of-time parsing as Rust, and so it might be kind to save aggregate work by renaming a few strings rather than requiring extra test runner logic across multiple drivers.

Anyway, this isn't a blocker for Rust; I've implemented indirection to deal with the ambiguity in mongodb/mongo-rust-driver#1366 and I expect if any other drivers run into this they can do similar.

@abr-egn abr-egn closed this Apr 30, 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.

3 participants