feat: Add DatabaseSessionService with Hibernate and Flyway support#654
feat: Add DatabaseSessionService with Hibernate and Flyway support#654prasadskarmarkar wants to merge 1 commit intogoogle:maingoogle/adk-java:mainfrom prasadskarmarkar:feature/database-session-serviceprasadskarmarkar/adk-java:feature/database-session-serviceCopy head branch name to clipboard
Conversation
|
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 @prasadskarmarkar, 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 introduces a new, production-ready 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-structured feature: a database-backed session service using Hibernate and Flyway. The implementation demonstrates strong practices, including multi-database support, Flyway for schema migrations, and pessimistic locking for concurrency control. The code is generally of high quality. However, I've identified a few critical and high-severity issues that should be addressed. The most critical concern is the use of Java serialization for persistence, which is fragile and insecure. There are also bugs causing data loss during the serialization/deserialization of certain event fields. Additionally, I've noted some misleading tests and opportunities for code cleanup and dependency improvements. My detailed comments are below.
core/src/main/java/com/google/adk/sessions/db/entity/StorageEvent.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/adk/sessions/DatabaseSessionService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/adk/sessions/db/entity/StorageEvent.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/adk/sessions/db/entity/StorageEvent.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/adk/sessions/DatabaseSessionService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/adk/sessions/db/converter/DialectAwareJsonConverter.java
Outdated
Show resolved
Hide resolved
fc948ac to
16e0ad4
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust, production-ready DatabaseSessionService using Hibernate and Flyway, which is a significant and well-executed feature. The implementation correctly uses pessimistic locking for concurrency control and Flyway for schema migrations, supporting multiple database backends. The test coverage is comprehensive and impressive.
My review focuses on improving maintainability, robustness, and consistency. Key suggestions include:
- Refactoring duplicated transaction management logic in
DatabaseSessionService. - Removing potential write side-effects from the
getSessionmethod to adhere to command-query separation. - Enhancing error logging by replacing
System.errwith a proper logger and logging exceptions that are currently swallowed. - Improving schema consistency in the MySQL migration script.
- Cleaning up potentially unused code and over-broad
.gitignorerules.
core/src/main/java/com/google/adk/sessions/DatabaseSessionService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/adk/sessions/DatabaseSessionService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/adk/sessions/db/converter/DialectAwareJsonConverter.java
Outdated
Show resolved
Hide resolved
89c8fe5 to
fd2fdc7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust, production-ready DatabaseSessionService using Hibernate for persistence and Flyway for database migrations. The implementation is impressive, with strong support for multiple database dialects, thread-safe operations using pessimistic locking, and comprehensive test coverage. My review focuses on a few areas for improvement, including strengthening the database schema for timestamp handling, simplifying state management logic, and removing potentially obsolete code. Overall, this is a high-quality contribution that significantly enhances the ADK's session management capabilities.
core/src/main/java/com/google/adk/sessions/DatabaseSessionService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/adk/sessions/DatabaseSessionService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/adk/sessions/db/util/SchemaInitializer.java
Outdated
Show resolved
Hide resolved
c1aa2b4 to
7a16f1a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust, production-ready DatabaseSessionService using Hibernate for persistence and Flyway for database migrations. The implementation is comprehensive, covering multi-database support, thread-safe operations with pessimistic locking, and extensive test coverage. The architecture is well-thought-out, particularly the handling of migrations in multi-pod environments.
My review focuses on improving concurrency handling and ensuring data type consistency. I've identified a race condition during the creation of new application/user states and an issue with the deep copying of state maps, both of which could affect correctness under load. I've also suggested an improvement to the MySQL schema for better performance and consistency. Overall, this is an excellent and thorough implementation.
| private StorageAppState getOrCreateAppState(EntityManager em, String appName) { | ||
| StorageAppState appState = em.find(StorageAppState.class, appName); | ||
| if (appState == null) { | ||
| appState = new StorageAppState(); | ||
| appState.setAppName(appName); | ||
| appState.setState(new HashMap<>()); | ||
| appState.setUpdateTime(Instant.now()); | ||
| em.persist(appState); | ||
| } | ||
| return appState; | ||
| } |
There was a problem hiding this comment.
There is a race condition in the getOrCreateAppState method (and similarly in getOrCreateUserState). The current "check-then-act" logic (em.find() followed by em.persist()) is not atomic across different transactions. If two threads concurrently call createSession for the same new application, both could find that the StorageAppState does not exist and then both attempt to persist a new one. This will cause the second transaction to fail with a PersistenceException (due to a primary key constraint violation) when it tries to commit. This can lead to failed session creations under concurrent load.
To fix this, you should handle the potential concurrency more robustly. One common pattern is to attempt the persist operation and catch the PersistenceException. If the exception is a constraint violation, it means another transaction created the entity in the meantime, so you can then safely find the existing entity.
A simplified version of this pattern would look like this:
// Inside a try-catch block for em.persist() and em.flush()
} catch (PersistenceException e) {
// A more robust check would inspect the exception cause for a constraint violation.
em.clear(); // Detach the failed entity from the persistence context.
StorageAppState existingState = em.find(StorageAppState.class, appName);
if (existingState != null) {
return existingState;
}
// If it's still null, something else went wrong.
throw e;
}This change will make the session creation process more resilient in a multi-threaded or multi-pod environment.
core/src/main/java/com/google/adk/sessions/db/converter/JsonUserType.java
Show resolved
Hide resolved
7a16f1a to
1fd831a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-designed DatabaseSessionService, adding persistent storage for sessions using JPA/Hibernate and Flyway for migrations. The implementation is robust, with good handling of concurrency through pessimistic locking, transaction management, and support for multiple database dialects. The test coverage is excellent, covering unit, integration, and concurrency scenarios.
My review focuses on a few key areas to further improve correctness and maintainability:
- Data Serialization: There are opportunities to make the serialization of
Contentobjects more robust by fully leveraging Jackson's capabilities, preventing potential data loss. - Schema Definition: A minor adjustment in the H2 database schema can prevent potential data truncation issues.
- Code Consistency: A small refactoring in one of the methods can improve consistency with the rest of the class.
Overall, this is a high-quality contribution that significantly enhances the ADK's capabilities.
7c46d73 to
f0e2716
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and production-ready DatabaseSessionService using Hibernate for persistence and Flyway for database migrations. The implementation is robust, featuring support for multiple database backends (PostgreSQL, MySQL, H2), pessimistic locking for concurrent operations, and a well-structured approach to schema management. The addition of extensive unit and integration tests covering concurrency, state management, and migrations is commendable. My review focuses on a few minor areas for improvement in error handling and code clarity within the new service.
core/src/main/java/com/google/adk/sessions/DatabaseSessionService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/adk/sessions/DatabaseSessionService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/adk/sessions/DatabaseSessionService.java
Outdated
Show resolved
Hide resolved
8030712 to
912d271
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and production-ready DatabaseSessionService. The implementation is robust, featuring support for multiple databases (PostgreSQL, MySQL, H2) via Hibernate, schema management with Flyway, and excellent concurrency control with pessimistic locking. The separation of state into app, user, and session scopes is well-designed. The test coverage is extensive and covers critical aspects like concurrency, migrations, and state management. My review includes a few suggestions for improving dependency management for MySQL, minor code style enhancements, and ensuring consistency in the database schema scripts. Overall, this is an excellent contribution.
There was a problem hiding this comment.
There's an inconsistency in the data types used for JSON storage in the MySQL schema. The events.actions column is defined as JSON, while other columns intended for JSON data (like sessions.state, events.content, etc.) are defined as LONGTEXT. The JsonUserType and EventActionsUserType implementations write a string to the database for non-PostgreSQL dialects. To maintain consistency with the UserType implementations and other columns, events.actions should also be LONGTEXT.
| actions JSON, | |
| actions LONGTEXT, |
| <!-- Database Migration --> | ||
|
|
||
| <!-- Flyway Core - Database schema migration and versioning tool --> | ||
| <dependency> | ||
| <groupId>org.flywaydb</groupId> | ||
| <artifactId>flyway-core</artifactId> | ||
| </dependency> | ||
|
|
||
| <!-- Flyway PostgreSQL Support - PostgreSQL-specific migration support --> | ||
| <dependency> | ||
| <groupId>org.flywaydb</groupId> | ||
| <artifactId>flyway-database-postgresql</artifactId> | ||
| <scope>runtime</scope> | ||
| </dependency> |
There was a problem hiding this comment.
For full support of MySQL, especially in multi-pod environments where migration locking is crucial, it's recommended to include the flyway-mysql dependency. This is analogous to how flyway-database-postgresql is included for PostgreSQL-specific features. Please consider adding it here and to the parent pom.xml's dependencyManagement section.
| return Completable.fromCallable( | ||
| () -> { | ||
| executeInTransaction( | ||
| em -> { | ||
| // Find session | ||
| SessionId id = new SessionId(appName, userId, sessionId); | ||
| StorageSession session = em.find(StorageSession.class, id); | ||
| if (session == null) { | ||
| throw new SessionNotFoundException( | ||
| String.format( | ||
| "Session not found: appName=%s, userId=%s, sessionId=%s", | ||
| appName, userId, sessionId)); | ||
| } | ||
|
|
||
| // Delete session (cascade will delete events) | ||
| em.remove(session); | ||
|
|
||
| return null; | ||
| }, | ||
| "Error deleting session"); | ||
| return null; | ||
| }) |
There was a problem hiding this comment.
The use of Completable.fromCallable with a Callable that just returns null is a bit indirect for an operation that has no return value. The Completable.fromAction operator is a better semantic fit here.
Consider refactoring to use fromAction to make the code slightly cleaner and more idiomatic, like so:
return Completable.fromAction(() -> executeInTransaction(em -> { ... }, "Error deleting session"))
.subscribeOn(Schedulers.io());| <property name="hibernate.hikari.idleTimeout" value="30000"/> | ||
| </properties> | ||
| </persistence-unit> | ||
| </persistence> No newline at end of file |
| CREATE INDEX IF NOT EXISTS idx_events_session ON events(app_name, user_id, session_id); | ||
|
|
||
| -- Index for sorting events by timestamp | ||
| CREATE INDEX IF NOT EXISTS idx_events_timestamp ON events(timestamp); No newline at end of file |
There was a problem hiding this comment.
| CREATE INDEX IF NOT EXISTS idx_events_session ON events(app_name, user_id, session_id); | ||
|
|
||
| -- Index for sorting events by timestamp | ||
| CREATE INDEX IF NOT EXISTS idx_events_timestamp ON events(timestamp); No newline at end of file |
There was a problem hiding this comment.
ae36c9e to
ac716fd
Compare
3fe9316 to
0126f61
Compare
7265a33 to
78506b1
Compare
Summary Implements a production-ready database-backed session service that provides persistent storage for ADK sessions, events, and state using JPA/Hibernate. Key Features Hibernate 6.6 + HikariCP connection pooling for optimal performance Flyway migrations for schema versioning and zero-downtime deployments Multi-database support: PostgreSQL, MySQL, H2, and other RDBMS Thread-safe operations with pessimistic locking for concurrent updates Comprehensive test coverage with H2 in-memory database Dialect-aware JSON storage (JSONB for PostgreSQL, CLOB for others) Event filtering and pagination for efficient data retrieval Architecture Database dependencies are non-optional in the core module for ease of use, consistent with Python ADK's approach where `DatabaseSessionService` is in the main package. Note: Database drivers (PostgreSQL, MySQL) are marked as `true`, so users must add their specific driver dependency to their project.
78506b1 to
c8e0960
Compare
Summary
Implements a production-ready database-backed session service that provides
persistent storage for ADK sessions, events, and state using JPA/Hibernate.
Key Features
Architecture
Database dependencies are non-optional in the core module for ease of use,
consistent with Python ADK's approach where `DatabaseSessionService` is in the
main package.
Note: Database drivers (PostgreSQL, MySQL) are marked as
`true`, so users must add their specific driver
dependency to their project.
Files Changed
Test Plan