-
Notifications
You must be signed in to change notification settings - Fork 16
[DAPS-1515] Integrate repository type system into core endpoints #1543
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: feat-DAPS-1513-rust-js-design
Are you sure you want to change the base?
[DAPS-1515] Integrate repository type system into core endpoints #1543
Conversation
b872681
to
2050419
Compare
adbf105
to
6d667b8
Compare
2050419
to
8607e8b
Compare
119eaa1
to
a4daf27
Compare
8607e8b
to
94f9d61
Compare
a4daf27
to
cc0a958
Compare
{ | ||
repo_type: repository.type, | ||
repo_id: repository.data._id, | ||
}, |
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 like that this information is being included in the exception.
871a716
to
6bb4228
Compare
cc0a958
to
b04e32d
Compare
b04e32d
to
f12d22b
Compare
e726e65
to
8e5ad6c
Compare
dd396d4
to
8b67bd3
Compare
@JoshuaSBrown Changes complete:
PTAL |
8e5ad6c
to
eb93c6d
Compare
4f6f23a
to
cde5516
Compare
@JoshuaSBrown Addressed all feedback:
Changes committed and CI passing (ignoring shfmt per team decision). Ready for re-review. |
|
||
// Check if repository supports data operations | ||
var findResult = RepositoryOps.find(repo_alloc._to); | ||
if (findResult.ok) { |
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.
What happens when findResult is not ok, shouldn't we be throwing?
if (obj.path && !obj.path.endsWith("/")) { | ||
obj.path += "/"; | ||
} |
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.
You are making sure that the .path ends with "/", in the original code, it is also checking to make sure that it begins with "/". Is that check being done somewhere else?
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 see it is in the validation.js file.
if (obj.path.substr(idx + 1, obj.path.length - idx - 2) != obj._key) | ||
throw [ | ||
g_lib.ERR_INVALID_PARAM, | ||
"Last part of repository path must be repository ID suffix (" + | ||
obj._key + | ||
")", | ||
]; |
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 contained in the validation.js file, in the validateRepositoryPath function.
if (!permResult.ok || !permResult.value) { | ||
throw g_lib.ERR_PERM_DENIED; | ||
} | ||
|
||
// Check if subject exists | ||
if (!g_db._exists(subject_id)) { | ||
throw [g_lib.ERR_NOT_FOUND, "Subject not found: " + subject_id]; |
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 clearer if we include in the exeception, that the allocation delete request failed, for audit reasons, in cases around authorization it is really useful to know who attempted to do what.
// For metadata-only repos, allocations are just database records | ||
// No actual storage allocation happens | ||
const allocation = { | ||
_key: `alloc_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`, |
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.
Why was this chosen for the key?
Arangodb will automatically create the key and id values, deviating from how the rest of the ArangoDB key and id values will introduce some level of technical debt that we might want to have justification for.
For instance here if you save allocation you will end up with an id like this
_id: 'alloc/alloc_data...blah..blah`
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.
_id: 'alloc/403756027'
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.
Just a placeholder, need to update that
const deleteAllocation = (repoData, subjectId) => { | ||
if (!subjectId || typeof subjectId !== "string") { | ||
return Result.err({ | ||
code: g_lib.ERR_INVALID_PARAM, | ||
message: "Subject ID is required for allocation deletion", | ||
}); | ||
} | ||
|
||
try { | ||
// For metadata-only repos, just remove the database record | ||
// No actual storage deallocation needed | ||
const result = { | ||
repo_id: repoData._id, | ||
subject: subjectId, | ||
status: "completed", | ||
message: "Metadata allocation removed", | ||
}; | ||
|
||
return Result.ok(createAllocationResult(ExecutionMethod.DIRECT, result)); | ||
} catch (e) { | ||
return Result.err({ | ||
code: g_lib.ERR_INTERNAL_FAULT, | ||
message: `Failed to delete metadata allocation: ${e.message}`, | ||
}); | ||
} | ||
}; |
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 take a look in taskInitAllocDelete, in the support.js file. Even though we are dealing with metadata only allocations, you will still need to remove all of the records and collections that are in there, otherwise you will end up with dangling documents.
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.
Maybe you are implementing that in a later PR? It doesn't look like you are removing the allocation document here.
repo_id: repoData._id, | ||
subject: params.subject, | ||
size: params.size, | ||
path: params.path || `/${params.subject}`, |
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.
What is the reason for having a path here, that is not some default size and also for not just setting the size to 0?
// Check if repository supports data operations | ||
var findResult = RepositoryOps.find(alloc._to); | ||
if (findResult.ok) { | ||
var repository = findResult.value; | ||
var dataOpsResult = RepositoryOps.supportsDataOperations(repository); | ||
|
||
// Skip metadata-only repositories | ||
if (dataOpsResult.ok && !dataOpsResult.value) { | ||
continue; | ||
} | ||
} | ||
|
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 should be ok to assign a metadata only repository when a recordCreate request comes in. We would probably want to check if there is raw data attached to the recordCreate request before picking a metadata only repository.
Note, if we only support metadata repositories and you don't enable this you will have to explicitly specify what allocation the record should be created in even if you only have the one. So we want to turn this on.
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.
return Result.ok({ | ||
total_capacity: repoData.capacity, | ||
used_capacity: 0, // Would be populated from actual usage | ||
available_capacity: repoData.capacity, |
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 this be capacity - used_capacity?
total_capacity: repoData.capacity, | ||
used_capacity: 0, // Would be populated from actual usage | ||
available_capacity: repoData.capacity, | ||
supports_quotas: true, |
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.
How will this be used, and what for?
subject: allocation.subject, | ||
size: allocation.size, | ||
path: allocation.path, | ||
status: "completed", |
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 have status entries defined in a centralized file that can be referenced by other parts of the code base, so that it is not ambigous how these items should be returned.
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 also not consistent with what is in the tasks.js and globus files. If you look in the support.js file you will see that there are these entried:
obj.TS_BLOCKED = 0;
obj.TS_READY = 1;
obj.TS_RUNNING = 2;
obj.TS_SUCCEEDED = 3;
obj.TS_FAILED = 4;
obj.TS_COUNT = 5;
They are not returned as strings but as codes. Either we need to be consistent with what is in here or also change what status is being returned by the globus status entry.
eb93c6d
to
69728ad
Compare
cde5516
to
db5f941
Compare
4d1a056
to
33c787f
Compare
7007e52
to
5e9fe8f
Compare
33c787f
to
7c755f9
Compare
…tterns - Implement RepositoryOps.find() for repository lookup - Add getRepository() to expose the new repository object - Adjust pathType() to handle metadata-only repositories - Preserve backward compatibility with existing API
- Implement repository type checks for data operations. - Replace allocation create/delete logic with RepositoryOps. - Add support for metadata-only repositories and relevant error handling. - Ensure API response compatibility is preserved.
f4f6690
to
50be6a3
Compare
Ticket
DAPS-1515
Description
The updates include integrating the new repository type system into existing API endpoints, adding type-aware operations for data uploads and allocations, and making production code changes that enable repository type features.
What's included:
data_router.js
repo_router.js
support.js
tasks.js
How Has This Been Tested?
See next PR
Artifacts
See CI