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

PYMODM-68 defer index creation #34

Merged
merged 5 commits into from Mar 6, 2018

Conversation

dpercy
Copy link
Contributor

@dpercy dpercy commented Mar 2, 2018

Here's one solution to PYMODM-68: instead of creating the indexes when the class is created, we could create them when the model's collection is first used. I think this is similar to what MongoEngine does.

Another option would be to give the user the option to disable automatic index creation, like MongoEngine's auto_create_index=False.

@@ -105,5 +105,8 @@ class Meta:
class ChildModel(ModelWithIndexes):
pass

# force connection
ModelWithIndexes._mongometa.collection
Copy link
Member

Choose a reason for hiding this comment

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

How come this change is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The index creation happens when you access the collection property, instead of when the model class is created. So without this line, the assertion about the index existing will fail.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. I think this should be updated to be a little more explicit:

         index_info = DB.model_with_indexes.index_information()
         self.assertNotIn('product_id_1_name_1', index_info)

         # Indexes are only created once the Model's collection is accessed.
         ModelWithIndexes._mongometa.collection

         index_info = DB.model_with_indexes.index_information()
         self.assertTrue(index_info['product_id_1_name_1']['unique'])

         # Creating indexes on the child should not error.
         ChildModel._mongometa.collection

         # Index info should not have changed.
         final_index_info = DB.model_with_indexes.index_information()
         self.assertEqual(index_info, final_index_info)

if self.indexes and not self._indexes_created:
coll.create_indexes(self.indexes)
self._indexes_created = True
return coll
Copy link
Member

Choose a reason for hiding this comment

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

Neat this is much simpler than I imagined.

self.assertFalse(client._topology._opened)

self.assertEqual(Article.objects.count(), 0)
self.assertTrue(client._topology._opened)
Copy link
Member

Choose a reason for hiding this comment

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

This is very clever but I think we can test the same thing without using the private MongoClient api. With the SDAM monitoring api instead:

from pymongo.monitoring import ServerHeartbeatListener

class HeartbeatStartedListener(ServerHeartbeatListener):
    def __init__(self):
        self.results = []

    def started(self, event):
        self.results.append(event)

    def succeeded(self, event):
        pass

    def failed(self, event):
        pass

heartbeat_listener = HeartbeatStartedListener()
connect('mongodb://localhost:27017/foo',
        'foo-connection',
         connect=False,
         event_listeners=[heartbeat_listener])
...

self.assertEqual(len(heartbeat_listener.results), 0)
self.assertEqual(Article.objects.count(), 0)
self.assertGreaterOrEqual(len(heartbeat_listener.results), 1)

self.assertFalse(client._topology._opened)

self.assertEqual(Article.objects.count(), 0)
self.assertTrue(client._topology._opened)
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here. We should also add a createIndexes command listener to make sure the createIndexes is run:

from pymongo.monitoring import CommandListener

class WhiteListEventListener(CommandListener):
    def __init__(self, *commands):
        self.commands = set(commands)
        self.results = defaultdict(list)

    def started(self, event):
        if event.command_name in self.commands:
            self.results['started'].append(event)

    def succeeded(self, event):
        if event.command_name in self.commands:
            self.results['succeeded'].append(event)

    def failed(self, event):
        if event.command_name in self.commands:
            self.results['failed'].append(event)

heartbeat_listener = HeartbeatStartedListener()
create_indexes_listener = WhiteListEventListener('createIndexes')
connect('mongodb://localhost:27017/foo',
        'foo-connection',
         connect=False,
         event_listeners=[heartbeat_listener, create_indexes_listener])
...

self.assertEqual(len(heartbeat_listener.results), 0)
self.assertEqual(len(create_indexes_listener.results['started']), 0)
self.assertEqual(Article.objects.count(), 0)
self.assertGreaterOrEqual(len(heartbeat_listener.results), 1)
self.assertEqual(len(create_indexes_listener.results['started']), 1)

@ShaneHarvey
Copy link
Member

The only (minor) downside I can find to this approach is if createIndexes fails or the user defines some invalid index spec then the error will start being thrown when the connection is first used. I think that's an acceptable trade-off.

Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @dpercy!

@ShaneHarvey ShaneHarvey merged commit 84ff4e9 into mongodb:master Mar 6, 2018
@dpercy
Copy link
Contributor Author

dpercy commented Mar 6, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants