Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2069 +/- ##
==========================================
+ Coverage 80.50% 80.91% +0.41%
==========================================
Files 84 85 +1
Lines 7909 8189 +280
==========================================
+ Hits 6367 6626 +259
- Misses 1542 1563 +21
🚀 New features to boost your workflow:
|
| """A mapping from block ID to block data.""" | ||
|
|
||
| display_order: list[str] = Field([]) | ||
| """The order in which to display block data in the UI.""" |
There was a problem hiding this comment.
From #1936 (comment):
@davidwaroquiers:
"Not sure I like it to have HasBlocks moved here somehow. It could be possible to move DataBlockResponse to another module (e.g. blocks_response.py or something) and keep HasBlocks in traits.py.
Not sure if it's worth. Maybe there is another solution also ? Maybe @ml-evs has an idea/preference or can comment on this."
@gpetretto:
"Indeed moving DataBlockResponse should be an option to avoid the circular import. I think this is a matter of preference."
| """The block payload, exactly as produced by `DataBlock.to_db()`.""" | ||
|
|
||
| version: int = 0 | ||
| """The latest committed version number of this block in `block_versions`. |
There was a problem hiding this comment.
From #1936 (comment)
@davidwaroquiers
"Wouldn't it be better to somehow follow a logic similar to items ? i.e. having a counter in the version_counter collection based on the immutable_id of the block ? What do you think ? I am actually a bit confused about the version that is in the item and the counter that is in the version_counter in the end. It seems they are always the same (version in item and counter in the corresponding document in version_counter). Probably good to clarify this with @ml-evs
After our discussion and regarding the logic of having version = 0 here, I propose we also discuss this with @ml-evs
Somehow we keep exactly the same behavior as it is currently but I have the feeling that we may want to change that somehow (to a better, more consistent behavior).
Behavior:
When we add a block, it already adds the block directly in the item without any save.
What I would expect (to be discussed of course :):
When we add a block in the item, it will add the block only when we save the item."
@gpetretto
"indeed this is partly to keep exactly the symmetry with what the blocks are doing now.
Other ways of achieving the same would be possible, but maybe a bit more involved on the side of the code. Or maybe it could also be possible to alter the general behaviour of the backend. I did not verify if this would require any change in the routes."
| (``{"immutable_id": ...}``) rather than a legacy embedded block. | ||
| """ | ||
| return ( | ||
| isinstance(blocks_obj_value, dict) |
There was a problem hiding this comment.
From #1936 (comment)
@davidwaroquiers
"Can it happen that "entry" is not a dict ? And if we assume it can, then this "non dict" object is then considered as a legacy embedded block. What would/could happen then ?"
@gpetretto
"in general it should not. Since this is used in different places it is a protection against what comes in and does risk giving internal errors. I can remove the check or put a warning. Any preference?"
| block_immutable_id=immutable_id, | ||
| block_id=block_doc["block_id"], | ||
| version=next_version, | ||
| timestamp=datetime.datetime.now(tz=datetime.timezone.utc), |
There was a problem hiding this comment.
From #1936 (comment)
@davidwaroquiers
"Why not using _now_isoformat ?"
@gpetretto
"Because timestamp in BlockVersion is a datetime, like in ItemVersion, while _now_isoformat is used to fill the last_modified attribute of Block which comes from Entry and is of type IsoformatDateTime."
| { | ||
| "$push": {"display_order": display_order_update}, | ||
| "$set": {f"blocks_obj.{block.block_id}": block.to_db()}, | ||
| "$set": {f"blocks_obj.{block.block_id}": {"immutable_id": block_immutable_id}}, |
There was a problem hiding this comment.
From https://github.com/datalab-org/datalab/pull/1936/changes#r3638903276
@davidwaroquiers
"Why not defining the id then do a find_one_and_update with the permissions, followed by the create_block_document ? In that case there is one less query to the items collection. The drawback is that then if the block creation fails we have a dangling reference to a block ? I guess that's why you did it this way ? Maybe a transaction would also be useful here."
@gpetretto (replying here)
"Indeed this would avoid the dangling reference. A transaction can be considered, but not sure if it is worth"
| ) | ||
|
|
||
| block_was_present = stored_blocks_obj_value is not None or ( | ||
| doc_before is not None and block_id in (doc_before.get("display_order") or []) |
There was a problem hiding this comment.
From #1936 (comment)
@davidwaroquiers
"Can this happen that the block was not in the list of blocks of the items but it was in the display order ? That seems like a case where the block actually does not exist and cannot be deleted, just that there was some kind of bug or weird thing happening to the display order at some point."
@gpetretto
"This would keep the equivalence with the original procedure. That one did find_one and the query was addressing the display_order. So in the case where the block_id was missing, but still in the display_order the document would have resulted as modified, returning a 200. Without this condition, instead the new version would return a 400. Here I switched to find_one_and_update to save one action on the DB, but then still tried to reproduce exactly the same logic. It can be questioned if the logic is correct (and properly preserved also...)"
| # the payload from the `block_versions` and becomes the new current `blocks` | ||
| # state, plus a new RESTORED `block_versions` entry. | ||
| restored_block_pins: dict[str, dict] = {} | ||
| if "blocks_obj" in restored_data: |
There was a problem hiding this comment.
From #1936 (comment)
@davidwaroquiers
"Not sure the if is needed here as the loop anyway uses "or {}""
@gpetretto
"Removing it would have the impact on the second loop, that instead works on current_item. Again, this is a case that in general should not happen, but it is a cheap check. Can be considered for removal."
| restored_data["display_order"] = [ | ||
| b for b in restored_data["display_order"] if b != block_id | ||
| ] | ||
| continue |
There was a problem hiding this comment.
From #1936 (comment)
@davidwaroquiers
"As this should not happen. I'm wondering if it should not be a BadRequest or something. It seems that if it happens it is a bug (from somewhere else) no ? Then if we "just" log it and drop, we may not discover it while using. What do you think ?"
@gpetretto
"Simply raising here in the middle of the loop would risk leaving the db in an inconsistent state. One would probably need to make a full check before and raise if one block is missing, and if not proceed with the update. Do you think it would be better?"
db7dd04 to
0637694
Compare
1 participant
Reopening #1936 after the pydantic 2 branch has been deleted. Note that, aside from the addressed comments, this also introduces a separate counter for the blocks versions that was not present in #1936.
Reporting below in text of the initial PR and I will copy/paste the comments that were not resolved there.
Implementing the separation of blocks from the items as discussed in #48 (closes #48)
They key principles followed for the implementation are those outlined in #48 (comment) (copy/pasted below for convenience)
Initial planning
No new functionality. The initial PR focuses exclusively on preserving existing functionalities with the blocks separated from the items. Some choices for the document structure may be driven by functionalities that are planned in the near future, but no additions here.
Blocks as separated entities. blocks are similar to
File: the new block Model has parentsEntry + HasOwner + HasRevisionControl.Two collections:
data_blocks+block_versions. Similar toitems/item_versions. An item will always point to the document in thedata_blockscollection (i.e. the latest block version). Document initem_versionspoints to documents inblock_versions.Items collection: the
blocks_objin theitemscollection can contain either the full block for backward compatibility or a dictionary with{"immutable_id": ObjectId}. The order will still be based ondisplay_order. This is preparatory for the subsequent steps in which the blocks_objanddisplay_orderwill be replaced by{"blocks": [{"immutable_id": ...}]to fit the suggested pattern ({"type": "my-item", "blocks": [{"immutable_id": }], "files": [{"immutable_id": }]}`).No reference to the items in the block document. Can be added in the future if needed.
Versioning. Like in the current data structure, modifying a block will not automatically trigger the creation of a new block version in
block_versions, but will just modify the document indata_blocks. A new version of the block inblock_versionswill be triggered by an item save, but only if the content of the block has changed from the previous block version stored, to avoid duplicated entries.Restore. Same mechanism as items. Restoring an old item version writes the old block content back in the current state + a new history entry. At this stage this can happen only when restoring an Item entirely.
No changes to File ownership. Most blocks are associated to a file. Association will remain the same.
No changes to the frontend API. Responses still expose
blocks_obj+display_order, reassembled server-side fromdata_blocks.Block deletion. As in the current implementation, if a block is deleted it will be removed from the
data_blockscollection, but preserved in theblock_versionsin case it needs to be restored. Block deletion triggered only when removed from an item. If an Item is deleted, the corresponding blocks indata_blocksare deleted (not those inblock_versions, mimicking what happens for items). This can revised in the future if a block can be shared.Collections.
CollectionisHasBlocks, but if I am not mistaken there is really no place where a block can be assigned to a collection at the moment in the UI, and it has no versioning. We keep the same structure as Items, but there should be no need to handle backward compatibility.A few additional points to be specified concerning the details of the implementation:
versionattribute of the block starts at0and becomes1when the item is saved.Note that I used the pydantic2 branch as a base for the implementation, as it seems reasonable as base, given the involvement of the backend. I targeted the PR to that branch, but the idea is to merge this in main after the pydantic2 branch has been merged.