feat: most generic single item writer for write_versioned API #27
Conversation
| @@ -1,3 +1,9 @@ | |||
| ## 1.16.0 | |||
|
|
|||
| - `write_item` single item write helper for the `write_versioned` | |||
There was a problem hiding this comment.
When would you recommend we use this or not use it? I think I would lean toward using it all the time.
There was a problem hiding this comment.
so, i've added a bit of documentation to it. but i also had a long conversation with Josh about it, and let me see if I can summarize.
This is super generic, which means that it's not going to be the best choice for cases where you want the narrower, more specific behavior offered by the other 3 existing helpers.
As an example - if you actually want to raise an exception when somebody tries to update a job that literally doesn't exist, then you should definitely use update_existing - it's the only one that promises it'll treat that situation as an exceptional case.
Similarly, I would advise using create_or_update if you definitely never intend to delete the item - as you can see, returning None will actually cause deletion, and it's actually quite common in our systems to never hard delete items. So don't use one where the type signature would allow you to delete the item if you never intend to be deleting it.
Overall, I would say this should be used incredibly rarely at XOi. I have introduced it because I do have a specific case where I wanted to be able to fully unit test some existing hard-deletion code. And there would be other circumstances where we might theoretically want to do hard deletions. But in order of more narrow to more general, I'd order these 4 helpers:
- update_existing
- update_if_exists
- create_or_update
- write_item
And I would suggest we always pick the narrowest one that is applicable to our situation. I think that makes our code simpler even if it's not as easy as just always picking the same helper off the tree.
|
|
||
| def test_api2_write_item_creates(): | ||
| vt, table = _fake_table("felicity", "id") | ||
| key = dict(id="goodbye") |
| @@ -168,3 +176,20 @@ def create_or_update_trans(vt: VersionedTransaction) -> VersionedTransaction: | |||
| return table.put(creator_updater(table.get(key)(vt)))(vt) | |||
|
|
|||
| return create_or_update_trans | |||
|
|
|||
|
|
|||
| def write_item( | |||
There was a problem hiding this comment.
I wonder if it would be surprising to someone to see that a function called write_item will delete the item if the updater returns None.
I'm trying to think if there is a better name, but I can't really think of one. put_or_delete_item maybe. That's a lot longer than what you have, though.
Maybe just something in the docstring that calls that out would be good enough?
There was a problem hiding this comment.
Just playing devil's advocate but wouldn't you still technically be writing the item to the database just doing a soft delete?
There was a problem hiding this comment.
actually, this is a super important point to bring up.
DynamoDB explicitly uses the word write to describe the set of "both puts and deletes", as you can see here and elsewhere. https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_BatchWriteItem.html
I think it's best that we try to capture their terminology with ours even in these higher level abstractions. I've tried to honor their terms in the past, and this is just an extension of that.
I agree that delete isn't what I necessarily think of first when I see the word write, but ultimately I think it's best to hew closer toward what the words mean in the underlying system, rather than inventing my own subtly different meanings for their own words.
There was a problem hiding this comment.
Crazy. This wasn't here when I wrote my comment, very much along the same lines.
@jstephensxoi -- no, this would do a hard delete, since a soft delete requires an update to the record to reflect the deleted_at and deleted_by values...
There was a problem hiding this comment.
(i think you may have experienced the weirdness of git force-push here, Nathan. it definitely wasn't there before. :) )
| @@ -168,3 +176,20 @@ def create_or_update_trans(vt: VersionedTransaction) -> VersionedTransaction: | |||
| return table.put(creator_updater(table.get(key)(vt)))(vt) | |||
|
|
|||
| return create_or_update_trans | |||
|
|
|||
|
|
|||
| def write_item( | |||
There was a problem hiding this comment.
This is a nice clean implementation, and I like the idea of being able to delegate the decision of whether to update or delete to the writer callable. Does write_item clearly communicate that this will delete the record when None is returned?
I might be overthinking this, but the versioned_diff_update code lets you return None when no change is desired and that, combined with this being the most general name in the file, might result in someone making some bad assumptions and deleting records...
It might be worth adding coverage of the None behavior in the """ doc (or possibly renaming the function, but "write" is a nice common term for all of the included behavior, so I definitely see the merits...)
There was a problem hiding this comment.
i've added more verbose documentation around this to the helper - please let me know what else we can add, because I agree that we don't want people accidentally deleting things they don't mean to.
In retrospect, I feel the behavior of versioned_diffed_update_items is one of those premature optimizations for DX - I thought it was 'cute' that returning None could mean "do nothing", but that causes a big problem with regard to composability - you can't transparently compose multiple different updaters if one of them returns None to mean "change nothing", because then the next function gets nothing to update. It was an explicit choice to start using None to represent "item does not exist in the table" in this new API, precisely because I wanted to help us make sure we kept everything maximally composable. But documenting this more clearly would be very valuable.
petergaultney
force-pushed
the
feat/single-item-write
branch
from
92515c6
to
2486f6a
Compare
| """The most general purpose single-item-write abstraction, with | ||
| necessarily weak type constraints on the writer implementation. | ||
|
|
||
| Note that in DynamoDB parlance, a write can be either a Put or a |
There was a problem hiding this comment.
Excellent. Yes. I see the value of sticking close to the Dynamo terminology. This makes that very clear. Thank you.
Overall, I would say this should be used incredibly rarely at XOi. I have introduced it because I do have a specific case where I wanted to be able to fully unit test some existing hard-deletion code. And there would be other circumstances where we might theoretically want to do hard deletions. But in order of more narrow to more general, I'd order these 4 helpers:
And I would suggest users of this library always pick the narrowest one that is applicable to our situation. I think that makes our code simpler even if it's not as easy as just always picking the same helper off the tree.