Issue #1472 - duplication in primary key (_id) raising an exception now #1485
Conversation
tests/document/indexes.py
Outdated
| User.drop_collection() | ||
|
|
||
| User.objects.create(name='huangz', password='secret') | ||
| def duplicate_primary_key(): |
There was a problem hiding this comment.
Instead of defining an inner function, a more elegant solution is to use a with block like so:
with self.assertRaises(NotUniqueError):
User.objects.create(name='huangz', password='secret2')
tests/document/indexes.py
Outdated
| self.assertEqual(User.objects.count(), 1) | ||
| self.assertEqual(User.objects.get().password, 'secret') | ||
|
|
||
| User.drop_collection() |
There was a problem hiding this comment.
You can remove this line. The rule of thumb we use in this repo is that all unit tests should call drop_collection immediately after defining a document (as you do above), so there's no need for this redundant call.
tests/document/indexes.py
Outdated
| """Create a new record with a duplicate primary key | ||
| throws an exception | ||
| """ | ||
|
|
There was a problem hiding this comment.
This empty line is not necessary.
There was a problem hiding this comment.
Thank you very much!
Just one question regarding the changed you requested - I have based my code on the previously existing function, test_unique_and_primary, and that function might require the same changes to be done:
- getting rid of inline function in favor of "with"
- removing the empty line after the comment
- removing the redundant drop_collection call (it also exists in test_unique_and_indexes, for example)
Do you want me to do the above changes on those functions (which are not mine) also? It is no problem at all, I just wanted to be sure :)
There was a problem hiding this comment.
That's a good question @ephraimberkovitch. It's basically a tradeoff between PR isolation (where we want to touch solely the parts of the code that are logically related to the particular problem we're solving) vs passive cleanup. Personally I like applying cosmetic tweaks to unrelated code as I go as long as it doesn't make the PR considerably less readable. Otherwise the likelihood of going back and tweaking it in a separate PR are quite low.
Your PR in its current form is still readable and the cleanup you did is very welcome!
There was a problem hiding this comment.
This is great @ephraimberkovitch, thank you for taking care of it! I requested a few minor changes, but overall I'm happy with your solution.
2 participants
The SAVE function of pymongo has been deprecated.
It does not raise an exception in case of duplication in value of _id, because in this case it treats the operation as UPDATE and not INSERT, and practically, does not do anything - not really duplicates the document, but also does not raise an exception
I stopped using SAVE and started using INSERT_ONCE instead, as recommended by pymongo