Skip to content

Conversation

prestonvasquez
Copy link
Member

@prestonvasquez prestonvasquez commented Aug 12, 2025

DRIVERS-3261

SERVER-90152 changed the behavior of dropIndex() to return ok:1 instead of an IndexNotFound error when the index does not exist.

This update removes error that would result from IndexNotFound. It also removed empty spaces from the yaml files via yaml language server.

Please complete the following before merging:

  • Update changelog.
  • Test changes in at least one language driver.
  • Test these changes against all server versions and topologies (including standalone, replica set, and sharded
    clusters).

@prestonvasquez prestonvasquez changed the title Remove IndexNotFound error checks for dropIndex DRIVERS-3261 Remove IndexNotFound error checks for dropIndex Aug 12, 2025
@prestonvasquez prestonvasquez marked this pull request as ready for review August 12, 2025 22:48
@prestonvasquez prestonvasquez requested a review from a team as a code owner August 12, 2025 22:48
@prestonvasquez
Copy link
Member Author

@sanych-sun Note that the Go Driver skips all tests in deprecated-options.yml, since this functionality was removed in Go Driver V2.

@prestonvasquez prestonvasquez force-pushed the task/drivers-3261-dropIndex-doesnt-return-IndexNotFound branch from c5720eb to 6868742 Compare August 13, 2025 00:05
@prestonvasquez prestonvasquez force-pushed the task/drivers-3261-dropIndex-doesnt-return-IndexNotFound branch from 750a6bc to 37626b0 Compare August 13, 2025 00:08
@rozza
Copy link
Member

rozza commented Aug 13, 2025

I think this will need to add server versioning as well. Otherwise it error on the old MongoDBs but pass on latest.

Copy link
Member

@sanych-sun sanych-sun left a comment

Choose a reason for hiding this comment

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

LGTM + verified the problem is gone on the latest variant.

@prestonvasquez
Copy link
Member Author

prestonvasquez commented Aug 19, 2025

@ShaneHarvey, @sanych-sun Suggest we remove the following templates since we need to extend the dropIndex operations to include createIndex and generate-basic-tests.py was designed to be single-operation oriented:

For example, the following would be required to support the difference between creating an index for dropIndex v dropIndexes:

{%- if operation.operation_name == "dropIndex" %}
# Create the index first so dropIndex doesn't return IndexNotFound on
# servers prior to 8.3.
- name: createIndex
  object: *collection
  arguments:
    keys: { x: 1 }
    timeoutMS: 100000
    {% for arg in operation.arguments -%}
    {{arg}}
    {%- endfor %}
{%- endif %}
{%- if operation.operation_name == "dropIndexes" %}
# Create the index first so dropIndex doesn't return IndexNotFound on
# servers prior to 8.3.
- name: createIndex
  object: *collection
  arguments:
    keys: { x: 1 }
    name: "x_1"
{%- endif %}

Additionally, generate-basic-tests.py would have to be refactored to avoid hardcoding keys: {x: 1} while creating an index. We would likely have to create a new tuple / object for "Operations" and at that point we've lost generalization.

@jyemin
Copy link
Contributor

jyemin commented Aug 19, 2025

Suggest we remove the following templates since we need to extend the dropIndex operations to include createIndex and generate-basic-tests.py was designed to be single-operation oriented

Can we merge this and deal with the above as follow-on work so that we can unblock drivers whose CI is broken?

@prestonvasquez
Copy link
Member Author

prestonvasquez commented Aug 19, 2025

Can we merge this and deal with the above as follow-on work so that we can unblock drivers whose CI is broken?

@jyemin This will cause the Unified Tests / Check if all JSON test files are up-to-date (pull_request) check to fail for all subsequent PRs, which will block merging. Alternatively, we could do one of the following:

  1. Temporarily implement the template updates required for this ticket. jinja will likely lead to additional deletions via formatting.
  2. Update generate-basic-tests.py to skip the above templates.
  3. Temporarily remove this line from github workflow w/ a follow-up ticket:
    python3 ./source/client-side-operations-timeout/etc/generate-basic-tests.py ./source/client-side-operations-timeout/etc/templates ./source/client-side-operations-timeout/tests

@jyemin
Copy link
Contributor

jyemin commented Aug 19, 2025

OK, I see. What do you recommend, assuming we want to merge something by EOD?

@prestonvasquez
Copy link
Member Author

@jyemin I think the second option is best. It's a very small code change and gives us time to re-assess if we've actually outgrown the template pattern for these cases.

@prestonvasquez prestonvasquez force-pushed the task/drivers-3261-dropIndex-doesnt-return-IndexNotFound branch from 662afd3 to 5cebe3c Compare August 19, 2025 16:49
@prestonvasquez prestonvasquez requested a review from a team as a code owner August 19, 2025 16:49
@prestonvasquez prestonvasquez removed the request for review from a team August 19, 2025 16:49
@prestonvasquez prestonvasquez marked this pull request as draft August 19, 2025 16:51
@prestonvasquez prestonvasquez removed the request for review from katcharov August 19, 2025 16:52
@prestonvasquez prestonvasquez force-pushed the task/drivers-3261-dropIndex-doesnt-return-IndexNotFound branch 2 times, most recently from 74b29ad to 662afd3 Compare August 19, 2025 17:01
@prestonvasquez prestonvasquez force-pushed the task/drivers-3261-dropIndex-doesnt-return-IndexNotFound branch from 662afd3 to e8d8abb Compare August 19, 2025 17:15
@prestonvasquez prestonvasquez marked this pull request as ready for review August 19, 2025 17:18
Copy link
Member

@sanych-sun sanych-sun left a comment

Choose a reason for hiding this comment

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

LGTM

@prestonvasquez prestonvasquez merged commit a6dbd20 into mongodb:master Aug 20, 2025
6 checks passed
@prestonvasquez prestonvasquez deleted the task/drivers-3261-dropIndex-doesnt-return-IndexNotFound branch August 20, 2025 02:36
mdb-ad pushed a commit to mdb-ad/specifications that referenced this pull request Aug 21, 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.

6 participants