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

Reindex on change of unique or sparse #21

Conversation

SimonSimCity
Copy link
Member

@SimonSimCity SimonSimCity commented Jan 5, 2018

This fixes #19 and requires #20 to be merged first.

The only downside is, that this script doesn't handle a changed definition from { name: 'foo', unique: true } to { name: 'foo', unique: false }. It will not delete the unique index of this property, neither will it re-create the index without uniqueness required.

Should I change the cod so it removes such an index?

@aldeed
Copy link
Collaborator

aldeed commented Jan 25, 2018

@SimonSimCity This is good and can be merged after a few things.

  1. Can you rebase on master now that I merged your other PR?
  2. Can you add a few tests to show that this works in different circumstances? I do not think this will work if you have unique: true without index: true and then you remove unique or set to false because it is wrapped in if ('index' in definition || definition.unique === true). The conditional logic will probably have to be tweaked a bit.
  3. As long as you're going to do this for unique and sparse, might as well make it work when you remove index without setting it to false, right? The correct approach to make it all work might be to loop through the existing indices before the schema loop, and if any don't match their schema, drop them. Then do the current loop through the schema to re-add them.

@aldeed
Copy link
Collaborator

aldeed commented Jan 25, 2018

We would also need to be careful that we don't drop existing indices added outside of this package. When adding them to existingIndexes, you should check first to make sure that index.name starts with c2_. Add a test for this, too.

@SimonSimCity
Copy link
Member Author

Sorry for it taking soo long time for me to get back to you here ...

I've now rebased it on master and tried to run the tests, but wasn't able to. The command, stated in the README.md (npm test:watch or npm test) resulted in an error and calling meteor test-packages ./package/indexing/ - which was my next guess, since this is a meteor package - left me without any tests. Can you please help me in getting this going?

@SimonSimCity
Copy link
Member Author

@aldeed could you please help me to run the tests?

@StorytellerCZ StorytellerCZ deleted the branch Meteor-Community-Packages:master January 9, 2024 17:23
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.

Doesn't re-create the index if an index-setting (like unique) changes
3 participants