-
Notifications
You must be signed in to change notification settings - Fork 17
[DAPS-1513] Rustlike JS #1523
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
base: devel
Are you sure you want to change the base?
[DAPS-1513] Rustlike JS #1523
Conversation
Reviewer's GuideImplements a Rust-compatible repository type system with enum-like types, explicit Result-based error handling, and trait-like operations; refactors the Foxx API and legacy Repo class to leverage these modular patterns; and adds comprehensive tests, examples, documentation, and a migration script. Class diagram for the new repository type systemclassDiagram
class RepositoryOps {
+validate(repository)
+createAllocation(repository, allocationParams)
+deleteAllocation(repository, subjectId)
+supportsDataOperations(repository)
+getCapacityInfo(repository)
+save(repository)
+update(repository, updates)
+find(repoId)
+list(filter)
+checkPermission(repository, userId, permission)
}
class Result {
+ok(value)
+err(error)
}
class RepositoryType {
<<enum>>
GLOBUS
METADATA_ONLY
}
class ExecutionMethod {
<<enum>>
TASK
DIRECT
}
class Repository {
type
data
}
class GlobusConfig {
endpoint
path
pub_key
address
exp_path
domain
}
class RepositoryData {
_key
_id
type
title
desc
capacity
admins
}
RepositoryData <|-- GlobusConfig : composition
RepositoryType <.. Repository : type
RepositoryData <.. Repository : data
Result <.. RepositoryOps
ExecutionMethod <.. Result
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
515fa84 to
2243fa3
Compare
core/database/foxx/api/repo.js
Outdated
|
|
||
| let repo = g_db._document(this.#repo_id); | ||
| if (!repo.path) { | ||
| const repoData = this.#repository.data; |
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.
Note
Context: The constructor now:
- Uses
RepositoryOps.find()to get the repository (line 86) - Stores the result in
this.#repositoryif found (line 90)
The old pattern let repo = g_db._document(this.#repo_id) would have directly fetched the document from the database. The new pattern already has this data stored in this.#repository from the
constructor, making it more efficient and consistent with the new repository type system.
The #repository private field contains a repository object that follows the new Rust-inspired pattern, which has a structure like:
{
type: "globus" | "metadata_only",
data: { /* actual repository data */ }
}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 a Rust-compatible design pattern system for the DataFed repository management, preparing the codebase for eventual Rust migration while maintaining JavaScript functionality. The implementation replaces inheritance-based patterns with composition and introduces explicit error handling through Result types.
- Creates modular repository type system with enum-like types (globus, metadata_only) and trait-like operations
- Implements factory patterns for type-based repository creation and Result types for explicit error propagation
- Maintains backward compatibility by refactoring the legacy Repo class to delegate to new repository operations internally
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/repository_types.test.js | Comprehensive unit tests covering type system functionality and operations |
| migrations/add_repository_type.js | Database migration script to backfill type field on existing repositories |
| api/repository/validation.js | Pure validation functions using Result pattern for error handling |
| api/repository/types.js | Core type definitions including RepositoryType enum and Result type |
| api/repository/operations.js | Trait-like repository operations with standardized interfaces |
| api/repository/metadata.js | Metadata-only repository implementation with synchronous operations |
| api/repository/globus.js | Globus repository implementation with asynchronous task-based operations |
| api/repository/factory.js | Factory pattern for type-based repository creation and dynamic dispatch |
| api/repository/example.js | Usage examples demonstrating Rust-compatible patterns |
| api/repository/README.md | Comprehensive documentation of design patterns and migration benefits |
| api/repo_router_new.js | New Foxx API router using repository type system |
| api/repo.js | Legacy Repo class refactored to use new repository operations internally |
77a982e to
ae39a84
Compare
b0dde8d to
341115c
Compare
| * @param {object} config - Globus configuration object | ||
| * @param {string} config.endpoint - Globus endpoint identifier | ||
| * @param {string} config.path - Repository path on filesystem | ||
| * @param {string} config.pub_key - Public SSH key for authentication |
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.
This is not an ssh key, we are using Curve25519, which is part of elliptic-curve cryptography that is used by ZeroMQ.
| if (!config.id || !config.type || !config.title || !config.capacity || !config.admins) { | ||
| return Result.err({ | ||
| code: g_lib.ERR_INVALID_PARAM, | ||
| message: "Missing required repository fields", |
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 would be better to identify which fields are found to be missing.
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.
Updated 🫡
| const globusRepo = require("./globus"); | ||
| const metadataRepo = require("./metadata"); |
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.
Shouldn't these two files be included in this PR, since they are included here? Sorry for the confusion, I know I'm creating work for you Aaron, just want to make sure I'm looking at things correctly.
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 placeholder for the changes in https://github.com/ORNL/DataFed/pull/1543/files. core/database/foxx/api/repository/globus.js and core/database/foxx/api/repository/metadata.js are in there
ab78fa9 to
ea59312
Compare
ea59312 to
0208cc0
Compare
|
All review items already addressed in code. Documentation simplified per feedback. LGTM |
dc486e5 to
4b538dc
Compare
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.
Please address these two items.
message: "Missing required repository fields",
@param {string} config.pub_key - Public SSH key for authentication
| * @param {string[]} config.admins - Array of admin user IDs | ||
| * @param {string} [config.endpoint] - Globus endpoint (required for GLOBUS type) | ||
| * @param {string} [config.path] - File path (required for GLOBUS type) | ||
| * @param {string} [config.pub_key] - Public SSH key (required for GLOBUS type) |
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.
This is not an ssh key, see below.
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.
4d1a056 to
33c787f
Compare
This reverts commit c5c2874. Revert "[DAPS-1531] Update comments" This reverts commit 464b0f1. Revert "[DAPS-1531] Address CI" This reverts commit 4d66b75. Revert "[DAPS-1531] Object to object" This reverts commit 5c39a8d. Revert "[DAPS-1531] Address Object -> object, address copilot comments" This reverts commit ae39a84.
… Rust-Compatible js
* [DAPS-1514] Add repository validation logic with tests. Add minimal Result type for Rust-like error handling. Implement pure validation functions for repository fields. Add comprehensive unit tests for all validation functions. Register validation tests in CMake configuration * [DAPS-1514] Run prettier
33c787f to
7c755f9
Compare

Ticket
Description
This PR implements a Rust-compatible design pattern system for the DataFed repository management, preparing the codebase for eventual Rust migration while maintaining JavaScript functionality.
Key changes:
-
types.js- Type definitions and data structures-
factory.js- Type-based repository creation-
operations.js- Trait-like repository operations-
validation.js- Pure validation functions-
globus.js/metadata.js- Type-specific implementationsHow Has This Been Tested?
Unit Tests - Created repository_types.test.js covering:
Integration Tests - Created test-foxx-api.sh:
Artifacts
API Response Examples:
Test Execution Output:
Summary by Sourcery
Introduce a Rust-compatible repository type system in JavaScript by modularizing repository logic into enum-like types, trait-like operations, and composition-based factories, update the Foxx API to use the new patterns while preserving legacy compatibility, and add supporting tests, examples, and documentation to facilitate a future Rust migration.
New Features:
typefield on existing repositoriesEnhancements:
Documentation:
Tests:
Chores: