Don't toggle mongo collection instance variable#433
Conversation
The current MongoUtil implementation has one `mongo_collection` instance variable that can be set to either the jobs collection or the logs collection depending on which method was called last. Methods that don't explicitly choose the collection themselves will use the collection set by whichever method was most recently called, which might be the wrong collection. This commit makes two separate variables and ensures the right collection is being used for each method. It also deletes a number of unused methods.
| :return: | ||
| """ | ||
| self.mongo_collection = mongo_collection | ||
| yield self.pymongoc |
There was a problem hiding this comment.
I've always been pretty confused about this point of this method, since it doesn't really do anything other than return the client. It doesn't actually close the client connection (which it shouldn't, so that's good) and doesn't do any context managing AFAICT. Setting the mongo collection instance variable here is what caused the problem so I decided to just delete the whole thing and simplify.
| return job_ids | ||
|
|
||
| def get_job_log_pymongo(self, job_id: str = None): | ||
|
|
There was a problem hiding this comment.
A lot of this is mostly whitespace changes - it'll be easier to read with whitespace off
| }, | ||
| ) | ||
|
|
||
| def update_jobs_to_queued( |
There was a problem hiding this comment.
Unused, replaced by the single job method above
| inserted = Job.objects.insert(doc_or_docs=jobs_to_insert, load_bulk=False) | ||
| return inserted | ||
|
|
||
| def insert_one(self, doc): |
There was a problem hiding this comment.
All these low level DB methods are unused
| e, "".join(traceback.format_exception(None, e, e.__traceback__)) | ||
| ) | ||
| raise ValueError(error_msg) | ||
| job_col = self.pymongoc[self.mongo_database][self._col_logs] |
There was a problem hiding this comment.
This method, if run when the old mongo_collection variable was set to the jobs collection, could add log information to the job record
There was a problem hiding this comment.
I also assume it'd fail with some sort of null pointer-ish error if it was called before one of the other methods that set mongo_collection had been called.
|
This pull request introduces 1 alert when merging 4c75bda into 18408b2 - view on LGTM.com new alerts:
|
2 participants
Description of PR purpose/changes
The current MongoUtil implementation has one
mongo_collectioninstancevariable that can be set to either the jobs collection or the logs collection
depending on which method was called last. Methods that don't explicitly
choose the collection themselves will use the collection set by whichever
method was most recently called, which might be the wrong collection.
This commit makes two separate variables and ensures the right
collection is being used for each method. It also deletes a number of
unused methods.
Jira Ticket / Github Issue
Testing Instructions
Dev Checklist:
Updating Version and Release Notes (if applicable)