fix: Use model_validate instead of model_copy for EventActions deserialization #3635
Conversation
…alization Fixes google#3633 ## Problem When using DatabaseSessionService, nested Pydantic models in EventActions (specifically EventCompaction) were incorrectly deserialized as dictionaries instead of proper Pydantic model instances. This caused AttributeError when trying to access attributes on the compaction object. ## Root Cause In database_session_service.py line 364, the code used: EventActions().model_copy(update=self.actions.model_dump()) Pydantic's model_copy() does not validate or reconstruct nested Pydantic models - it just assigns dictionary values directly. This meant that nested objects like EventCompaction remained as dicts after deserialization. ## Solution Changed line 364 to use model_validate() instead: EventActions.model_validate(self.actions.model_dump()) if self.actions else None model_validate() properly reconstructs nested Pydantic models from dictionaries, ensuring EventCompaction and other nested models are instantiated correctly. ## Testing Added comprehensive test test_event_compaction_deserialization that: - Creates an event with EventCompaction - Persists it to database - Retrieves it and verifies compaction is an EventCompaction instance - Tests with all three session service types (InMemory, Database, SQLite) All 45 session service tests pass, including the new test. ## Impact - Fixes EventCompaction deserialization bug in DatabaseSessionService - Also fixes the same issue in SqliteSessionService - No impact on InMemorySessionService (it stores objects directly) - Prevents AttributeError in ADK's own code (contents.py:265, compaction.py:117)
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @fhuzjan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical deserialization bug where nested Pydantic models within Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Response from ADK Triaging Agent Hello @fhuzjan, thank you for creating this PR! It looks like you have not signed the Contributor License Agreement (CLA). Please visit https://cla.developers.google.com/ to sign it. This is a necessary step before we can review your contribution. Thanks! |
adk-bot
added
the
services
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a great pull request that clearly addresses a deserialization bug with nested Pydantic models. The problem, root cause, and solution are well-documented in the description. The fix to use model_validate instead of model_copy is correct and effectively resolves the issue. The addition of the test_event_compaction_deserialization test is excellent, as it provides comprehensive coverage for the fix across all session service types. I have one minor suggestion to improve the code formatting for better readability.
| actions=EventActions.model_validate(self.actions.model_dump()) | ||
| if self.actions | ||
| else None, |
There was a problem hiding this comment.
For better readability and conciseness, this conditional expression can be formatted as a standard one-line ternary operator.
actions=EventActions.model_validate(self.actions.model_dump()) if self.actions else None,There was a problem hiding this comment.
Script autoformat.sh rewrites this into multiple lines
|
Hi @fhuzjan , Thank you for your contribution! We appreciate you taking the time to submit this pull request. |
@ryanaiagent thank you for the reply, I have made and pushed the changes using |
3 participants
fix: Use model_validate instead of model_copy for EventActions deserialization
Fixes #3633
Problem
When using DatabaseSessionService, nested Pydantic models in EventActions
(specifically EventCompaction) were incorrectly deserialized as dictionaries
instead of proper Pydantic model instances. This caused AttributeError when
trying to access attributes on the compaction object.
Root Cause
In database_session_service.py line 364, the code used:
EventActions().model_copy(update=self.actions.model_dump())
Pydantic's model_copy() does not validate or reconstruct nested Pydantic
models - it just assigns dictionary values directly. This meant that nested
objects like EventCompaction remained as dicts after deserialization.
Solution
Changed line 364 to use model_validate() instead:
EventActions.model_validate(self.actions.model_dump()) if self.actions else None
model_validate() properly reconstructs nested Pydantic models from
dictionaries, ensuring EventCompaction and other nested models are
instantiated correctly.
Testing
Added comprehensive test test_event_compaction_deserialization that:
All 45 session service tests pass, including the new test.
Impact