Repository navigation
Conversation
Reviewer's GuideThis PR implements lazy migration of the repository type to default to Globus—injecting missing type fields in RepositoryOps.find and update, setting the default on new repositories in the router, extends the Repo class for metadata-only repos without a path, and adds comprehensive tests for these behaviors. Sequence diagram for creating a new repository with default typesequenceDiagram
participant User
participant repo_router.js
participant g_db.repo
User->>repo_router.js: Create new repository
repo_router.js->>repo_router.js: Set type to GLOBUS
repo_router.js->>g_db.repo: Save repository
g_db.repo-->>repo_router.js: Return new repository
repo_router.js-->>User: Respond with repository
Class diagram for updated RepositoryOps and Repo classclassDiagram
class RepositoryOps {
+find(repoId)
+update(repository, updates)
}
class RepositoryType {
<<enum>>
GLOBUS
// ...other types
}
class Repo {
+type
+path
// ...other attributes
}
RepositoryOps --> RepositoryType
Repo --> RepositoryType
Flow diagram for lazy migration of repository typeflowchart TD
A[RepositoryOps.find or update called] --> B{Does repository have type?}
B -- Yes --> C[Return repository]
B -- No --> D[Set type to GLOBUS]
D --> C
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In repo_router.js you reference RepositoryType.GLOBUS but never import RepositoryType—add the required import from your types module to avoid a ReferenceError.
- Rather than unconditionally setting obj.type to Globus for all new repositories, only default it when no type is provided to avoid overwriting explicit type inputs.
- The find() migration sets type on the returned object but doesn’t persist it back to the database—consider whether you need to write the default type to the stored document for a proper migration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In repo_router.js you reference RepositoryType.GLOBUS but never import RepositoryType—add the required import from your types module to avoid a ReferenceError.
- Rather than unconditionally setting obj.type to Globus for all new repositories, only default it when no type is provided to avoid overwriting explicit type inputs.
- The find() migration sets type on the returned object but doesn’t persist it back to the database—consider whether you need to write the default type to the stored document for a proper migration.
## Individual Comments
### Comment 1
<location> `core/database/foxx/tests/operations.test.js:8` </location>
<code_context>
+const { RepositoryType } = require("../api/repository/types");
+const g_db = require("@arangodb").db;
+
+describe("RepositoryOps lazy migration", function () {
+ beforeEach(function () {
+ g_db.repo.truncate();
+ });
+
+ it("should default type to 'globus' when finding repo without type field", function () {
</code_context>
<issue_to_address>
Missing test for error handling in RepositoryOps.find and update.
Please add tests for scenarios where RepositoryOps.find and RepositoryOps.update encounter missing repositories or database errors to ensure robust error handling.
</issue_to_address>
### Comment 2
<location> `core/database/foxx/tests/operations.test.js:52` </location>
<code_context>
+ expect(updateResult.value.title).to.equal("Updated Title");
+ });
+
+ it("should add type field during update if missing", function () {
+ const legacyRepo = g_db.repo.save({
+ _key: "test-migration",
+ title: "Repository for Migration",
+ capacity: 500000,
+ });
+
+ const beforeUpdate = g_db.repo.document("test-migration");
+ expect(beforeUpdate.type).to.be.undefined;
+
+ const findResult = RepositoryOps.find("test-migration");
+ const updateResult = RepositoryOps.update(findResult.value, {
+ capacity: 600000,
+ });
+
+ expect(updateResult.ok).to.be.true;
+
+ const afterUpdate = g_db.repo.document("test-migration");
+ expect(afterUpdate.type).to.equal(RepositoryType.GLOBUS);
+ expect(afterUpdate.capacity).to.equal(600000);
+ });
+
</code_context>
<issue_to_address>
Consider adding a test for explicit type update.
Please add a test where the update sets the type explicitly to ensure the logic correctly preserves the provided type value.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
it("should add type field during update if missing", function () {
const legacyRepo = g_db.repo.save({
_key: "test-migration",
title: "Repository for Migration",
capacity: 500000,
});
const beforeUpdate = g_db.repo.document("test-migration");
expect(beforeUpdate.type).to.be.undefined;
const findResult = RepositoryOps.find("test-migration");
const updateResult = RepositoryOps.update(findResult.value, {
capacity: 600000,
});
expect(updateResult.ok).to.be.true;
const afterUpdate = g_db.repo.document("test-migration");
expect(afterUpdate.type).to.equal(RepositoryType.GLOBUS);
expect(afterUpdate.capacity).to.equal(600000);
});
=======
it("should add type field during update if missing", function () {
const legacyRepo = g_db.repo.save({
_key: "test-migration",
title: "Repository for Migration",
capacity: 500000,
});
const beforeUpdate = g_db.repo.document("test-migration");
expect(beforeUpdate.type).to.be.undefined;
const findResult = RepositoryOps.find("test-migration");
const updateResult = RepositoryOps.update(findResult.value, {
capacity: 600000,
});
expect(updateResult.ok).to.be.true;
const afterUpdate = g_db.repo.document("test-migration");
expect(afterUpdate.type).to.equal(RepositoryType.GLOBUS);
expect(afterUpdate.capacity).to.equal(600000);
});
it("should preserve explicit type value during update", function () {
const legacyRepo = g_db.repo.save({
_key: "test-explicit-type",
title: "Repository for Explicit Type",
capacity: 100000,
});
const beforeUpdate = g_db.repo.document("test-explicit-type");
expect(beforeUpdate.type).to.be.undefined;
const findResult = RepositoryOps.find("test-explicit-type");
const updateResult = RepositoryOps.update(findResult.value, {
type: RepositoryType.S3,
capacity: 200000,
});
expect(updateResult.ok).to.be.true;
const afterUpdate = g_db.repo.document("test-explicit-type");
expect(afterUpdate.type).to.equal(RepositoryType.S3);
expect(afterUpdate.capacity).to.equal(200000);
});
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| describe("RepositoryOps lazy migration", function () { | ||
| beforeEach(function () { | ||
| g_db.repo.truncate(); | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Missing test for error handling in RepositoryOps.find and update.
Please add tests for scenarios where RepositoryOps.find and RepositoryOps.update encounter missing repositories or database errors to ensure robust error handling.
| it("should add type field during update if missing", function () { | ||
| const legacyRepo = g_db.repo.save({ | ||
| _key: "test-migration", | ||
| title: "Repository for Migration", | ||
| capacity: 500000, | ||
| }); | ||
|
|
||
| const beforeUpdate = g_db.repo.document("test-migration"); | ||
| expect(beforeUpdate.type).to.be.undefined; | ||
|
|
||
| const findResult = RepositoryOps.find("test-migration"); | ||
| const updateResult = RepositoryOps.update(findResult.value, { | ||
| capacity: 600000, | ||
| }); | ||
|
|
||
| expect(updateResult.ok).to.be.true; | ||
|
|
||
| const afterUpdate = g_db.repo.document("test-migration"); | ||
| expect(afterUpdate.type).to.equal(RepositoryType.GLOBUS); | ||
| expect(afterUpdate.capacity).to.equal(600000); | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for explicit type update.
Please add a test where the update sets the type explicitly to ensure the logic correctly preserves the provided type value.
| it("should add type field during update if missing", function () { | |
| const legacyRepo = g_db.repo.save({ | |
| _key: "test-migration", | |
| title: "Repository for Migration", | |
| capacity: 500000, | |
| }); | |
| const beforeUpdate = g_db.repo.document("test-migration"); | |
| expect(beforeUpdate.type).to.be.undefined; | |
| const findResult = RepositoryOps.find("test-migration"); | |
| const updateResult = RepositoryOps.update(findResult.value, { | |
| capacity: 600000, | |
| }); | |
| expect(updateResult.ok).to.be.true; | |
| const afterUpdate = g_db.repo.document("test-migration"); | |
| expect(afterUpdate.type).to.equal(RepositoryType.GLOBUS); | |
| expect(afterUpdate.capacity).to.equal(600000); | |
| }); | |
| it("should add type field during update if missing", function () { | |
| const legacyRepo = g_db.repo.save({ | |
| _key: "test-migration", | |
| title: "Repository for Migration", | |
| capacity: 500000, | |
| }); | |
| const beforeUpdate = g_db.repo.document("test-migration"); | |
| expect(beforeUpdate.type).to.be.undefined; | |
| const findResult = RepositoryOps.find("test-migration"); | |
| const updateResult = RepositoryOps.update(findResult.value, { | |
| capacity: 600000, | |
| }); | |
| expect(updateResult.ok).to.be.true; | |
| const afterUpdate = g_db.repo.document("test-migration"); | |
| expect(afterUpdate.type).to.equal(RepositoryType.GLOBUS); | |
| expect(afterUpdate.capacity).to.equal(600000); | |
| }); | |
| it("should preserve explicit type value during update", function () { | |
| const legacyRepo = g_db.repo.save({ | |
| _key: "test-explicit-type", | |
| title: "Repository for Explicit Type", | |
| capacity: 100000, | |
| }); | |
| const beforeUpdate = g_db.repo.document("test-explicit-type"); | |
| expect(beforeUpdate.type).to.be.undefined; | |
| const findResult = RepositoryOps.find("test-explicit-type"); | |
| const updateResult = RepositoryOps.update(findResult.value, { | |
| type: RepositoryType.S3, | |
| capacity: 200000, | |
| }); | |
| expect(updateResult.ok).to.be.true; | |
| const afterUpdate = g_db.repo.document("test-explicit-type"); | |
| expect(afterUpdate.type).to.equal(RepositoryType.S3); | |
| expect(afterUpdate.capacity).to.equal(200000); | |
| }); |
4f6f23a to
cde5516
Compare
c7e29d8 to
39ca1d1
Compare
cde5516 to
db5f941
Compare
27ec97e to
9233377
Compare
7007e52 to
5e9fe8f
Compare
9233377 to
120b760
Compare
f4f6690 to
50be6a3
Compare
120b760 to
ef7a67d
Compare
Ticket
DAPS-1516
Description
RepositoryOps.find()to set missing types to "globus".RepositoryOps.update()to add type if missing.repo_router.jscreate endpoint to set type for new repos.How Has This Been Tested?
operations.test.js&repo.test.jsManually
docker-compose -f compose/local/compose.yml exec -T database arangosh --server.password lolArtifacts (if appropriate):
See CI