TASK-P1-004: Design Exception Hierarchy #3048
Conversation
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 significantly overhauls ArcadeDB's exception handling mechanism to bring consistency, clarity, and better diagnostic capabilities. The core objective is to move away from disparate exception types towards a unified, code-driven system that not only simplifies error identification for developers but also provides richer, structured error responses for API consumers. This foundational change aims to improve the overall robustness, maintainability, and observability of the system. 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 1d8d116. 🟢 All jobs passed!But CI Insights is watching 👀 |
![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 does an excellent job of introducing a standardized and robust exception handling framework. The new ArcadeDBException hierarchy, ErrorCode enum, and ExceptionBuilder provide a solid foundation for consistent error handling, improved diagnostics, and better API responses. The changes are well-designed and the phased migration plan is thoughtful. My review includes suggestions to further improve consistency, performance, and API design within the new framework. These include optimizing an enum lookup, ensuring consistent map implementations, and refining the API of the new exception classes to prevent misuse. Overall, this is a high-quality contribution that significantly improves the project's maintainability.
engine/src/main/java/com/arcadedb/exception/ExceptionBuilder.java
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/arcadedb/exception/ExceptionBuilder.java
Outdated
Show resolved
Hide resolved
Code Review Comments - All Addressed ✅I've reviewed and addressed all 6 code review comments from the Copilot reviewer: 1. ADR Exception Code Table - FIXED✅ Expanded the error code ranges table in ADR-001-exception-handling.md to include all 11 categories:
The table now comprehensively documents all error code ranges with examples. 2. ArcadeDBException.toString() - FIXED✅ Refactored context string building to use Stream API with if (!context.isEmpty()) {
sb.append(" {")
.append(context.entrySet().stream()
.map(entry -> entry.getKey() + "=" + entry.getValue())
.collect(Collectors.joining(", ")))
.append("}");
}3. ErrorCode.fromCode() Performance - FIXED✅ Optimized from O(N) to O(1) using a static unmodifiable map: private static final Map<Integer, ErrorCode> CODE_MAP =
java.util.stream.Stream.of(values())
.collect(Collectors.toUnmodifiableMap(ErrorCode::getCode, e -> e));
public static ErrorCode fromCode(final int code) {
return CODE_MAP.getOrDefault(code, INTERNAL_ERROR);
}4. ExceptionBuilder Context Map - FIXED✅ Changed from private final Map<String, Object> context = new LinkedHashMap<>();5. ExceptionBuilder Exception Handling - FIXED✅ Changed from throwing generic throw new InternalException("Failed to build exception: " + exceptionClass.getName(), e);6. InternalException API - FIXED✅ Made ErrorCode-accepting constructors protected InternalException(final ErrorCode errorCode, final String message) {
super(errorCode, message);
}This enforces proper use of specific exception types while keeping the string-based public constructors available. Build Status✅ Engine module compiles successfully with all improvements |
robfrank
left a comment
robfrank
left a comment
There was a problem hiding this comment.
All code review suggestions have been implemented and verified. The changes improve code quality through better performance, readability, and API design consistency.
| | Index | 8000-8999 | Index operations | 8001: INDEX_NOT_FOUND, 8002: DUPLICATE_KEY | | ||
|
|
||
| ### Enhanced ArcadeDBException Design | ||
|
|
There was a problem hiding this comment.
✅ FIXED - Expanded the error code ranges table to include all 11 categories:
- Added Graph (9000-9999)
- Added Import/Export (10000-10999)
- Added Internal (99000-99999)
The table now comprehensively shows all error code ranges and their respective examples for clarity and consistency.
engine/src/main/java/com/arcadedb/exception/ArcadeDBException.java
Outdated
Show resolved
Hide resolved
| private ErrorCode errorCode; | ||
| private String message; | ||
| private Throwable cause; | ||
| private final Map<String, Object> context = new HashMap<>(); |
There was a problem hiding this comment.
✅ FIXED - Changed context map to use LinkedHashMap for consistent insertion-order preservation, matching the behavior in ArcadeDBException.
engine/src/main/java/com/arcadedb/exception/ExceptionBuilder.java
Outdated
Show resolved
Hide resolved
robfrank
force-pushed
the
feat/2866-exceptions
branch
from
fc02cd0 to
786b40d
Compare
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 |
robfrank
changed the title
robfrank
force-pushed
the
feat/2866-exceptions
branch
3 times, most recently
from
3aee0af to
cee01b9
Compare
lvca
left a comment
lvca
left a comment
There was a problem hiding this comment.
I don't like to mix the http code concept all over the modules, I think it's a violation of responsibility. We could just have a HttpCodeResolver that takes an exception and returns the http code if this is the requirement.
| * @return the HTTP status code | ||
| */ | ||
| @Override | ||
| public int getHttpStatus() { |
There was a problem hiding this comment.
Not nice the basic exception has concepts of http here
|
|
||
| // ========== Network Errors (6xxx) ========== | ||
| /** Failed to establish or maintain connection */ | ||
| CONNECTION_ERROR(6001, "Connection error"), |
There was a problem hiding this comment.
Same here, the engine base model now has concepts from other modules (network)
| * @return the HTTP status code | ||
| */ | ||
| @Override | ||
| public int getHttpStatus() { |
There was a problem hiding this comment.
Instead of contaminating all the exceptions with HTTP concept and codes, I'd prefer to have 1 method that returns the Http code from any exception, and it should be in the http server.
…ed context formatting
…ontext and error codes
robfrank
force-pushed
the
feat/2866-exceptions
branch
from
cee01b9 to
1d8d116
Compare
This pull request introduces a new standardized exception handling framework for ArcadeDB, focused on providing structured error codes, improved diagnostics, and better API response formatting. The changes include a major refactoring of the base exception class, the introduction of an
ErrorCodeenum for categorizing and identifying errors, and the addition of a specializedDatabaseExceptionclass. These updates enable more consistent error handling, easier troubleshooting, and better integration with HTTP APIs.Exception Framework Refactor and Enhancements:
ArcadeDBExceptionclass to be abstract and to support standardized error codes, diagnostic context, JSON serialization for API responses, HTTP status code mapping, and timestamp tracking. Also added method chaining for context and improved string representation. Legacy constructors are retained for backward compatibility but marked as deprecated.ErrorCodeenum, providing a comprehensive and categorized list of error codes for all major error domains (Database, Transaction, Query, Security, Storage, Network, Schema, Index, Graph, Import/Export, Internal). Each error code includes a numeric value, default message, and category, with utility methods for lookup and formatting.Specialized Exception Classes:
DatabaseExceptionclass that extendsArcadeDBException, tailored for database lifecycle and operation errors. This class sets a default error code, provides constructors for error code/message/cause, and overrides HTTP status mapping for common database errors (e.g., 404 for not found, 409 for conflict).