community[patch]: update dynamodb chat history to update instead of overwrite #22397
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
dosubot
bot
added
size:L
Davi-Schumacher
force-pushed
the
fix-dynamodb-chat-history-put-overwrite
branch
3 times, most recently
from
fc4c447 to
98023e1
Compare
|
Hi @eyurtsev, can I get some guidance on what it would take to get this PR in shape for review/approval? |
Davi-Schumacher
force-pushed
the
fix-dynamodb-chat-history-put-overwrite
branch
from
baa221b to
df863f2
Compare
…ead of put_item this allows someone to use the same chat session record to store additional attributes by updating the History attribute without overwriting the entire record
Davi-Schumacher
force-pushed
the
fix-dynamodb-chat-history-put-overwrite
branch
from
df863f2 to
ebe1485
Compare
eyurtsev
reviewed
Dec 11, 2024
|
@Davi-Schumacher looks good, could you confirm whether we need to escape any of the values to avoid sql injection? |
eyurtsev
approved these changes
Dec 11, 2024
dosubot
bot
added
the
lgtm
eyurtsev
changed the title
eyurtsev
added
the
waiting-on-author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
The current implementation of
DynamoDBChatMessageHistoryupdates theHistoryattribute for a given chat history record by first extracting the existing contents into memory, appending the new message, and then using theput_itemmethod to put the record back. This has the effect of overwriting any additional attributes someone may want to include in the record, like chat session metadata.This PR suggests changing from using
put_itemto usingupdate_iteminstead which will keep any other attributes in the record untouched. The change is backward compatible sinceupdate_itemis an "upsert" operation, creating the record if it doesn't already exist, otherwise updating itDependencies:
None
Tests and docs:
No unit tests currently exist for the
DynamoDBChatMessageHistoryclass. This PR adds the filelibs/community/tests/unit_tests/chat_message_histories/test_dynamodb_chat_message_history.pyto test theadd_messageandclearmethods. I wanted to use the moto library to mock DynamoDB calls but I could not get poetry to resolve it so I mocked those calls myself in the test. Therefore, no test dependencies were added.The change was tested on a test DynamoDB table as well. The first three images below show the current behavior. First a message is added to chat history, then a value is inserted in the record in some other attribute, and finally another message is added to the record, destroying the other attribute.
The next three images show the new behavior. Once again a value is added to an attribute other than the History attribute, but now when the followup message is added it does not destroy that other attribute. The History attribute itself is unaffected by this change.
The doc located at
docs/docs/integrations/memory/aws_dynamodb.ipynbrequired no changes and was tested as well.