Skip to content

Commit 062e6e8

Browse files
solenuksolenuk
committed
fix: coderabbit issues
1 parent 75660c5 commit 062e6e8

9 files changed

Lines changed: 154 additions & 38 deletions

File tree

‎src/main/java/com/itasocialacademy/oitassist/filemanager/api/FileManagerFacade.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,4 +78,22 @@ List<FileDetailsDTO> uploadFiles(
7878
* @param newRole the new role to assign
7979
*/
8080
void updateFileRole(Long fileId, FileRole newRole);
81+
82+
/**
83+
* Updates the role of a file that must belong to the specified entity.
84+
* Validates that the file is ATTACHED to the given entity type and ID before
85+
* updating.
86+
*
87+
* <p>
88+
* <strong>Does NOT perform per-file ownership checks.</strong> The calling
89+
* module must verify that the current user is authorized to modify the entity
90+
* (e.g. task ownership check) before invoking this method.
91+
* </p>
92+
*
93+
* @param fileId the ID of the file to update
94+
* @param newRole the new role to assign
95+
* @param entityType the expected related entity type
96+
* @param entityId the expected related entity ID
97+
*/
98+
void updateRoleForMultiOwnerEntity(Long fileId, FileRole newRole, RelatedEntityType entityType, Long entityId);
8199
}

‎src/main/java/com/itasocialacademy/oitassist/filemanager/controller/FileController.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
package com.itasocialacademy.oitassist.filemanager.controller;
22

3-
import com.azure.core.annotation.QueryParam;
43
import com.itasocialacademy.oitassist.core.web.ErrorResponse;
54
import com.itasocialacademy.oitassist.filemanager.dao.enums.RelatedEntityType;
65
import com.itasocialacademy.oitassist.filemanager.dto.request.FileUploadRequestDto;
@@ -241,7 +240,7 @@ public ResponseEntity<List<FileResponseDto>> getFiles(
241240
@PreAuthorize("isAuthenticated()")
242241
public ResponseEntity<FileResponseDto> updateRole(
243242
@PathVariable Long id,
244-
@Valid @QueryParam("newRole") UpdateFileRoleRequestDto requestDto) {
245-
return ResponseEntity.ok(fileService.updateRole(id, requestDto));
243+
@Valid @org.springframework.web.bind.annotation.RequestBody UpdateFileRoleRequestDto requestDto) {
244+
return ResponseEntity.ok(fileService.updateRoleGeneral(id, requestDto));
246245
}
247246
}

‎src/main/java/com/itasocialacademy/oitassist/filemanager/service/FileManagerFacadeImpl.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,12 @@ public void detachFiles(RelatedEntityType entityType, Long entityId, List<Long>
5252

5353
@Override
5454
public void updateFileRole(Long fileId, FileRole newRole) {
55-
fileService.updateRole(fileId, new UpdateFileRoleRequestDto(newRole));
55+
fileService.updateRoleGeneral(fileId, new UpdateFileRoleRequestDto(newRole));
56+
}
57+
58+
@Override
59+
public void updateRoleForMultiOwnerEntity(Long fileId, FileRole newRole, RelatedEntityType entityType,
60+
Long entityId) {
61+
fileService.updateRoleForMultiOwnerEntity(fileId, newRole, entityType, entityId);
5662
}
5763
}

‎src/main/java/com/itasocialacademy/oitassist/filemanager/service/FileServiceImpl.java‎

Lines changed: 57 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -331,51 +331,87 @@ public Map<Long, List<FileDetailsDTO>> getFilesByEntities(RelatedEntityType enti
331331

332332
@Override
333333
@Transactional
334-
public FileResponseDto updateRole(Long fileId, UpdateFileRoleRequestDto request) {
334+
public FileResponseDto updateRoleGeneral(Long fileId, UpdateFileRoleRequestDto request) {
335335
Long currentUserId = securityFacade.getCurrentUserId()
336-
.orElseThrow(() -> new AuthorizationException(NOT_AUTHENTICATED, ErrorCode.ACCESS_DENIED));
336+
.orElseThrow(() -> new AuthorizationException(
337+
NOT_AUTHENTICATED, ErrorCode.ACCESS_DENIED));
337338

338339
FileAsset file = repository.findById(fileId)
339-
.orElseThrow(() -> new FileAssetNotFoundException(FILE_NOT_FOUND_IN_THE_DATABASE + fileId));
340+
.orElseThrow(() -> new FileAssetNotFoundException(
341+
FILE_NOT_FOUND_IN_THE_DATABASE + fileId));
340342

341343
checkOwnerOrAdmin(file.getUserId(), currentUserId);
342344

345+
FileAsset saved = validateAndUpdateRole(file, request.getNewRole(),
346+
file.getRelatedEntityType(), file.getRelatedEntityId());
347+
348+
StorageProvider provider = providerResolver.resolve(saved.getStorageProvider());
349+
FileResponseDto dto = fileMapper.toDto(saved);
350+
dto.setUrl(provider.getFileUrl(saved.getStorageKey()));
351+
return dto;
352+
}
353+
354+
@Override
355+
@Transactional
356+
public void updateRoleForMultiOwnerEntity(Long fileId, FileRole newRole, RelatedEntityType entityType,
357+
Long entityId) {
358+
FileAsset file = repository.findById(fileId)
359+
.orElseThrow(() -> new FileAssetNotFoundException(
360+
FILE_NOT_FOUND_IN_THE_DATABASE + fileId));
361+
362+
validateAndUpdateRole(file, newRole, entityType, entityId);
363+
}
364+
365+
/**
366+
* Validates and updates the role of a given file asset. Ensures the file is in
367+
* an ATTACHED state, belongs to the specified entity, and that the new role is
368+
* allowed for its extension based on the entity's policy.
369+
*
370+
* @param file the file asset to update
371+
* @param newRole the new role to assign
372+
* @param entityType the expected related entity type
373+
* @param entityId the expected related entity ID
374+
* @return the saved file asset (or the original if the role is already set)
375+
* @throws ValidationException if the file is not ATTACHED, does not belong to
376+
* the entity, or the extension is disallowed
377+
*/
378+
private FileAsset validateAndUpdateRole(FileAsset file, FileRole newRole, RelatedEntityType entityType,
379+
Long entityId) {
343380
if (!file.getStatus().equals(FileStatus.ATTACHED)) {
344381
throw new ValidationException(
345382
"File must be in ATTACHED state to update its role",
346383
ErrorCode.FILE_VALIDATION_FAILED);
347384
}
348385

349-
StorageProvider provider = providerResolver.resolve(file.getStorageProvider());
386+
if (!entityType.equals(file.getRelatedEntityType())
387+
|| !entityId.equals(file.getRelatedEntityId())) {
388+
throw new ValidationException(
389+
"File id=" + file.getId() + " does not belong to "
390+
+ entityType + " id=" + entityId,
391+
ErrorCode.FILE_VALIDATION_FAILED);
392+
}
350393

351-
if (file.getFileRole().equals(request.getNewRole())) {
352-
FileAsset saved = repository.save(file);
353-
FileResponseDto dto = fileMapper.toDto(saved);
354-
dto.setUrl(provider.getFileUrl(saved.getStorageKey()));
355-
return dto;
394+
if (file.getFileRole().equals(newRole)) {
395+
return file;
356396
}
357397

358-
// Validate if new role is allowed for the file extension
359-
FilePolicy newPolicy = filePolicyResolver.resolve(file.getRelatedEntityType(), request.getNewRole());
398+
FilePolicy newPolicy = filePolicyResolver.resolve(entityType, newRole);
360399
String extension = extractExtension(file.getOriginalFilename());
361400

362401
if (isExtensionNotAllowed(extension, newPolicy.getAllowedExtensions())) {
363402
throw new ValidationException(
364-
"File '%s' has extension '%s' which is not allowed for role %s. Allowed: %s.".formatted(
365-
file.getOriginalFilename(), extension, request.getNewRole(),
366-
formatAllowed(newPolicy.getAllowedExtensions())),
403+
"File '%s' has extension '%s' which is not allowed for role %s. Allowed: %s."
404+
.formatted(file.getOriginalFilename(), extension, newRole,
405+
formatAllowed(newPolicy.getAllowedExtensions())),
367406
ErrorCode.FILE_VALIDATION_FAILED);
368407
}
369408

370-
file.setFileRole(request.getNewRole());
409+
file.setFileRole(newRole);
371410
FileAsset saved = repository.save(file);
411+
log.debug("Updated file role for id={} to {} (entity {}:{})",
412+
file.getId(), newRole, entityType, entityId);
372413

373-
log.debug("Updated file role for id={} to {}", saved.getId(), saved.getFileRole());
374-
375-
FileResponseDto dto = fileMapper.toDto(saved);
376-
dto.setUrl(provider.getFileUrl(saved.getStorageKey()));
377-
378-
return dto;
414+
return saved;
379415
}
380416

381417
/**

‎src/main/java/com/itasocialacademy/oitassist/filemanager/service/interfaces/FileService.java‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,5 +127,16 @@ Map<Long, List<FileDetailsDTO>> getFilesByEntities(RelatedEntityType entityType,
127127
* @param requestDto the DTO containing the new role
128128
* @return the updated file response DTO
129129
*/
130-
FileResponseDto updateRole(Long fileId, UpdateFileRoleRequestDto requestDto);
130+
FileResponseDto updateRoleGeneral(Long fileId, UpdateFileRoleRequestDto requestDto);
131+
132+
/**
133+
* Updates the role of a file that must be ATTACHED to the specified entity.
134+
* Validates entity boundary. Does NOT check per-file ownership.
135+
*
136+
* @param fileId the ID of the file to update
137+
* @param newRole the new role to assign
138+
* @param entityType the expected related entity type
139+
* @param entityId the expected related entity ID
140+
*/
141+
void updateRoleForMultiOwnerEntity(Long fileId, FileRole newRole, RelatedEntityType entityType, Long entityId);
131142
}

‎src/main/java/com/itasocialacademy/oitassist/task/service/TaskServiceImpl.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,8 @@ public TaskResponseDTO updateTask(
159159
}
160160

161161
if (requestDTO.roleUpdates() != null) {
162-
requestDTO.roleUpdates().forEach(fileManagerFacade::updateFileRole);
162+
requestDTO.roleUpdates().forEach((fileId, newRole) -> fileManagerFacade.updateRoleForMultiOwnerEntity(
163+
fileId, newRole, RelatedEntityType.TASK, updatedTask.getId()));
163164
}
164165

165166
uploadFilesByRole(updatedTask.getId(), problemFiles, FileRole.PROBLEM);

‎src/test/java/com/itasocialacademy/oitassist/filemanager/controller/FileControllerTest.java‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
import com.itasocialacademy.oitassist.filemanager.dao.enums.FileRole;
1414
import com.itasocialacademy.oitassist.filemanager.dao.enums.RelatedEntityType;
1515
import com.itasocialacademy.oitassist.filemanager.dto.request.FileUploadRequestDto;
16-
import com.itasocialacademy.oitassist.filemanager.dto.request.UpdateFileRoleRequestDto;
1716
import com.itasocialacademy.oitassist.filemanager.dto.response.FileResponseDto;
1817
import com.itasocialacademy.oitassist.filemanager.exceptions.FileAssetNotFoundException;
1918
import com.itasocialacademy.oitassist.filemanager.exceptions.FileUploadException;
@@ -239,13 +238,14 @@ void updateRole_ShouldReturn200_WhenValidRequest() throws Exception {
239238
Long fileId = 1L;
240239
FileResponseDto responseDto = FileResponseDto.builder().id(fileId).build();
241240

242-
when(fileService.updateRole(
241+
when(fileService.updateRoleGeneral(
243242
eq(fileId),
244243
argThat(request -> request.getNewRole() == FileRole.PROBLEM)))
245244
.thenReturn(responseDto);
246245

247246
mockMvc.perform(patch("/api/v1/files/{id}/role", fileId)
248-
.param("newRole", "PROBLEM"))
247+
.contentType(MediaType.APPLICATION_JSON)
248+
.content("{\"newRole\":\"PROBLEM\"}"))
249249
.andExpect(status().isOk())
250250
.andExpect(jsonPath("$.id").value(fileId));
251251
}
@@ -254,7 +254,9 @@ void updateRole_ShouldReturn200_WhenValidRequest() throws Exception {
254254
void updateRole_ShouldReturn400_WhenRoleIsNull() throws Exception {
255255
Long fileId = 1L;
256256

257-
mockMvc.perform(patch("/api/v1/files/{id}/role", fileId))
257+
mockMvc.perform(patch("/api/v1/files/{id}/role", fileId)
258+
.contentType(MediaType.APPLICATION_JSON)
259+
.content("{}"))
258260
.andExpect(status().isBadRequest());
259261

260262
verifyNoInteractions(fileService);

‎src/test/java/com/itasocialacademy/oitassist/filemanager/service/FileServiceImplTest.java‎

Lines changed: 49 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1381,14 +1381,15 @@ void detachAllFilesByEntityId_ShouldThrowAuthorizationException_WhenUserIsNotOwn
13811381
// --- Update Role Tests ---
13821382

13831383
@Test
1384-
void updateRole_ShouldUpdateRoleAndReturnDto_WhenValidRequest() {
1384+
void updateRole_ShouldUpdateRoleGeneralAndReturnDto_WhenValidRequest() {
13851385
UpdateFileRoleRequestDto requestDto = new UpdateFileRoleRequestDto(FileRole.PROBLEM);
13861386

13871387
FileAsset file = new FileAsset();
13881388
file.setId(fileId);
13891389
file.setUserId(userId);
13901390
file.setStatus(FileStatus.ATTACHED);
13911391
file.setRelatedEntityType(RelatedEntityType.TASK);
1392+
file.setRelatedEntityId(1L);
13921393
file.setFileRole(FileRole.REFERENCE);
13931394
file.setStorageProvider(StorageProviderType.LOCAL);
13941395
file.setStorageKey("task/problem.docx");
@@ -1408,7 +1409,7 @@ void updateRole_ShouldUpdateRoleAndReturnDto_WhenValidRequest() {
14081409
when(providerResolver.resolve(StorageProviderType.LOCAL)).thenReturn(storageProvider);
14091410
when(storageProvider.getFileUrl("task/problem.docx")).thenReturn("/uploads/task/problem.docx");
14101411

1411-
FileResponseDto result = fileService.updateRole(fileId, requestDto);
1412+
FileResponseDto result = fileService.updateRoleGeneral(fileId, requestDto);
14121413

14131414
assertNotNull(result);
14141415
assertEquals(expectedDto, result);
@@ -1420,7 +1421,7 @@ void updateRole_ShouldUpdateRoleAndReturnDto_WhenValidRequest() {
14201421
}
14211422

14221423
@Test
1423-
void updateRole_ShouldThrowValidationException_WhenFileNotAttached() {
1424+
void updateRole_General_ShouldThrowValidationException_WhenFileNotAttached() {
14241425
UpdateFileRoleRequestDto requestDto = new UpdateFileRoleRequestDto(FileRole.PROBLEM);
14251426

14261427
FileAsset file = new FileAsset();
@@ -1433,15 +1434,15 @@ void updateRole_ShouldThrowValidationException_WhenFileNotAttached() {
14331434
when(securityFacade.hasRole("ADMIN")).thenReturn(false);
14341435

14351436
ValidationException exception =
1436-
assertThrows(ValidationException.class, () -> fileService.updateRole(fileId, requestDto));
1437+
assertThrows(ValidationException.class, () -> fileService.updateRoleGeneral(fileId, requestDto));
14371438
assertTrue(exception.getMessage().contains("ATTACHED state"));
14381439

14391440
verifyNoInteractions(filePolicyResolver, fileMapper);
14401441
verify(fileRepository, never()).save(any());
14411442
}
14421443

14431444
@Test
1444-
void updateRole_ShouldThrowAuthorizationException_WhenUserNotOwnerOrAdmin() {
1445+
void updateRole_General_ShouldThrowAuthorizationException_WhenUserNotOwnerOrAdmin() {
14451446
Long ownerId = 99L;
14461447
UpdateFileRoleRequestDto requestDto = new UpdateFileRoleRequestDto(FileRole.PROBLEM);
14471448

@@ -1453,12 +1454,54 @@ void updateRole_ShouldThrowAuthorizationException_WhenUserNotOwnerOrAdmin() {
14531454
when(fileRepository.findById(fileId)).thenReturn(Optional.of(file));
14541455
when(securityFacade.hasRole("ADMIN")).thenReturn(false);
14551456

1456-
assertThrows(AuthorizationException.class, () -> fileService.updateRole(fileId, requestDto));
1457+
assertThrows(AuthorizationException.class, () -> fileService.updateRoleGeneral(fileId, requestDto));
14571458

14581459
verifyNoInteractions(filePolicyResolver, fileMapper);
14591460
verify(fileRepository, never()).save(any());
14601461
}
14611462

1463+
@Test
1464+
void updateRoleForMultiOwnerEntity_ShouldUpdateRole_WhenValidRequest() {
1465+
FileAsset file = new FileAsset();
1466+
file.setId(fileId);
1467+
file.setStatus(FileStatus.ATTACHED);
1468+
file.setRelatedEntityType(RelatedEntityType.TASK);
1469+
file.setRelatedEntityId(2L);
1470+
file.setFileRole(FileRole.PROBLEM);
1471+
file.setStorageProvider(StorageProviderType.LOCAL);
1472+
file.setOriginalFilename("test.docx");
1473+
1474+
FilePolicy newRolePolicy = mock(FilePolicy.class);
1475+
when(newRolePolicy.getAllowedExtensions()).thenReturn(Set.of(AllowedExtension.DOCX));
1476+
1477+
when(fileRepository.findById(fileId)).thenReturn(Optional.of(file));
1478+
when(filePolicyResolver.resolve(RelatedEntityType.TASK, FileRole.SOLUTION)).thenReturn(newRolePolicy);
1479+
when(fileRepository.save(any(FileAsset.class))).thenAnswer(invocation -> invocation.getArgument(0));
1480+
1481+
fileService.updateRoleForMultiOwnerEntity(fileId, FileRole.SOLUTION, RelatedEntityType.TASK, 2L);
1482+
1483+
assertEquals(FileRole.SOLUTION, file.getFileRole());
1484+
verify(fileRepository).save(file);
1485+
}
1486+
1487+
@Test
1488+
void updateRoleForMultiOwnerEntity_ShouldThrowValidationException_WhenWrongEntityBoundary() {
1489+
FileAsset file = new FileAsset();
1490+
file.setId(fileId);
1491+
file.setStatus(FileStatus.ATTACHED);
1492+
file.setRelatedEntityType(RelatedEntityType.TASK);
1493+
file.setRelatedEntityId(99L); // Belongs to task 99
1494+
1495+
when(fileRepository.findById(fileId)).thenReturn(Optional.of(file));
1496+
1497+
// Attempting to update it in the context of task 42 -> should throw
1498+
ValidationException exception = assertThrows(ValidationException.class,
1499+
() -> fileService.updateRoleForMultiOwnerEntity(fileId, FileRole.SOLUTION, RelatedEntityType.TASK, 42L));
1500+
1501+
assertTrue(exception.getMessage().contains("does not belong to"));
1502+
verify(fileRepository, never()).save(any());
1503+
}
1504+
14621505
// --- Helpers ---
14631506

14641507
private static FileUploadRequestDto uploadRequest(RelatedEntityType type, Long relatedEntityId) {

‎src/test/java/com/itasocialacademy/oitassist/task/service/TaskServiceTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,7 @@ void updateTask_asOwner_shouldDetachAndUploadFiles() {
364364
// Verify detach
365365
verify(fileManagerFacade).detachFiles(RelatedEntityType.TASK, 1L, List.of(51L), 100L);
366366
// Verify role update
367-
verify(fileManagerFacade).updateFileRole(52L, FileRole.SOLUTION);
367+
verify(fileManagerFacade).updateRoleForMultiOwnerEntity(52L, FileRole.SOLUTION, RelatedEntityType.TASK, 1L);
368368
// Verify upload
369369
verify(fileManagerFacade).uploadFiles(newProblemFiles, RelatedEntityType.TASK, 1L, FileRole.PROBLEM);
370370
}

0 commit comments

Comments
 (0)