feat(mysql): add MysqlSkillRepository and associated tests for skill … #600
Conversation
Summary of ChangesHello @jianjun159, 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 significant enhancement to the AgentScope-Java framework by integrating MySQL as a persistent storage solution for agent skills. It provides a robust and secure mechanism for managing skill metadata and associated resources, allowing for scalable and reliable skill management within AgentScope applications. This addition expands the data persistence options available for AgentScope components. 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
|
![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 pull request introduces a MysqlSkillRepository for persisting agent skills in a MySQL database. The implementation is robust, with good practices like using a builder pattern, transaction management, and validation to prevent SQL injection. The accompanying unit tests are comprehensive and well-structured.
I've identified a few areas for improvement, primarily concerning data integrity, performance, and transactional behavior. My key recommendations are:
- Enforcing data integrity by adding the documented foreign key constraint to the database schema.
- Optimizing the
getAllSkillsmethod to resolve an N+1 query issue. - Revisiting the transaction logic in the
savemethod to handle lists of skills more predictably. - A minor performance enhancement in resource insertion and a correction in the corresponding unit test.
Overall, this is a solid contribution. Addressing these points will make the repository more performant and resilient.
| String createResourcesTableSql = | ||
| "CREATE TABLE IF NOT EXISTS " | ||
| + getFullTableName(resourcesTableName) | ||
| + " (skill_name VARCHAR(255) NOT NULL, resource_path VARCHAR(500) NOT NULL," | ||
| + " resource_content LONGTEXT NOT NULL, created_at TIMESTAMP DEFAULT" | ||
| + " CURRENT_TIMESTAMP, updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON" | ||
| + " UPDATE CURRENT_TIMESTAMP, PRIMARY KEY (skill_name, resource_path))" | ||
| + " DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci"; |
There was a problem hiding this comment.
The Javadoc for the agentscope_skill_resources table (lines 60-68) mentions a foreign key constraint: FOREIGN KEY (skill_name) REFERENCES agentscope_skills(name) ON DELETE CASCADE. However, this CREATE TABLE statement is missing that constraint.
While the code manually deletes resources in deleteSkillInternal, adding the foreign key at the database level is a best practice for ensuring data integrity and preventing orphaned resource records. It also allows the database to enforce the relationship automatically.
| String createResourcesTableSql = | |
| "CREATE TABLE IF NOT EXISTS " | |
| + getFullTableName(resourcesTableName) | |
| + " (skill_name VARCHAR(255) NOT NULL, resource_path VARCHAR(500) NOT NULL," | |
| + " resource_content LONGTEXT NOT NULL, created_at TIMESTAMP DEFAULT" | |
| + " CURRENT_TIMESTAMP, updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON" | |
| + " UPDATE CURRENT_TIMESTAMP, PRIMARY KEY (skill_name, resource_path))" | |
| + " DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci"; | |
| String createResourcesTableSql = | |
| "CREATE TABLE IF NOT EXISTS " | |
| + getFullTableName(resourcesTableName) | |
| + " (skill_name VARCHAR(255) NOT NULL, resource_path VARCHAR(500) NOT NULL," | |
| + " resource_content LONGTEXT NOT NULL, created_at TIMESTAMP DEFAULT" | |
| + " CURRENT_TIMESTAMP, updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON" | |
| + " UPDATE CURRENT_TIMESTAMP, PRIMARY KEY (skill_name, resource_path)," | |
| + " FOREIGN KEY (skill_name) REFERENCES " + getFullTableName(skillsTableName) + "(name) ON DELETE CASCADE)" | |
| + " DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci"; |
| public List<AgentSkill> getAllSkills() { | ||
| List<String> skillNames = getAllSkillNames(); | ||
| List<AgentSkill> skills = new ArrayList<>(); | ||
|
|
||
| for (String name : skillNames) { | ||
| try { | ||
| skills.add(getSkill(name)); | ||
| } catch (Exception e) { | ||
| logger.warn("Failed to load skill '{}': {}", name, e.getMessage(), e); | ||
| // Continue processing other skills | ||
| } | ||
| } | ||
|
|
||
| return skills; | ||
| } |
There was a problem hiding this comment.
The getAllSkills() method currently suffers from the N+1 query problem. It first fetches all skill names and then iterates through them, executing getSkill() for each name. Each getSkill() call can perform up to two additional queries. This results in 1 + 2*N queries, where N is the number of skills, which is highly inefficient for a large number of skills.
To optimize this, you can fetch all skills in one query and all resources in another, then map them together in memory. This reduces the database round-trips to a constant number (2), regardless of the number of skills.
| if (exists && !force) { | ||
| logger.info("Skill already exists and force=false: {}", skillName); | ||
| conn.rollback(); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The save method's transaction handling for a list of skills could lead to unexpected behavior. If one skill in the list already exists (and force=false), the entire transaction is rolled back. This means any preceding, valid skills in the list that were already processed are also rolled back.
This 'all-or-nothing' approach might be unintentional. Consider if it would be better to either:
- Check for the existence of all skills before starting the transaction.
- Use a separate transaction for each skill in the list.
- Throw a specific exception indicating which skill caused the failure, rather than returning
false.
| int insertedCount = 0; | ||
| for (Map.Entry<String, String> entry : resources.entrySet()) { | ||
| String path = entry.getKey(); | ||
| String content = entry.getValue(); | ||
|
|
||
| validateResourcePath(path); | ||
|
|
||
| try (PreparedStatement stmt = conn.prepareStatement(insertSql)) { | ||
| stmt.setString(1, skillName); | ||
| stmt.setString(2, path); | ||
| stmt.setString(3, content); | ||
| int affected = stmt.executeUpdate(); | ||
| if (affected > 0) { | ||
| insertedCount++; | ||
| logger.debug("Inserted resource '{}' for skill '{}'", path, skillName); | ||
| } else { | ||
| logger.warn("Failed to insert resource '{}' for skill '{}'", path, skillName); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In the insertResources method, a new PreparedStatement is created inside the for loop for every resource. This is inefficient as it requires the database to parse the same SQL query repeatedly. For better performance, you should prepare the statement once outside the loop and reuse it.
| int insertedCount = 0; | |
| for (Map.Entry<String, String> entry : resources.entrySet()) { | |
| String path = entry.getKey(); | |
| String content = entry.getValue(); | |
| validateResourcePath(path); | |
| try (PreparedStatement stmt = conn.prepareStatement(insertSql)) { | |
| stmt.setString(1, skillName); | |
| stmt.setString(2, path); | |
| stmt.setString(3, content); | |
| int affected = stmt.executeUpdate(); | |
| if (affected > 0) { | |
| insertedCount++; | |
| logger.debug("Inserted resource '{}' for skill '{}'", path, skillName); | |
| } else { | |
| logger.warn("Failed to insert resource '{}' for skill '{}'", path, skillName); | |
| } | |
| } | |
| } | |
| int insertedCount = 0; | |
| try (PreparedStatement stmt = conn.prepareStatement(insertSql)) { | |
| for (Map.Entry<String, String> entry : resources.entrySet()) { | |
| String path = entry.getKey(); | |
| String content = entry.getValue(); | |
| validateResourcePath(path); | |
| stmt.setString(1, skillName); | |
| stmt.setString(2, path); | |
| stmt.setString(3, content); | |
| int affected = stmt.executeUpdate(); | |
| if (affected > 0) { | |
| insertedCount++; | |
| logger.debug("Inserted resource '{}' for skill '{}'", path, skillName); | |
| } else { | |
| logger.warn("Failed to insert resource '{}' for skill '{}'", path, skillName); | |
| } | |
| } | |
| } |
| when(mockStatement.executeUpdate()).thenReturn(1); | ||
| when(mockStatement.executeQuery()).thenReturn(mockResultSet); | ||
| when(mockResultSet.next()).thenReturn(false); // skill doesn't exist | ||
| when(mockStatement.executeBatch()).thenReturn(new int[] {1, 1}); |
There was a problem hiding this comment.
This test mocks executeBatch(), but the insertResources method explicitly avoids batching and calls executeUpdate() in a loop. This makes the test assertion misleading as it's not testing the actual implementation path.
The test likely passes due to a broader mock on executeUpdate(). To make the test clearer and more accurate, this unused mock should be removed.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
👌 |
docs/zh/task/agent-skill.md
Outdated
| AgentSkill loaded = repo.getSkill("data_analysis"); | ||
| ``` | ||
|
|
||
| #### MySQL 数据库存储 |
There was a problem hiding this comment.
Do not provide a detailed introduction to the usage of MySQLRepo here. The documentation only explains the behavior of the interface. Just add the flag indicating that the MySQL storage in the above database has not been implemented.
docs/zh/task/agent-skill.md
Outdated
|
|
||
| - 文件系统存储 | ||
| - 数据库存储 (暂未实现) | ||
| - MySQL 数据库存储 |
There was a problem hiding this comment.
- 数据库存储
- MySQL数据库存储 (agentscope-extensions-skill-mysql-repository)
| <relativePath>../pom.xml</relativePath> | ||
| </parent> | ||
|
|
||
| <artifactId>agentscope-extensions-skill-mysql</artifactId> |
There was a problem hiding this comment.
change to "agentscope-extensions-skill-mysql-repository"
There was a problem hiding this comment.
plz fix this code bot suggestions. If you think this suggestion is unnecessary, close the suggestion.
There was a problem hiding this comment.
@fang-tech Corresponding modifications have been made in accordance with your suggestions
# Conflicts: # docs/en/task/agent-skill.md # docs/zh/task/agent-skill.md
fang-tech
left a comment
fang-tech
left a comment
There was a problem hiding this comment.
LGTM, tks for your contribution!
docs/zh/task/agent-skill.md
Outdated
|
|
||
| - 文件系统存储 | ||
| - 数据库存储 | ||
| - MySQL数据库存储 (agentscope-extensions-skill-mysql-repository) |
docs/en/task/agent-skill.md
Outdated
|
|
||
| #### File System Storage | ||
| - File system storage | ||
| - database storage |
|
|
||
| Bind Tools to Skills for on-demand activation. Avoids context pollution from pre-registering all Tools, only passing relevant Tools to LLM when the Skill is actively used. | ||
|
|
||
| **Lifecycle of Progressively Disclosed Tools**: Tool lifecycle remains consistent with Skill lifecycle. Once a Skill is activated, Tools remain available throughout the entire session, avoiding the call failures caused by Tool deactivation after each conversation round in the old mechanism. |
There was a problem hiding this comment.
do not change this statement
There was a problem hiding this comment.
update here, do not change this
| conn.rollback(); | ||
| throw e; | ||
| } finally { | ||
| conn.setAutoCommit(true); |
There was a problem hiding this comment.
If conn.setAutoCommit(true) throws SQLException, it will be caught by the outer catch block and wrapped as RuntimeException, potentially leaving the connection in an inconsistent state.
There was a problem hiding this comment.
finally {
try {
conn.setAutoCommit(true);
} catch (SQLException e) {
logger.error("Failed to restore autoCommit", e);
// Don't rethrow - let try-with-resources close the connection
}
}There was a problem hiding this comment.
Same issue exists in: delete() method (line 745) and clearAllSkills() method (line 908)
| * | ||
| * <pre> | ||
| * CREATE TABLE IF NOT EXISTS agentscope_skills ( | ||
| * name VARCHAR(255) NOT NULL PRIMARY KEY, |
There was a problem hiding this comment.
add field skill_id as primary key, do not use name
| * ) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci; | ||
| * | ||
| * CREATE TABLE IF NOT EXISTS agentscope_skill_resources ( | ||
| * skill_name VARCHAR(255) NOT NULL, |
| stmt.setString(2, path); | ||
| stmt.setString(3, content); | ||
|
|
||
| int affected = stmt.executeUpdate(); |
There was a problem hiding this comment.
calling executeUpdate() in each loop iteration is NOT batch processing. This creates N network round-trips for N resources.
Recommended Fix:
for (Map.Entry<String, String> entry : resources.entrySet()) {
String path = entry.getKey();
String content = entry.getValue();
stmt.setString(1, skillName);
stmt.setString(2, path);
stmt.setString(3, content);
stmt.addBatch(); // Add to batch instead of executing immediately
}
// Execute all inserts in one batch
int[] results = stmt.executeBatch();
// Count successful insertions
int insertedCount = 0;
for (int i = 0; i < results.length; i++) {
if (results[i] > 0 || results[i] == Statement.SUCCESS_NO_INFO) {
insertedCount++;
} else if (results[i] == Statement.EXECUTE_FAILED) {
logger.error("Failed to insert resource at index {}", i);
}
}
logger.debug("Batch inserted {} resources for skill '{}'", insertedCount, skillName);| } | ||
|
|
||
| // Validate all resource paths first | ||
| for (String path : resources.keySet()) { |
There was a problem hiding this comment.
This validation happens inside insertResources(), which is called after the transaction has started. If validation fails, it causes an unnecessary transaction rollback.
Recommended Fix: Move validation to the pre-check section in save() method:
// Pre-check: validate all skill names and resource paths first
for (AgentSkill skill : skills) {
validateSkillName(skill.getName());
// Validate resource paths before transaction
Map<String, String> resources = skill.getResources();
if (resources != null && !resources.isEmpty()) {
for (String path : resources.keySet()) {
validateResourcePath(path);
}
}
}| * @throws SQLException if deletion fails | ||
| */ | ||
| private void deleteSkillInternal(Connection conn, String skillName) throws SQLException { | ||
| // Delete resources first (if foreign key doesn't have CASCADE) |
There was a problem hiding this comment.
the comment says "if foreign key doesn't have CASCADE", but the table creation SQL at line 312 does define ON DELETE CASCADE:
Option A - Remove redundant delete
Option B - Keep explicit delete with updated
| * @throws IllegalStateException if createIfNotExist is false and | ||
| * database/tables do not exist | ||
| */ | ||
| public MysqlSkillRepository( |
There was a problem hiding this comment.
Keep Simple Constructor + Builder
Just keep Simple Constructor:
public MysqlSkillRepository(datasource, createIfNotExist, writeable);There was a problem hiding this comment.
OK, I have completed the corresponding modifications. Could you please review it again
There was a problem hiding this comment.
make Full-parameter construction method private, because you already have a Builder.
| * @throws IllegalStateException if createIfNotExist is false and | ||
| * database/tables do not exist | ||
| */ | ||
| public MysqlSkillRepository( |
There was a problem hiding this comment.
make Full-parameter construction method private, because you already have a Builder.
docs/en/task/agent-skill.md
Outdated
| Skills need to remain available after application restart, or be shared across different environments. Persistence storage supports: | ||
|
|
||
| #### File System Storage | ||
| #### File System Storag |
| @@ -170,8 +170,6 @@ ReActAgent agent = ReActAgent.builder() | |||
|
|
|||
| Bind Tools to Skills for on-demand activation. Avoids context pollution from pre-registering all Tools, only passing relevant Tools to LLM when the Skill is actively used. | |||
|
|
|||
There was a problem hiding this comment.
do not delete this chunk
| + " underscore. Invalid value: " | ||
| + identifier); | ||
| } | ||
| } |
There was a problem hiding this comment.
What I mean is to keep only the simplest constructor and the Builder, and retain the Builder.
There was a problem hiding this comment.
I'm very sorry. I misunderstood your meaning just now. I have made corresponding corrections. Thank you very much for your correction
|
Update the unnecessary modifications in the English document |
…disclosure of tools
Ok. Now the content of the Chinese document and the English document is consistent |
|
@AlbumenJ help to merge |
3 participants
…management
AgentScope-Java Version
[The version of AgentScope-Java you are working on, e.g. 1.0.7, check your pom.xml dependency version or run
mvn dependency:tree | grep agentscope-parent:pom(only mac/linux)]version: 1.0.7
Description
[Please describe the background, purpose, changes made, and how to test this PR]
feat(mysql): add MysqlSkillRepository and associated tests for skill management #599
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)