1705 daps feature foxx integrate metadata globus factory repo router create #1706
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ove circular dependency from validation.js by removing validateRepositorySupportsDataOperations Co-authored-by: Aaron Perez <[email protected]>
…data (#1603) Co-authored-by: Aaron Perez <[email protected]>
Reviewer's GuideThis PR introduces a new class-based repository model layer under api/models/repositories with Rust-style Result handling, integrates it into the Foxx router via a factory, refactors validation and permissions, and overhauls the test suite and CMake targets accordingly. Sequence diagram for repository creation via Foxx router and factorysequenceDiagram
participant User as actor User
participant Router as "Repo Router"
participant Factory as "Repositories (Factory)"
participant RepoClass as "Repository Class (GlobusRepo/MetadataRepo)"
participant DB as "ArangoDB"
User->>Router: POST /repo (create repository)
Router->>Factory: createRepositoryByType(config)
Factory->>RepoClass: new Repository(config)
RepoClass->>DB: Check if repo exists
alt Repo exists
RepoClass->>DB: Load existing repo
else Repo does not exist
RepoClass->>DB: Prepare new repo data
end
RepoClass->>Router: Return Result (ok/error)
Router->>DB: Save repository
Router->>User: Respond with repository info
ER diagram for repository and allocation entitieserDiagram
REPO {
string _id
string _key
string type
string title
string desc
int capacity
}
ALLOC {
string _id
string _from
string _to
int data_limit
int rec_limit
int rec_count
int data_size
string path
string type
}
REPO ||--o{ ALLOC : "allocations"
ALLOC }o--|| REPO : "allocated to repo"
Class diagram for the new repository model layerclassDiagram
class BaseRepository {
+constructor(config, typeSpecificConfig)
+id()
+save()
+update(updates)
+type()
+createAllocation(allocationParams)
+deleteAllocation(subjectId)
+supportsDataOperations()
+getCapacityInfo()
}
class GlobusRepo {
+constructor(config)
+type()
+createAllocation(params)
+deleteAllocation(client, subject)
+supportsDataOperations()
+getCapacityInfo()
+validate(config)
}
class MetadataRepo {
+constructor(config)
+type()
+createAllocation(params)
+deleteAllocation(client, subject)
+supportsDataOperations()
+getCapacityInfo()
+validate(config)
}
class Repositories {
+static createRepositoryByType(config)
+static find(repoId)
+static list(filter)
}
BaseRepository <|-- GlobusRepo
BaseRepository <|-- MetadataRepo
Repositories --> GlobusRepo
Repositories --> MetadataRepo
Class diagram for Result and types utilitiesclassDiagram
class Result {
+ok(value)
+err(error)
}
class RepositoryType {
<<enumeration>>
GLOBUS
METADATA
}
class ExecutionMethod {
<<enumeration>>
DEFERRED
DIRECT
}
class AllocationResult {
+execution_method
+task
+result
}
Result ..> AllocationResult
AllocationResult ..> ExecutionMethod
AllocationResult ..> RepositoryType
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected directly writing to a Response object from user-defined input. This bypasses any HTML escaping and may expose your application to a Cross-Site-scripting (XSS) vulnerability. Instead, use 'resp.render()' to render safely escaped HTML. (link)
General comments:
- Remove all debug console.log statements and commented-out code before merging to avoid log spam and keep the codebase clean.
- Review and unify repository key references in repo_router (using obj.key vs obj._key) to ensure the path validation and key assignment logic still works correctly.
- Double-check the CMakeLists test definitions—adding trailing colons to the test names may break the test discovery in test_foxx.sh.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove all debug console.log statements and commented-out code before merging to avoid log spam and keep the codebase clean.
- Review and unify repository key references in repo_router (using obj.key vs obj._key) to ensure the path validation and key assignment logic still works correctly.
- Double-check the CMakeLists test definitions—adding trailing colons to the test names may break the test discovery in test_foxx.sh.
## Individual Comments
### Comment 1
<location> `core/database/foxx/api/repo_router.js:133-134` </location>
<code_context>
- error.ERR_INVALID_PARAM,
- "Repository path must be an absolute path file system path.",
- ];
+ if (req.body?.type == undefined || req.body?.type == RepositoryType.GLOBUS) {
+ obj["type"] = RepositoryType.GLOBUS;
+ console.log("create 6");
+ g_lib.procInputParam(req.body, "domain", false, obj);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Defaulting to GLOBUS type when type is undefined may mask client errors.
Explicitly require the type field and return an error if it is missing, unless backward compatibility necessitates defaulting to GLOBUS.
</issue_to_address>
### Comment 2
<location> `core/database/foxx/api/repo_router.js:143-145` </location>
<code_context>
- ];
+ if (!obj.path.endsWith("/")) obj.path += "/";
- if (req.body.exp_path) {
- obj.exp_path = req.body.exp_path;
- if (!obj.exp_path.endsWith("/")) obj.path += "/";
+ var idx = obj.path.lastIndexOf("/", obj.path.length - 2);
+ if (obj.path.substr(idx + 1, obj.path.length - idx - 2) != obj.key)
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential bug: appending '/' to obj.path instead of obj.exp_path.
Please check if '/' should be appended to exp_path instead of path to avoid incorrect path assignment.
</issue_to_address>
### Comment 3
<location> `core/database/foxx/api/repo_router.js:134-143` </location>
<code_context>
- if (req.body.exp_path) {
- obj.exp_path = req.body.exp_path;
- if (!obj.exp_path.endsWith("/")) obj.path += "/";
+ var idx = obj.path.lastIndexOf("/", obj.path.length - 2);
+ if (obj.path.substr(idx + 1, obj.path.length - idx - 2) != obj.key)
+ throw [
+ error.ERR_INVALID_PARAM,
</code_context>
<issue_to_address>
**issue:** Inconsistent use of obj.key and obj._key in error message.
The error message should reference obj.key instead of obj._key to avoid confusion and ensure consistency with the validation logic.
</issue_to_address>
### Comment 4
<location> `core/database/foxx/api/repo_router.js:184-197` </location>
<code_context>
+ let repo_doc = repo_doc_result.value;
+ console.log("create 13");
for (var i in req.body.admins) {
+ console.log("create 14");
if (!g_db._exists(req.body.admins[i]))
throw [
error.ERR_NOT_FOUND,
"User, " + req.body.admins[i] + ", not found",
];
+ console.log("create 15");
g_db.admin.save({
- _from: repo._id,
+ _from: repo.id(),
</code_context>
<issue_to_address>
**suggestion:** Using for...in to iterate over array may cause unexpected behavior.
'for...in' may iterate over inherited or custom properties, not just array elements. Use a standard for loop or 'for...of' for safer array iteration.
```suggestion
for (const admin of req.body.admins) {
console.log("create 14");
if (!g_db._exists(admin))
throw [
error.ERR_NOT_FOUND,
"User, " + admin + ", not found",
];
console.log("create 15");
g_db.admin.save({
_from: repo.id(),
_to: admin,
});
}
```
</issue_to_address>
### Comment 5
<location> `core/database/foxx/api/models/repositories/base_repository.js:43-44` </location>
<code_context>
+
+class BaseRepository {
+ constructor(config, typeSpecificConfig) {
+ if (new.target === BaseRepository) {
+ return Result.err({
+ code: error.ERR_INTERNAL_FAULT,
+ message: "BaseRepository cannot be instantiated directly",
</code_context>
<issue_to_address>
**issue (bug_risk):** Returning Result.err from constructor may break subclass instantiation expectations.
Throwing an error or using a factory method is preferable for enforcing abstract class constraints, as returning a value from a constructor is non-standard and may cause issues with subclassing.
</issue_to_address>
### Comment 6
<location> `core/database/foxx/api/models/repositories/base_repository.js:185-188` </location>
<code_context>
+ console.log(key);
+ console.log(this.repoData);
+ if (key != undefined) {
+ if (g_db._exists(id)) {
+ const updated = g_db.repo.update(
+ { _key: key, ...repo_data },
+ { returnNew: true },
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Update operation may overwrite fields unintentionally due to object spread.
Explicitly define updatable fields to prevent unintended overwrites and ensure data integrity.
```suggestion
// Define updatable fields
const updatableFields = ['name', 'description', 'url', 'owner']; // Add all allowed fields here
const updateData = { _key: key };
updatableFields.forEach(field => {
if (repo_data.hasOwnProperty(field)) {
updateData[field] = repo_data[field];
}
});
const updated = g_db.repo.update(
updateData,
{ returnNew: true },
);
```
</issue_to_address>
### Comment 7
<location> `core/database/foxx/api/models/repositories/repository/globus.js:86-90` </location>
<code_context>
+
+ console.log("Normalized COnfig");
+ console.log(normalizedConfig);
+ const result = super(normalizedConfig, globusConfig);
+ if (result.ok == false) {
+ return result;
+ }
+ this.repoData = result.value.repoData;
+
+ console.log("repoData after calling super");
</code_context>
<issue_to_address>
**nitpick:** Excessive use of console.log for debugging may clutter logs in production.
Consider removing or gating the console.log statements with a debug flag to prevent unnecessary output in production.
</issue_to_address>
### Comment 8
<location> `core/database/foxx/api/repo_router.js:206` </location>
<code_context>
res.send([repo_doc]);
</code_context>
<issue_to_address>
**security (javascript.express.security.audit.xss.direct-response-write):** Detected directly writing to a Response object from user-defined input. This bypasses any HTML escaping and may expose your application to a Cross-Site-scripting (XSS) vulnerability. Instead, use 'resp.render()' to render safely escaped HTML.
*Source: opengrep*
</issue_to_address>
### Comment 9
<location> `core/database/foxx/api/models/repositories/base_repository.js:62-66` </location>
<code_context>
if (config.id !== undefined) {
if (config.id.startsWith("repo/") && config.id.length > "repo/".length) {
id_defined = true;
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))
```suggestion
if (config.id !== undefined && (config.id.startsWith("repo/") && config.id.length > "repo/".length)) {
id_defined = true;
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 10
<location> `core/database/foxx/api/models/repositories/base_repository.js:73-80` </location>
<code_context>
if (key_defined && id_defined) {
if (config.id !== `repo/${config.key}`) {
return Result.err({
code: error.ERR_INVALID_PARAM,
message: `BaseRepository - provided key ${config.key} is in conflict with id ${config.id}.`,
});
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))
```suggestion
if (key_defined && id_defined && config.id !== `repo/${config.key}`) {
return Result.err({
code: error.ERR_INVALID_PARAM,
message: `BaseRepository - provided key ${config.key} is in conflict with id ${config.id}.`,
});
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 11
<location> `core/database/foxx/api/models/repositories/repository/globus.js:128` </location>
<code_context>
const task = taskResult.task;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {task} = taskResult;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 12
<location> `core/database/foxx/api/models/repositories/repository/metadata.js:95-98` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 13
<location> `core/database/foxx/api/models/repositories/repository/metadata.js:169` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 14
<location> `core/database/foxx/api/models/repositories/repository/metadata.js:208-216` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 15
<location> `core/database/foxx/api/repo_router.js:115-123` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 16
<location> `core/database/foxx/api/repo_router.js:147` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 17
<location> `core/database/foxx/tests/base_repository.test.js:29-37` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 18
<location> `core/database/foxx/tests/base_repository.test.js:38-46` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 19
<location> `core/database/foxx/tests/base_repository.test.js:47-53` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 20
<location> `core/database/foxx/tests/repo_globus.test.js:38-50` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 21
<location> `core/database/foxx/tests/repo_globus.test.js:52-64` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 22
<location> `core/database/foxx/tests/repo_globus.test.js:66-75` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 23
<location> `core/database/foxx/tests/repo_globus.test.js:77-86` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 24
<location> `core/database/foxx/tests/repo_globus.test.js:88-97` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 25
<location> `core/database/foxx/tests/repo_globus.test.js:99-111` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 26
<location> `core/database/foxx/tests/repo_globus.test.js:113-119` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 27
<location> `core/database/foxx/tests/repo_globus.test.js:121-131` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 28
<location> `core/database/foxx/tests/repo_globus.test.js:133-143` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 29
<location> `core/database/foxx/tests/repo_metadata.test.js:26-33` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 30
<location> `core/database/foxx/tests/repo_metadata.test.js:35-40` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
143
to
145
| ]; | ||
|
|
||
| var idx = obj.path.lastIndexOf("/", obj.path.length - 2); | ||
| if (obj.path.substr(idx + 1, obj.path.length - idx - 2) != obj._key) | ||
| throw [ | ||
| error.ERR_INVALID_PARAM, | ||
| "Last part of repository path must be repository ID suffix (" + | ||
| obj._key + | ||
| ")", | ||
| ]; | ||
| if (!obj.path.endsWith("/")) obj.path += "/"; |
There was a problem hiding this comment.
issue (bug_risk): Potential bug: appending '/' to obj.path instead of obj.exp_path.
Please check if '/' should be appended to exp_path instead of path to avoid incorrect path assignment.
Comment on lines
43
to
44
| if (new.target === BaseRepository) { | ||
| return Result.err({ |
There was a problem hiding this comment.
issue (bug_risk): Returning Result.err from constructor may break subclass instantiation expectations.
Throwing an error or using a factory method is preferable for enforcing abstract class constraints, as returning a value from a constructor is non-standard and may cause issues with subclassing.
JoshuaSBrown
force-pushed
the
1705-DAPS-feature-foxx-integrate-metadata-globus-factory-repo-router-create
branch
from
7e27a13 to
d1e5c1f
Compare
JoshuaSBrown
added this to the
Objective - Enable Metadata Only Mode in DataFed milestone
JoshuaSBrown
force-pushed
the
1705-DAPS-feature-foxx-integrate-metadata-globus-factory-repo-router-create
branch
from
adb1bb0 to
3caf6ca
Compare
JoshuaSBrown
modified the milestones:
Objective - Enable Metadata Only Mode in DataFed,
Log Cleanup
JoshuaSBrown
commented
Oct 23, 2025
JoshuaSBrown
deleted the
1705-DAPS-feature-foxx-integrate-metadata-globus-factory-repo-router-create
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
Description
How Has This Been Tested?
Artifacts (if appropriate):
Tasks
Summary by Sourcery
Implement a new repository abstraction layer for Foxx service that cleanly separates metadata and Globus-backed repositories, introduces a factory for creating them, updates the router to use the models directly, reorganizes validation code, and adds comprehensive unit and integration tests
New Features:
Enhancements:
Build:
Tests: