Conversation
…tegration dependency
Summary of ChangesHello @robfrank, 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 focuses on enhancing the architectural modularity and reducing inter-module dependencies by relocating core server path utility logic to a more appropriate module. This refactoring ensures a cleaner separation of concerns between the server and integration components. Concurrently, new comprehensive test cases have been introduced to validate the robustness and correct operation of vector index functionality, particularly concerning database persistence and querying capabilities. 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
|
🧪 CI InsightsHere's what we observed from your CI run for 35f9aff. 🟢 All jobs passed!But CI Insights is watching 👀 |
There was a problem hiding this comment.
Code Review
This pull request successfully refactors server path utility logic to improve modularity and reduce dependencies, which is a great improvement. The dependency of the server module on arcadedb-integration has been correctly moved to a test-only scope. However, the newly added test files, particularly SimpleGloVeTest.java, introduce some significant issues. This includes a hardcoded, user-specific file path which makes the build non-portable, and the use of System.exit(0) in a test class. I've provided detailed feedback and suggestions to address these and other minor issues in the code.
| */ | ||
| public class SimpleGloVeTest { | ||
| private final static int PARALLEL_LEVEL = 8; | ||
| private static final String FILE_NAME = "/Users/frank/Downloads/glove.twitter.27B/glove.twitter.27B.100d.txt"; |
There was a problem hiding this comment.
This file contains a hardcoded, user-specific file path. This will cause the test/application to fail on any machine other than the original author's. This path should be parameterized, for example, by reading it from a configuration file or a system property. Hardcoding absolute paths like this makes the build non-portable and should be avoided.
| public static void main(String[] args) { | ||
| new SimpleGloVeTest(); | ||
| } | ||
|
|
||
| public SimpleGloVeTest() { | ||
|
|
||
| final DatabaseFactory factory = new DatabaseFactory("databases/glovedb"); | ||
| final Database database; | ||
|
|
||
| try { | ||
|
|
||
| database = importDataSet(factory); | ||
|
|
||
| LogManager.instance().log(this, Level.SEVERE, "Found index with %d entries", | ||
| database.getSchema().getIndexByName("Word[vector]").countEntries()); | ||
|
|
||
| querySQL(database); | ||
| } finally { | ||
| factory.close(); | ||
| } | ||
| System.exit(0); | ||
| } |
There was a problem hiding this comment.
This class is located in the test sources (src/test/java) but is structured like a standalone application with a main method and a System.exit(0) call. This is not appropriate for automated tests.
- If this is intended to be an example application, it should be moved to
src/main/javain an examples package. - If it is an automated test, it should be refactored into a proper JUnit test case with
@Testmethods, andSystem.exit()must be removed.
The call to System.exit(0) is particularly problematic as it can terminate the entire JVM, which will prematurely stop the test suite.
| * | ||
| * @author Luca Garulli (l.garulli@arcadedata.com) | ||
| */ | ||
| public class ServerPathUtils { |
There was a problem hiding this comment.
This is a utility class with only static methods. To prevent instantiation, it's a good practice to add a private constructor.
public class ServerPathUtils {
private ServerPathUtils() {
// Utility class
}| * @author Luca Garulli (l.garulli@arcadedata.com) | ||
| */ | ||
| @Deprecated(forRemoval = true, since = "25.11.1") | ||
| public class IntegrationUtils { |
There was a problem hiding this comment.
This utility class only contains a static method. To prevent accidental instantiation, it's a good practice to add a private constructor, even for a deprecated class.
public class IntegrationUtils {
private IntegrationUtils() {
// Utility class
}| if (!factory.exists()) { | ||
| System.out.println("Skipping test - glovedb database does not exist"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Instead of manually checking for the database's existence and printing to standard output, it's better to use JUnit 5's Assumptions.assumeTrue(). This will properly mark the test as skipped in test reports, which is more informative. You'll need to add import org.junit.jupiter.api.Assumptions;.
Additionally, System.out.println is used throughout this test. It should be avoided in tests. Please use a logger for any output.
Assumptions.assumeTrue(factory.exists(), "Skipping test - glovedb database does not exist");| private final static int PARALLEL_LEVEL = 8; | ||
| private static final String FILE_NAME = "/Users/frank/Downloads/glove.twitter.27B/glove.twitter.27B.100d.txt"; | ||
| private boolean USE_SQL = true; |
There was a problem hiding this comment.
There are several unused variables in this class that should be removed to improve code clarity:
- The field
PARALLEL_LEVEL(line 48) is not used. - The field
USE_SQL(line 50) is not used. - The local variable
startWordinquerySQL(line 87) is initialized but never used.
| LogManager.instance().log(this, Level.SEVERE, "Found index with %d entries", | ||
| database.getSchema().getIndexByName("Word[vector]").countEntries()); |
There was a problem hiding this comment.
The logging level Level.SEVERE is used here and in other places in this file for informational messages. SEVERE should be reserved for serious failures. For informational messages, Level.INFO or Level.FINE would be more appropriate. Please review all logging statements in this file and use the correct log levels.
| LogManager.instance().log(this, Level.SEVERE, "Found index with %d entries", | |
| database.getSchema().getIndexByName("Word[vector]").countEntries()); | |
| LogManager.instance().log(this, Level.INFO, "Found index with %d entries", |
| // TODO: REMOVE THIS | ||
| // if (factory.exists()) | ||
| // factory.open().drop(); |
There was a problem hiding this comment.
This commented-out code block with a TODO: REMOVE THIS should be removed before merging. Leaving such artifacts in the codebase can cause confusion.
Review Comments ResolutionFixed Issues✅ ServerPathUtils - Private Constructor (Comment #2593292814)
✅ IntegrationUtils - Private Constructor (Comment #2593292821)
Requires Removal/Refactoring❌ SimpleGloVeTest.java - Critical Issues
RecommendationSimpleGloVeTest.java should be removed from this PR. This file:
The core fix (ServerPathUtils, dependency management, IntegrationUtils deprecation) is solid and should be merged separately from this problematic test file. |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
… /studio in the security-critical group [skip ci] Bumps the security-critical group in /studio with 1 update: [sweetalert2](https://github.com/sweetalert2/sweetalert2). Updates `sweetalert2` from 11.26.1 to 11.26.2 Release notes *Sourced from [sweetalert2's releases](https://github.com/sweetalert2/sweetalert2/releases).* > v11.26.2 > -------- > > [11.26.2](sweetalert2/sweetalert2@v11.26.1...v11.26.2) (2025-10-12) > ---------------------------------------------------------------------------------------------- > > ### Bug Fixes > > * DismissReason type, ESLint warnings ([ArcadeData#2882](https://redirect.github.com/sweetalert2/sweetalert2/issues/2882)) ([c5af0cf](sweetalert2/sweetalert2@c5af0cf)) Changelog *Sourced from [sweetalert2's changelog](https://github.com/sweetalert2/sweetalert2/blob/main/CHANGELOG.md).* > [11.26.2](sweetalert2/sweetalert2@v11.26.1...v11.26.2) (2025-10-12) > ---------------------------------------------------------------------------------------------- > > ### Bug Fixes > > * DismissReason type, ESLint warnings ([ArcadeData#2882](https://redirect.github.com/sweetalert2/sweetalert2/issues/2882)) ([c5af0cf](sweetalert2/sweetalert2@c5af0cf)) Commits * [`c19156f`](sweetalert2/sweetalert2@c19156f) chore(release): 11.26.2 [skip ci] * [`c5af0cf`](sweetalert2/sweetalert2@c5af0cf) fix: DismissReason type, ESLint warnings ([ArcadeData#2882](https://redirect.github.com/sweetalert2/sweetalert2/issues/2882)) * [`5f32bff`](sweetalert2/sweetalert2@5f32bff) chore: bump bun.lock * [`8be0497`](sweetalert2/sweetalert2@8be0497) chore: yarn -> bun migration ([ArcadeData#2881](https://redirect.github.com/sweetalert2/sweetalert2/issues/2881)) * See full diff in [compare view](sweetalert2/sweetalert2@v11.26.1...v11.26.2) [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore major version` will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself) - `@dependabot ignore minor version` will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself) - `@dependabot ignore ` will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself) - `@dependabot unignore ` will remove all of the ignore conditions of the specified dependency - `@dependabot unignore ` will remove the ignore condition of the specified dependency and ignore conditions
This pull request refactors the server path utility logic to improve modularity and reduce dependencies between the server and integration modules. It introduces a new utility class in the engine module, deprecates the old integration utility, and updates references throughout the codebase. Additionally, it adds new vector index test cases and adjusts Maven dependencies for better separation of concerns.
Refactoring and modularization
ServerPathUtilsutility class in theenginemodule to handle server root path configuration, moving the logic fromIntegrationUtilsto avoid server → integration dependency. (engine/src/main/java/com/arcadedb/utility/ServerPathUtils.java)IntegrationUtilsclass in the integration module, updating its documentation and implementation to delegate toServerPathUtilsfor backward compatibility. (integration/src/main/java/com/arcadedb/integration/misc/IntegrationUtils.java)Dependency management
server/pom.xmlto remove the compile-time dependency onarcadedb-integrationand add it as a test-scoped dependency, ensuring the server module does not depend on integration code at runtime. (server/pom.xml) [1] [2]Codebase updates
IntegrationUtils.setRootPathwithServerPathUtils.setRootPathin theArcadeDBServerclass, reflecting the new utility location. (server/src/main/java/com/arcadedb/server/ArcadeDBServer.java) [1] [2] [3]Testing improvements
GloVeReopenVerificationTestto verify vector searches after database restarts andSimpleGloVeTestas an example application for querying word vectors. (integration/src/test/java/com/arcadedb/integration/importer/vector/GloVeReopenVerificationTest.java,integration/src/test/java/com/arcadedb/integration/importer/vector/SimpleGloVeTest.java) [1] [2]