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

[Feature] Schema#removeIndex(...) #11547

Closed
AbdelrahmanHafez opened this issue Mar 20, 2022 · 4 comments · Fixed by #11606
Closed

[Feature] Schema#removeIndex(...) #11547

AbdelrahmanHafez opened this issue Mar 20, 2022 · 4 comments · Fixed by #11606
Assignees
Labels
new feature This change adds new functionality, like a new method or class
Milestone

Comments

@AbdelrahmanHafez
Copy link
Collaborator

I came across a case where I cloned a schema, and would like to reuse it in another model without the indexes.
The issue is that the cloned schema includes its indexes, and there is no official way to remove indexes, I had to access schema._indexes which doesn't work with TS.

const cartSchema = new Schema({ customerId: String, products: [productSchema]  });
cartSchema.index({ customerId: 1 }, { unique: true });

const cartSchemaClone = cartSchema.clone();

cartSchemaClone._indexes = []; // there is no other way to do it.

const checkoutSchema = new Schema({ cart: cartSchemaClone });
@AbdelrahmanHafez AbdelrahmanHafez added the new feature This change adds new functionality, like a new method or class label Mar 20, 2022
@AbdelrahmanHafez
Copy link
Collaborator Author

Turns out cartSchemaClone._indexes = []; doesn't really remove all indexes, it only removes the top-level paths indexes and ignores nested paths indexes.

The workaround that I found for removing all indexes:

const cartSchemaClone = cartSchema.clone();
cartSchemaClone.set('excludeIndexes', true); // ignore _all_ indexes
const checkoutSchema = new Schema({ cart: cartSchemaClone });

This fixes my issue, but would become a problem once I need to add some indexes. I think we need to provide:
Schema#clearIndexes and Schema#removeIndex.

@vkarpov15 vkarpov15 added this to the 6.3 milestone Mar 28, 2022
@AbdelrahmanHafez
Copy link
Collaborator Author

@vkarpov15
I can imagine the API for Schema#clearIndexes();
How do you think the API for Schema#removeIndex(): should look like?
One path can have multiple indexes, one option would be to make const indexId = schema.index({ status: 1 }); return an indexId which is a randomoy generated string, we can assign that id as metadata in schema._indexes, and allow users to remove using those ids.

Admittedly, this is a hacky solution that I can see causing issues for users thinking that this identifier representing something on MongoDB server, but I am not sure what other way we can reliably identify indexes in our schemas.

Another way would be to identify using the object reference by strict equality, that way we make it the dev's responsibility to find the desired index from Schema#indexes();

@vkarpov15
Copy link
Collaborator

vkarpov15 commented Mar 31, 2022

We should support both schema.removeIndex('my-index-name') and schema.removeIndex({ status: 1 }) . The latter should remove any index that is deep equal to the given spec.

Also schema.clearIndexes() should remove all indexes defined on the schema. Shouldn't have any impact on indexes in MongoDB

@AbdelrahmanHafez
Copy link
Collaborator Author

I agree that removing an index from schema shouldn't touch MongoDB.

For deep equality check, I think we should check both the index definition and the options. Also, deep equality check should treat objects with the same properties in different order as equal.

For string index names, I am not sure if they're unique or not, one other concern, AFAIK people can set a custom index name, if we wanna match with the index name, does that mean we'll need to fetch indexes from the database? Needing to fetch indexes means we'll have to tie the index removal with database connections.

I think supporting string index names, knowing that users can set custom indexes names, will introduce more issues than the feature requires, deep equality check should do the trick.

@IslandRhythms IslandRhythms linked a pull request Mar 31, 2022 that will close this issue
vkarpov15 added a commit that referenced this issue Apr 10, 2022
vkarpov15 added a commit that referenced this issue Apr 10, 2022
gh-11547 Can remove and clear indexes on the schema
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature This change adds new functionality, like a new method or class
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants