-
Notifications
You must be signed in to change notification settings - Fork 43
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
Added update_metadata() to write_ adapters #716
Added update_metadata() to write_ adapters #716
Conversation
|
||
class DocumentRevision(BaseDocument): | ||
revision: int | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be useful to add a classmethod constructor here to do what you were trying to do in __init__
.
@classmethod
def from_document(cls, document)
return cls(key=document.key, ...)
@@ -62,8 +63,9 @@ def inner(self, *args, **kwargs): | |||
class WritingArrayAdapter: | |||
structure_family = "array" | |||
|
|||
def __init__(self, collection, directory, doc): | |||
def __init__(self, collection, revisions, directory, doc): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to pass in the database
here rather than separately passing in each of its collections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in the latest commit
updated_at = datetime.now(tz=timezone.utc) | ||
self.doc.updated_at = updated_at | ||
|
||
if len(metadata) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I want to update metadata
to be empty {}
or specs
to be empty []
, shouldn't that update be processed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in the latest commit
) | ||
|
||
if result.matched_count != result.modified_count: | ||
raise ValueError("Error while writing to database") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest classifying this as a RuntimeError
.
5583ff5
to
0518b1d
Compare
Now that we are adding indexes, I think we should also add an index to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems on track. A couple comments. More tests would be good, too.
def __len__(self): | ||
return self._collection.count_documents( | ||
{"key": self._key} | ||
) # maybe wrong MongoDB usage here... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete comment (assuming this usage is now correct).
{"key": self._key} | ||
) # maybe wrong MongoDB usage here... | ||
|
||
def __getitem__(self, item_): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks mixed up and likely needs testing.
The usage r[i:j]
should lead to skip(offset).limit(j - i)
. The usage r[i:]
or r[i:None]
or r[:-1]
(all equivalent) should lead to skip(offset)
with no limit. Pymongo also accept skip(offset).limit(0)
where 0
means "no limit", which is an option if you find it leads to cleaner code.
if now > self.deadline: | ||
self._doc = Document( | ||
**self.collection.find_one({"key": self.key}) | ||
) # run query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems superfluous. :-)
def create_indexes(self): | ||
self.revision_coll.create_index( | ||
[("key", pymongo.ASCENDING), ("revision", pymongo.DESCENDING)], unique=True | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're creating indexes, we should also create an index on the nodes
collection to ensure that key
is unique.
This PR started as an approach to add more information to the document schema that will help to keep track of all the updates made to the metadata and specs of the samples.
In addition, we implemented a revisions system with the mongo database that will keep track of old versions of documents. Every time that
update_metadata()
is run, the active document that is saved in collections is copied to revisions where every entry is protected by the samekey
id plus arevision
number. There two parameters is used as a unique identifier for every entry.