Skip to content

Commit d7db754

Browse files
wp99cpwp99cp
authored
Merge pull request #2076 from leggedrobotics/fix/rename-yaml
fix: Cannot Rename .yaml Files #2066
2 parents 7e4afac + ac77d7d commit d7db754

4 files changed

Lines changed: 221 additions & 24 deletions

File tree

‎backend/.gitignore‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,10 @@ report.[0-9]*.[0-9]*.[0-9]*.[0-9]*.json
6161

6262
/junit.xml
6363

64-
# ignore bag and mcap files
64+
# ignore bag, mcap and other test files
6565
*.bag
6666
*.mcap
67+
tests/fixtures/*
6768

6869
# OpenApi
6970
swagger.json

‎backend/src/services/file.service.ts‎

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,16 @@ const FIND_MANY_SORT_KEYS = {
9999
'file.date': 'file.date',
100100
};
101101

102+
const FILE_EXTENSION_TO_FILE_TYPE_MAP: ReadonlyMap<string, FileType> = new Map([
103+
['.bag', FileType.BAG],
104+
['.mcap', FileType.MCAP],
105+
['.yaml', FileType.YAML],
106+
['.yml', FileType.YAML],
107+
['.svo2', FileType.SVO2],
108+
['.tum', FileType.TUM],
109+
['.db3', FileType.DB3],
110+
]);
111+
102112
@Injectable()
103113
export class FileService implements OnModuleInit {
104114
private fileCleanupQueue!: Queue.Queue;
@@ -931,10 +941,18 @@ export class FileService implements OnModuleInit {
931941
const isRenamed = file.filename !== oldFilename;
932942

933943
// validate file ending
934-
const fileEnding =
935-
databaseFile.type === FileType.MCAP ? '.mcap' : '.bag';
936-
if (!file.filename.endsWith(fileEnding)) {
937-
throw new BadRequestException('File ending must not be changed');
944+
const validExtensions = [...FILE_EXTENSION_TO_FILE_TYPE_MAP.entries()]
945+
.filter(([, type]) => type === databaseFile.type)
946+
.map(([extension]) => extension);
947+
948+
if (
949+
!validExtensions.some((extension) =>
950+
file.filename.endsWith(extension),
951+
)
952+
) {
953+
throw new BadRequestException(
954+
`File ending must be one of: ${validExtensions.join(', ')}`,
955+
);
938956
}
939957

940958
databaseFile.filename = file.filename;
@@ -1340,23 +1358,9 @@ export class FileService implements OnModuleInit {
13401358
error: null,
13411359
};
13421360

1343-
const fileExtensionToFileTypeMap: ReadonlyMap<
1344-
string,
1345-
FileType
1346-
> = new Map([
1347-
['.bag', FileType.BAG],
1348-
1349-
['.mcap', FileType.MCAP],
1350-
['.yaml', FileType.YAML],
1351-
['.yml', FileType.YAML],
1352-
['.svo2', FileType.SVO2],
1353-
['.tum', FileType.TUM],
1354-
['.db3', FileType.DB3],
1355-
]);
1356-
13571361
// eslint-disable-next-line @typescript-eslint/naming-convention
13581362
const supported_file_endings = [
1359-
...fileExtensionToFileTypeMap.keys(),
1363+
...FILE_EXTENSION_TO_FILE_TYPE_MAP.keys(),
13601364
];
13611365

13621366
if (
@@ -1375,7 +1379,7 @@ export class FileService implements OnModuleInit {
13751379
if (matchingFileType === undefined)
13761380
throw new UnsupportedMediaTypeException();
13771381
const fileType: FileType | undefined =
1378-
fileExtensionToFileTypeMap.get(matchingFileType);
1382+
FILE_EXTENSION_TO_FILE_TYPE_MAP.get(matchingFileType);
13791383
if (fileType === undefined)
13801384
throw new UnsupportedMediaTypeException();
13811385

@@ -1434,8 +1438,7 @@ export class FileService implements OnModuleInit {
14341438
// Add to local set to catch duplicates in the same batch
14351439
existingFilenames.add(filename);
14361440
});
1437-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
1438-
} catch (error: any) {
1441+
} catch (error: unknown) {
14391442
if (
14401443
error instanceof QueryFailedError &&
14411444
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access

‎backend/tests/files/file-rename.test.ts‎

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
import { FileEntity } from '@kleinkram/backend-common';
2+
import { UserRole } from '@kleinkram/shared';
3+
import { DEFAULT_URL } from '../auth/utilities';
4+
import { HeaderCreator, uploadFile } from '../utils/api-calls';
5+
import { database } from '../utils/database-utilities';
6+
import {
7+
setupDatabaseHooks,
8+
setupTestEnvironment,
9+
} from '../utils/test-helpers';
10+
11+
describe('File Rename Bug Verification', () => {
12+
setupDatabaseHooks();
13+
14+
test('should succeed to rename a .yaml file', async () => {
15+
const { user, missionUuid } = await setupTestEnvironment(
16+
'test-rename-yaml@kleinkram.dev',
17+
'Rename User',
18+
UserRole.ADMIN,
19+
);
20+
21+
// 1. Upload a .yaml file
22+
await uploadFile(user, 'config.yaml', missionUuid);
23+
24+
// 2. Find it in DB
25+
const fileRepo = database.getRepository(FileEntity);
26+
const file = await fileRepo.findOneOrFail({
27+
where: { filename: 'config.yaml' },
28+
});
29+
30+
const headers = new HeaderCreator(user);
31+
headers.addHeader('Content-Type', 'application/json');
32+
33+
// 3. Attempt to rename it
34+
const renameResponse = await fetch(
35+
`${DEFAULT_URL}/files/${file.uuid}`,
36+
{
37+
method: 'PUT',
38+
headers: headers.getHeaders(),
39+
body: JSON.stringify({
40+
uuid: file.uuid,
41+
date: file.date,
42+
filename: 'new_config.yaml',
43+
}),
44+
},
45+
);
46+
47+
expect(renameResponse.status).toBeLessThan(300);
48+
const updatedFile = await fileRepo.findOneOrFail({
49+
where: { uuid: file.uuid },
50+
});
51+
expect(updatedFile.filename).toBe('new_config.yaml');
52+
}, 30_000);
53+
54+
test('should succeed to rename a .yml file', async () => {
55+
const { user, missionUuid } = await setupTestEnvironment(
56+
'test-rename-yml@kleinkram.dev',
57+
'Rename User',
58+
UserRole.ADMIN,
59+
);
60+
61+
await uploadFile(user, 'config.yml', missionUuid);
62+
63+
const fileRepo = database.getRepository(FileEntity);
64+
const file = await fileRepo.findOneOrFail({
65+
where: { filename: 'config.yml' },
66+
});
67+
68+
const headers = new HeaderCreator(user);
69+
headers.addHeader('Content-Type', 'application/json');
70+
71+
const renameResponse = await fetch(
72+
`${DEFAULT_URL}/files/${file.uuid}`,
73+
{
74+
method: 'PUT',
75+
headers: headers.getHeaders(),
76+
body: JSON.stringify({
77+
uuid: file.uuid,
78+
date: file.date,
79+
filename: 'renamed_config.yml',
80+
}),
81+
},
82+
);
83+
84+
expect(renameResponse.status).toBeLessThan(300);
85+
const updatedFile = await fileRepo.findOneOrFail({
86+
where: { uuid: file.uuid },
87+
});
88+
expect(updatedFile.filename).toBe('renamed_config.yml');
89+
}, 30_000);
90+
91+
test('should fail if changing extension (e.g. .bag to .mcap)', async () => {
92+
const { user, missionUuid } = await setupTestEnvironment(
93+
'test-rename-invalid@kleinkram.dev',
94+
'Rename User',
95+
UserRole.ADMIN,
96+
);
97+
98+
await uploadFile(user, 'test.bag', missionUuid);
99+
100+
const fileRepo = database.getRepository(FileEntity);
101+
const file = await fileRepo.findOneOrFail({
102+
where: { filename: 'test.bag' },
103+
});
104+
105+
const headers = new HeaderCreator(user);
106+
headers.addHeader('Content-Type', 'application/json');
107+
108+
const renameResponse = await fetch(
109+
`${DEFAULT_URL}/files/${file.uuid}`,
110+
{
111+
method: 'PUT',
112+
headers: headers.getHeaders(),
113+
body: JSON.stringify({
114+
uuid: file.uuid,
115+
date: file.date,
116+
filename: 'test.mcap',
117+
}),
118+
},
119+
);
120+
121+
expect(renameResponse.status).toBe(400);
122+
const error = (await renameResponse.json()) as { message: string };
123+
expect(error.message).toContain('File ending must be one of');
124+
}, 30_000);
125+
126+
test('should allow .yaml <-> .yml rename but fail for others', async () => {
127+
const { user, missionUuid } = await setupTestEnvironment(
128+
'test-yaml-yml-swap@kleinkram.dev',
129+
'Rename User',
130+
UserRole.ADMIN,
131+
);
132+
133+
// 1. Upload .yaml
134+
await uploadFile(user, 'config.yaml', missionUuid);
135+
const fileRepo = database.getRepository(FileEntity);
136+
let file = await fileRepo.findOneOrFail({
137+
where: { filename: 'config.yaml' },
138+
});
139+
140+
const headers = new HeaderCreator(user);
141+
headers.addHeader('Content-Type', 'application/json');
142+
143+
// 2. Rename .yaml -> .yml (Should Succeed)
144+
let renameResponse = await fetch(`${DEFAULT_URL}/files/${file.uuid}`, {
145+
method: 'PUT',
146+
headers: headers.getHeaders(),
147+
body: JSON.stringify({
148+
uuid: file.uuid,
149+
date: file.date,
150+
filename: 'config.yml',
151+
}),
152+
});
153+
expect(renameResponse.status).toBeLessThan(300);
154+
file = await fileRepo.findOneOrFail({ where: { uuid: file.uuid } });
155+
expect(file.filename).toBe('config.yml');
156+
157+
// 3. Rename .yml -> .yaml (Should Succeed)
158+
renameResponse = await fetch(`${DEFAULT_URL}/files/${file.uuid}`, {
159+
method: 'PUT',
160+
headers: headers.getHeaders(),
161+
body: JSON.stringify({
162+
uuid: file.uuid,
163+
date: file.date,
164+
filename: 'config.yaml',
165+
}),
166+
});
167+
expect(renameResponse.status).toBeLessThan(300);
168+
file = await fileRepo.findOneOrFail({ where: { uuid: file.uuid } });
169+
expect(file.filename).toBe('config.yaml');
170+
171+
// 4. Rename .yaml -> .bag (Should Fail)
172+
renameResponse = await fetch(`${DEFAULT_URL}/files/${file.uuid}`, {
173+
method: 'PUT',
174+
headers: headers.getHeaders(),
175+
body: JSON.stringify({
176+
uuid: file.uuid,
177+
date: file.date,
178+
filename: 'config.bag',
179+
}),
180+
});
181+
expect(renameResponse.status).toBe(400);
182+
const error = (await renameResponse.json()) as { message: string };
183+
expect(error.message).toContain('File ending must be one of');
184+
}, 30_000);
185+
});

‎cli/tests/generate_test_data.py‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -296,8 +296,16 @@ def main():
296296
generate_bag(os.path.join(backend_fixtures_dir, "file1.bag"), 10 * 1024)
297297
generate_bag(os.path.join(backend_fixtures_dir, "file2.bag"), 10 * 1024)
298298
generate_bag(os.path.join(backend_fixtures_dir, "move_me.bag"), 10 * 1024)
299-
generate_bag(os.path.join(backend_fixtures_dir, "move_me.bag"), 10 * 1024)
300299
generate_bag(os.path.join(backend_fixtures_dir, "state_test.bag"), 10 * 1024)
300+
301+
# Generate backend dummy MCAP and YAML
302+
with open(os.path.join(backend_fixtures_dir, "config.yaml"), "w") as f:
303+
f.write("test: true\nvalue: 123\n")
304+
with open(os.path.join(backend_fixtures_dir, "config.yml"), "w") as f:
305+
f.write("test: true\nvalue: 123\n")
306+
with open(os.path.join(backend_fixtures_dir, "test.mcap"), "wb") as f:
307+
f.write(b"\x89MCAP\x30\r\n")
308+
301309
generate_frontend_bag(os.path.join(data_dir, "frontend_test.bag"))
302310

303311
# Generate dummy MCAP and YAML

0 commit comments

Comments
 (0)