Skip to content
This repository was archived by the owner on Oct 19, 2026. It is now read-only.

Commit 683c28d

Browse files
WiedersehenMAcfboygenednaIvanbeethoven
authored
refactor(lfs): improve API structure and add docs/tests (#1716)
* refactor(lfs): improve API structure and add docs/tests - Refactored parts of LFS API handlers for better structure - Added OpenAPI documentation and developer notes - Added unit tests for LFS batch and object endpoints - Ensured compatibility with GitHub LFS behavior Signed-off-by: WiedersehenM <your_email@example.com> Signed-off-by: Hongze Gao <15101764808@163.com> * fix(lfs): resolve clippy, rustfmt and test failures - Remove unused `routes` import from utoipa_axum - Rename unused variables to underscore prefix (router, expected_code) - Reorder imports to satisfy rustfmt - Split long use statements into multiple lines - Trim trailing whitespaces and normalize blank lines - Derive PartialEq for lfs_structs::Ref to enable assert_eq! in tests Signed-off-by: Hongze Gao <15101764808@163.com> * fix(lfs): address clippy, fmt, naming, tests and docs - Remove unused `put` import (method comes from Route trait) - Let rustfmt reorder imports automatically - Rename `router()` -> `routers()` to keep consistent with other modules - Update callers in http_server.rs - test_error_response_format: assert against expected_code directly - test_map_lfs_error_priority: correct logic & comments for "Not found" priority - OpenAPI: fix download/chunk content-type to application/octet-stream - Correct chunk-download path template in docs: .../{oid}/chunks/{chunk_id} Signed-off-by: Hongze Gao <15101764808@163.com> * style(lfs): reorder imports to satisfy cargo fmt - Move ahead of the block - Keep order: std -> axum::routing -> axum -> futures -> utoipa_axum Signed-off-by: Hongze Gao <15101764808@163.com> * Integrated Buck2-Change-Detector to orion-server and orion workflow (#1721) * feat: integrate buck2-change-detector into orion build progress Signed-off-by: Acfboy <acfboyu@outlook.com> * fix: some implementation issues Signed-off-by: Acfboy <acfboyu@outlook.com> * fix: clippy Signed-off-by: Acfboy <acfboyu@outlook.com> --------- Signed-off-by: Acfboy <acfboyu@outlook.com> Co-authored-by: Quanyi Ma <eli@patch.sh> * [antares]: change server router bind and add func for orion (#1725) Signed-off-by: Luxian <lux1an@qq.com> * feat(lfs): sync upstream and align error handling, docs, tests 1. Merge latest upstream main 2. Introduce `lfs_error_response` helper – all routes now consistently return JSON `{"message": "..."}` instead of mixed `(code, msg)` or plain text - Keep `map_lfs_error` with TODO for future typed-error refactor 3. Remove LFS OpenAPI tags/paths/schemas append in http_server.rs; stick to project-wide convention 4. Adjust `test_map_lfs_error_priority` to real precedence ("Not found" first) 5. OpenAPI & responses polish - Set download/chunk Content-Type to application/octet-stream - Add 400/404 responses for every route: list_locks, list_locks_for_verification, create_lock, delete_lock, batch, fetch_chunk_ids, download_object, download_chunk, upload_object - Document dual-path support: /info/lfs and /api/v1/lfs Signed-off-by: Hongze Gao <15101764808@163.com> * fix: resolve all clippy warnings Signed-off-by: Hongze Gao <15101764808@163.com> Signed-off-by: Hongze Gao <15101764808@163.com> * fix: resolve all Testing Mono errors Signed-off-by: Hongze Gao <15101764808@163.com> Signed-off-by: Hongze Gao <15101764808@163.com> * fix(lfs): correct error handling & response format 1. delete_lock: replace hard-coded INTERNAL_SERVER_ERROR with map_lfs_error, now returns 404/400 as documented in OpenAPI. 2. lfs_upload_object: use lfs_error_response to guarantee correct Content-Type: application/vnd.git-lfs+json and JSON body. 3. test_error_response_format: add unit tests for status code, header and body structure of lfs_error_response. 4. annotate smoke tests (test_lfs_routes_structure, test_lfs_router_paths) to clarify they only ensure routes compile without panic. 5. unify rustdoc comment format across all touched functions. 6. move User struct doc comment above derive macro in lfs_structs.rs. Signed-off-by: Hongze Gao <15101764808@163.com> * refactor(lfs): improve API documentation and fix OpenAPI registration - Add LFS_TAG constant to http_server.rs for consistency with other routers - Remove non-standard tags from ApiDoc struct - Fix path duplication issue by using relative paths in utoipa::path annotations - Register only /api/v1/lfs paths in OpenAPI, /info/lfs for runtime only - Switch from .route() to routes! macro for better OpenAPI collection - Add descriptions to all LFS endpoints documenting dual path support - Fix chunk download error format to use standard LFS JSON response - Fix compilation errors (unused imports, api_state borrowing) This ensures LFS API endpoints are properly displayed in Swagger UI with correct paths (/api/v1/lfs/*) while maintaining runtime compatibility with standard Git LFS paths (/info/lfs/*). Signed-off-by: Hongze Gao <15101764808@163.com> * resolve Rustfmt Check errors Signed-off-by: Hongze Gao <15101764808@163.com> Signed-off-by: Hongze Gao <15101764808@163.com> --------- Signed-off-by: WiedersehenM <your_email@example.com> Signed-off-by: Hongze Gao <15101764808@163.com> Signed-off-by: Acfboy <acfboyu@outlook.com> Signed-off-by: Luxian <lux1an@qq.com> Co-authored-by: Acfboy <acfboyu@outlook.com> Co-authored-by: Quanyi Ma <eli@patch.sh> Co-authored-by: Xiaoyang Han <lux1an@qq.com>
1 parent 74044f6 commit 683c28d

6 files changed

Lines changed: 1577 additions & 854 deletions

File tree

‎ceres/src/lfs/handler.rs‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -676,3 +676,86 @@ async fn lfs_check_sub_oid_exist(
676676
let result = storage.get_lfs_relations_ori_oid(sub_oid).await.unwrap();
677677
Ok(!result.is_empty())
678678
}
679+
680+
#[cfg(test)]
681+
mod tests {
682+
use super::*;
683+
use crate::lfs::lfs_structs::{Action, Ref, ResCondition, ResponseObject};
684+
685+
#[test]
686+
fn response_object_download_existing() {
687+
let meta = MetaObject {
688+
oid: "oid1".into(),
689+
size: 10,
690+
exist: true,
691+
splited: false,
692+
};
693+
let res = ResponseObject::new(
694+
&meta,
695+
ResCondition {
696+
file_exist: true,
697+
operation: Operation::Download,
698+
use_tus: false,
699+
},
700+
"http://dl",
701+
"http://ul",
702+
);
703+
assert!(res.actions.is_some());
704+
let actions = res.actions.unwrap();
705+
assert!(actions.contains_key(&Action::Download));
706+
assert!(res.error.is_none());
707+
}
708+
709+
#[test]
710+
fn response_object_upload_new() {
711+
let meta = MetaObject {
712+
oid: "oid2".into(),
713+
size: 20,
714+
exist: false,
715+
splited: false,
716+
};
717+
let res = ResponseObject::new(
718+
&meta,
719+
ResCondition {
720+
file_exist: false,
721+
operation: Operation::Upload,
722+
use_tus: false,
723+
},
724+
"http://dl",
725+
"http://ul",
726+
);
727+
let actions = res.actions.expect("upload should provide actions");
728+
assert!(actions.contains_key(&Action::Upload));
729+
assert!(res.error.is_none());
730+
}
731+
732+
#[test]
733+
fn response_object_download_missing_sets_error() {
734+
let meta = MetaObject {
735+
oid: "oid3".into(),
736+
size: 30,
737+
exist: false,
738+
splited: false,
739+
};
740+
let res = ResponseObject::new(
741+
&meta,
742+
ResCondition {
743+
file_exist: false,
744+
operation: Operation::Download,
745+
use_tus: false,
746+
},
747+
"http://dl",
748+
"http://ul",
749+
);
750+
assert!(res.actions.is_none());
751+
assert!(res.error.is_some());
752+
assert_eq!(res.error.unwrap().code, 404);
753+
}
754+
755+
#[test]
756+
fn unlock_request_defaults() {
757+
let req = UnlockRequest::default();
758+
assert!(req.force.is_none());
759+
assert_eq!(req.refs, Ref { name: "".into() });
760+
}
761+
}

‎ceres/src/lfs/lfs_structs.rs‎

Lines changed: 43 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,9 @@ use chrono::{DateTime, Duration, Utc};
44
use common::config::LFSConfig;
55
use serde::{Deserialize, Serialize};
66
use std::collections::HashMap;
7+
use utoipa::ToSchema;
78

8-
#[derive(Serialize, Deserialize, Debug, Default)]
9+
#[derive(Serialize, Deserialize, Debug, Default, ToSchema)]
910
pub enum TransferMode {
1011
#[default]
1112
#[serde(rename = "basic")]
@@ -16,7 +17,7 @@ pub enum TransferMode {
1617
STREAMING,
1718
}
1819

19-
#[derive(Serialize, Deserialize, PartialEq, Eq, Hash, Debug, Clone)]
20+
#[derive(Serialize, Deserialize, PartialEq, Eq, Hash, Debug, Clone, ToSchema)]
2021
pub enum Operation {
2122
#[serde(rename = "download")]
2223
Download,
@@ -28,7 +29,7 @@ pub enum Operation {
2829
/// Upload operations can specify an upload and a verify action.
2930
/// The upload action describes how to upload the object. If the object has a verify action, the LFS client will hit this URL after a successful upload. Servers can use this for extra verification, if needed.
3031
/// If a client requests to upload an object that the server already has, the server should omit the actions property completely. The client will then assume the server already has it.
31-
#[derive(Serialize, Deserialize, PartialEq, Eq, Hash, Debug)]
32+
#[derive(Serialize, Deserialize, PartialEq, Eq, Hash, Debug, ToSchema)]
3233
pub enum Action {
3334
#[serde(rename = "download")]
3435
Download,
@@ -84,7 +85,8 @@ impl MetaObject {
8485
}
8586
}
8687

87-
#[derive(Serialize, Deserialize, Debug, Default)]
88+
#[derive(Serialize, Deserialize, Debug, Default, ToSchema)]
89+
/// Request object for LFS operations
8890
pub struct RequestObject {
8991
pub oid: String,
9092
pub size: i64,
@@ -98,20 +100,23 @@ pub struct RequestObject {
98100
pub authorization: String,
99101
}
100102

101-
#[derive(Serialize, Deserialize, Debug, Clone)]
103+
#[derive(Serialize, Deserialize, Debug, Clone, ToSchema)]
104+
/// LFS lock information
102105
pub struct Lock {
103106
pub id: String,
104107
pub path: String,
105108
pub locked_at: String,
106109
pub owner: Option<User>,
107110
}
108111

109-
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
112+
/// User information for lock ownership
113+
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, ToSchema)]
110114
pub struct User {
111115
pub name: String,
112116
}
113117

114-
#[derive(Serialize, Deserialize, Debug)]
118+
#[derive(Serialize, Deserialize, Debug, ToSchema)]
119+
/// Batch request for LFS operations
115120
pub struct BatchRequest {
116121
// Should be download or upload.
117122
pub operation: Operation,
@@ -122,21 +127,24 @@ pub struct BatchRequest {
122127
pub hash_algo: String,
123128
}
124129

125-
#[derive(Serialize, Deserialize)]
130+
#[derive(Serialize, Deserialize, ToSchema)]
131+
/// Batch response for LFS operations
126132
pub struct BatchResponse {
127133
pub transfer: TransferMode,
128134
pub objects: Vec<ResponseObject>,
129135
pub hash_algo: String,
130136
}
131137

132-
#[derive(Serialize, Deserialize)]
133-
pub struct FetchchunkResponse {
138+
#[derive(Serialize, Deserialize, ToSchema)]
139+
/// Response for fetching chunk IDs
140+
pub struct FetchChunkResponse {
134141
pub oid: String,
135142
pub size: i64,
136143
pub chunks: Vec<ChunkDownloadObject>,
137144
}
138145

139-
#[derive(Serialize, Deserialize, Clone)]
146+
#[derive(Serialize, Deserialize, Clone, ToSchema)]
147+
/// Link information for LFS object transfer
140148
pub struct Link {
141149
pub href: String,
142150
#[serde(default)] // Optional field
@@ -160,13 +168,15 @@ impl Link {
160168
}
161169
}
162170

163-
#[derive(Serialize, Deserialize, Default)]
171+
#[derive(Serialize, Deserialize, Default, ToSchema)]
172+
/// Error information for LFS object operations
164173
pub struct ObjectError {
165174
pub code: i64,
166175
pub message: String,
167176
}
168177

169-
#[derive(Serialize, Deserialize)]
178+
#[derive(Serialize, Deserialize, ToSchema)]
179+
/// Response object for LFS batch operations
170180
pub struct ResponseObject {
171181
pub oid: String,
172182
pub size: i64,
@@ -261,67 +271,77 @@ impl ResponseObject {
261271
}
262272
}
263273

264-
#[derive(Serialize, Deserialize)]
274+
#[derive(Serialize, Deserialize, ToSchema)]
275+
/// Chunk download object information
265276
pub struct ChunkDownloadObject {
266277
pub sub_oid: String,
267278
pub offset: i64,
268279
pub size: i64,
269280
pub link: Link,
270281
}
271282

272-
#[derive(Serialize, Deserialize, Debug, Default)]
283+
#[derive(Serialize, Deserialize, Debug, Default, PartialEq, ToSchema)]
284+
/// Git reference information
273285
pub struct Ref {
274286
pub name: String,
275287
}
276288

277-
#[derive(Serialize, Deserialize, Debug, Default)]
289+
#[derive(Serialize, Deserialize, Debug, Default, ToSchema)]
290+
/// Request to create a lock
278291
pub struct LockRequest {
279292
pub path: String,
280293
#[serde(rename(serialize = "ref", deserialize = "ref"))]
281294
pub refs: Ref,
282295
}
283296

284-
#[derive(Serialize, Deserialize)]
297+
#[derive(Serialize, Deserialize, ToSchema)]
298+
/// Response after creating a lock
285299
pub struct LockResponse {
286300
pub lock: Lock,
287301
pub message: String,
288302
}
289303

290-
#[derive(Serialize, Deserialize, Default)]
304+
#[derive(Serialize, Deserialize, Default, ToSchema)]
305+
/// Request to unlock a file
291306
pub struct UnlockRequest {
292307
pub force: Option<bool>,
293308
#[serde(rename(serialize = "ref", deserialize = "ref"))]
294309
pub refs: Ref,
295310
}
296311

297-
#[derive(Serialize, Deserialize, Debug)]
312+
#[derive(Serialize, Deserialize, Debug, ToSchema)]
313+
/// Response after unlocking a file
298314
pub struct UnlockResponse {
299315
pub lock: Lock,
300316
pub message: String,
301317
}
302318

303-
#[derive(Serialize, Deserialize)]
319+
#[derive(Serialize, Deserialize, ToSchema)]
320+
/// List of locks
304321
pub struct LockList {
305322
pub locks: Vec<Lock>,
306323
pub next_cursor: String,
307324
}
308325

309-
#[derive(Serialize, Deserialize, Debug, Default)]
326+
#[derive(Serialize, Deserialize, Debug, Default, ToSchema)]
327+
/// Request to verify locks
310328
pub struct VerifiableLockRequest {
311329
#[serde(rename(serialize = "ref", deserialize = "ref"))]
312330
pub refs: Ref,
313331
pub cursor: Option<String>,
314332
pub limit: Option<i64>,
315333
}
316334

317-
#[derive(Serialize, Deserialize, Debug)]
335+
#[derive(Serialize, Deserialize, Debug, ToSchema)]
336+
/// List of verifiable locks
318337
pub struct VerifiableLockList {
319338
pub ours: Vec<Lock>,
320339
pub theirs: Vec<Lock>,
321340
pub next_cursor: String,
322341
}
323342

324-
#[derive(Serialize, Deserialize, Debug)]
343+
#[derive(Serialize, Deserialize, Debug, ToSchema)]
344+
/// Query parameters for listing locks
325345
pub struct LockListQuery {
326346
#[serde(default)]
327347
pub path: String,

‎docs/lfs-api-improvements.md‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
# LFS API Improvement Recommendations (GitHub Official Specification Comparison)
2+
3+
## Comparison Baseline
4+
- Git LFS Batch API: https://github.com/git-lfs/git-lfs/blob/main/docs/api/batch.md
5+
- Git LFS Locking API: https://github.com/git-lfs/git-lfs/blob/main/docs/api/locking.md
6+
- Git LFS Server Discovery: https://github.com/git-lfs/git-lfs/blob/main/docs/api/server-discovery.md
7+
8+
## Issues Found and Fix Recommendations
9+
10+
1) Error codes are too uniform, missing 4xx status codes
11+
- Current state: Most errors in `mono/src/api/router/lfs_router.rs` are mapped to 500.
12+
- Specification: Not found objects/locks should return 404; request parameter errors should return 400.
13+
- Completed: Added `map_lfs_error` in router layer, mapping error messages containing `Not found` / `Invalid` to 404/400.
14+
- Follow-up: Use explicit error types instead of strings in `ceres/src/lfs/handler.rs` to avoid string matching.
15+
16+
2) Download endpoint Content-Type
17+
- Current state: Download object/chunk uses `application/vnd.git-lfs+json`.
18+
- Specification: Data streams should return binary stream `application/octet-stream`.
19+
- Completed: Changed router download response to `application/octet-stream`.
20+
21+
3) Batch download missing 404 semantics
22+
- Current state: `lfs_process_batch` returns errors mapped to 500 at router layer when download objects are missing.
23+
- Specification: Not found objects should return `error` at object level, with overall status 200.
24+
- Recommendation: In `handler::lfs_process_batch`, preserve object-level `error` for missing download objects (already implemented), router maintains 200; avoid treating missing as top-level error in handler.
25+
26+
4) Upload pre-validation insufficient
27+
- Current state: `lfs_upload_object` returns generic error if meta doesn't exist (router maps to 404).
28+
- Recommendation: Use explicit "Not found" message or error type in handler to avoid string matching; validate that `size` matches request body length (currently commented out).
29+
30+
5) Chunk download behavior in non-split mode
31+
- Current state: `lfs_download_chunk` validates hash after slicing in non-split mode, returns 500 on mismatch.
32+
- Recommendation: Return 400 for hash mismatch; return 404 for missing chunk.
33+
34+
6) Lock endpoint error semantics
35+
- Current state: Deleting non-existent lock returns 500.
36+
- Recommendation: `lfs_delete_lock` should return 404 for lock not found; return 403 for no permission (currently no owner validation).
37+
38+
7) OpenAPI coverage
39+
- Completed: Added `utoipa::path` to all LFS routes and registered LFS schemas in `ApiDoc`.
40+
- Recommendation: Provide example response (stream) documentation for LFS download endpoints in future versions.
41+
42+
## Priority
43+
- High: Error code semantics (404/400), download Content-Type, lock deletion 404.
44+
- Medium: Batch download missing objects maintain 200 + object-level error, upload size validation.
45+
- Low: Hash mismatch 400, permission/lock owner validation supplement.

0 commit comments

Comments
 (0)