-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[OPIK-3012] [BE] Add workspace dashboards CRUD functionality #4058
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[OPIK-3012] [BE] Add workspace dashboards CRUD functionality #4058
Conversation
a7352e4 to
b762afc
Compare
Backend Tests Results 296 files 296 suites 52m 11s ⏱️ Results for commit c9178c2. ♻️ This comment has been updated with latest results. |
SDK E2E Tests Results0 tests 0 ✅ 0s ⏱️ Results for commit 57d9657. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements complete CRUD functionality for workspace-level custom dashboards in the Opik backend. The feature enables users to create, read, update, and delete custom dashboards with JSON-based configurations, including automatic slug generation, optimistic locking for concurrent updates, and workspace-scoped operations.
Key changes:
- New database schema with
dashboardstable supporting JSON configurations and workspace multi-tenancy - RESTful API endpoints for all CRUD operations with proper validation and error handling
- Business logic layer with slug generation utilities and optimistic concurrency control
- Comprehensive test suite with 23 test cases covering all operations and edge cases
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
000034_create_dashboards_table.sql |
Liquibase migration creating dashboards table with UUIDv7 IDs, JSON config storage, and unique constraints |
JsonNodeColumnMapper.java |
JDBI3 column mapper for serializing/deserializing JSON columns to Jackson JsonNode objects |
SlugUtils.java |
Utility class for generating URL-safe slugs from dashboard names with Unicode normalization and deduplication |
DashboardDAO.java |
JDBI3 data access interface with parameterized queries for database operations |
DashboardService.java |
Service layer implementing business logic including slug generation, validation, and transaction management |
DashboardsResource.java |
REST API controller with OpenAPI documentation for all CRUD endpoints |
Dashboard.java |
Domain model with JsonView annotations for field visibility control |
DashboardUpdate.java |
DTO for dashboard update operations with validation constraints |
DashboardResourceClient.java |
Test client utility for making API requests in integration tests |
DashboardsResourceTest.java |
Comprehensive test suite with 23 tests covering all CRUD operations and edge cases |
apps/opik-backend/src/main/java/com/comet/opik/domain/SlugUtils.java
Outdated
Show resolved
Hide resolved
...kend/src/main/resources/liquibase/db-app-state/migrations/000034_create_dashboards_table.sql
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/db/JsonNodeColumnMapper.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DashboardService.java
Outdated
Show resolved
Hide resolved
...kend/src/test/java/com/comet/opik/api/resources/utils/resources/DashboardResourceClient.java
Outdated
Show resolved
Hide resolved
- Add MAX_SLUG_LENGTH constant (150) to match database constraint - Remove DROP TABLE IF EXISTS from main migration changeset - Make ObjectMapper static final in JsonNodeColumnMapper for performance - Add retry logic with proper for loop for slug race conditions - Remove unused podamFactory field from DashboardResourceClient
apps/opik-backend/src/main/java/com/comet/opik/api/DashboardUpdate.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/DashboardsResource.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/DashboardsResource.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/DashboardsResource.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/DashboardsResource.java
Outdated
Show resolved
Hide resolved
| * Utility class for generating URL-safe slugs from dashboard names. | ||
| */ | ||
| @UtilityClass | ||
| public class SlugUtils { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd try to use a stable and license compatible library to avoid re-inventing the while. I believe this is an option:
But there would be more library options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you believe we really need a lib for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a matter of avoiding maintaining, testing etc. our own code, if we can get it for free from a well-known and stable library etc.
Also to avoid bugs, side cases etc. as this implementation might have not considered all characters in regex etc.
But this is not a blocker.
apps/opik-backend/src/main/java/com/comet/opik/domain/SlugUtils.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/SlugUtils.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/db/JsonNodeColumnMapper.java
Outdated
Show resolved
Hide resolved
...kend/src/test/java/com/comet/opik/api/resources/utils/resources/DashboardResourceClient.java
Outdated
Show resolved
Hide resolved
- Remove optimistic locking (lastUpdatedAt from DashboardUpdate) - Consolidate update + refresh into single service method that returns Dashboard - Remove retry logic from update method - Rename 'search' query parameter to 'name' with proper documentation - Change ORDER BY from last_updated_at to id DESC - Replace ObjectMapper with JsonUtils in JsonNodeColumnMapper and test client - Remove unnecessary @nonnull annotations from SlugUtils - Update API documentation to clarify partial update support
- Removed 'updateDashboardWithTimestampCheck' test that relied on lastUpdatedAt field - All 20 remaining tests pass successfully
- Changed path from /v1/private/workspaces/{workspaceId}/dashboards to /v1/private/dashboards
- Removed unused workspaceIdPath parameters from all endpoint methods
- Updated test client RESOURCE_PATH constant and all API calls
- WorkspaceId is now resolved exclusively from authentication context (RequestContext)
- All 20 tests pass successfully
andrescrz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, you can move forward.
I strongly recommend changing the update method to PATCH. It doesn't look like a PUT.
My secondary commendation is using a library for generating slugs, but not a blocker.
The rest if minor an optional.
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/DashboardsResource.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DashboardDAO.java
Outdated
Show resolved
Hide resolved
| * Utility class for generating URL-safe slugs from dashboard names. | ||
| */ | ||
| @UtilityClass | ||
| public class SlugUtils { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a matter of avoiding maintaining, testing etc. our own code, if we can get it for free from a well-known and stable library etc.
Also to avoid bugs, side cases etc. as this implementation might have not considered all characters in regex etc.
But this is not a blocker.
Details
This PR implements the complete CRUD functionality for workspace-level custom dashboards. The implementation includes:
Core Features
Key Components
Database Layer:
dashboardstable with UUIDv7 IDs, workspace scoping, and optimistic lockingJsonNodeColumnMapperfor JDBI3 JSON handlingDomain Layer:
DashboardDAO: JDBI3 interface for database operationsDashboardService: Business logic including slug generation and validationDashboardConfigValidator: JSON schema validation for dashboard configurationsSlugUtils: URL-safe slug generation with deduplicationAPI Layer:
/v1/private/dashboardsTesting:
OptimizationsResourceTestpatternDashboardResourceClientfor test API interactionsTechnical Decisions
Change checklist
Issues
Testing
All 23 tests passing in
DashboardsResourceTest:Create Operations (7 tests)
Read Operations (4 tests)
Update Operations (6 tests)
Delete Operations (4 tests)
Slug Generation (2 tests)
Run tests:
Documentation
No documentation updates required for this internal API. Future PRs will add: