PO-3948 Implement get interface file content api#73
PO-3948 Implement get interface file content api#73thomas-croasdale-cgi wants to merge 16 commits into
Conversation
|
The suppression for the failing dependency is expired, but this has been updated in master, so rebase to fix |
| @Configuration | ||
| public class CapsReportBaisFileProcessorConfig implements BaisFileProcessorConfig { | ||
|
|
||
| @Value("${opal.file-handling-service.file-types.CAPS-report.storage-container-name}") |
There was a problem hiding this comment.
I think @ConfigurationProperties("opal.file-handling-service.file-types.caps-report") as a top-level annotation would be preferred to @Value, especially when the configurations will have many fields
# Conflicts: # build.gradle # config/owasp/suppressions.xml
hmcts-jenkins-cnp
Bot
added
ns:opal
prd:opal
rel:opal-file-handler-service-pr-73
labels
| import org.springframework.stereotype.Component; | ||
|
|
||
| @ConfigurationProperties(prefix = "opal.file-handling-service.file-types.bteckoh-report") | ||
| @Component("BTECKOH_REPORT") |
There was a problem hiding this comment.
Can you add config to the name please
| import org.springframework.stereotype.Component; | ||
|
|
||
| @ConfigurationProperties(prefix = "opal.file-handling-service.file-types.caps-report") | ||
| @Component("CAPS_REPORT") |
There was a problem hiding this comment.
Can you add config to the name please
| @Autowired | ||
| private Map<String, BaisFileProcessorConfig> configs; |
There was a problem hiding this comment.
How is the mapping done here?
| id, entity.getStatus())); | ||
| } | ||
|
|
||
| BaisFileProcessorConfig config = configs.get(entity.getSource().toString()); |
There was a problem hiding this comment.
Can this be its own method in which we provide a source enum and we get the config?
As this will be re-used later
|
|
||
| public class PermissionUtil { | ||
|
|
||
| public static void checkPermission(PermissionDescriptor permission) { |
There was a problem hiding this comment.
Missing unit tests
| import org.springframework.web.server.ResponseStatusException; | ||
|
|
||
| @Getter | ||
| public class BlobNotFoundException extends ResponseStatusException { |
There was a problem hiding this comment.
Missing unit tests
| import org.springframework.web.server.ResponseStatusException; | ||
|
|
||
| @Getter | ||
| public class BlobStorageContainerNotFoundException extends ResponseStatusException { |
There was a problem hiding this comment.
Missing unit tests
| import org.springframework.web.server.ResponseStatusException; | ||
|
|
||
| @Getter | ||
| public class InvalidInterfaceFileStatusException extends ResponseStatusException { |
There was a problem hiding this comment.
Missing unit tests
| import org.springframework.context.annotation.Configuration; | ||
|
|
||
| @Configuration | ||
| public class FileHandlerAzureStorageConfig { |
There was a problem hiding this comment.
Missing unit tests
|
|
||
| private final BlobServiceClient blobServiceClient; | ||
|
|
||
| private BlobContainerClient getBlobContainerClient(String containerName) { |
There was a problem hiding this comment.
Should be tested in isolation (so remove the private from the method to make it package-private or public (as we may re-use this method) to support isolated testing)
| return container; | ||
| } | ||
|
|
||
| private BlobClient getBlobClient(BlobContainerClient blobContainerClient, String file) { |
| return blob; | ||
| } | ||
|
|
||
| private BinaryData getFileContents(BlobClient blob) { |
| assertThrows( | ||
| PermissionNotAllowedException.class, | ||
| () -> interfaceFileService.getInterfaceFilesContent(1L) | ||
| ); |
There was a problem hiding this comment.
Needs to validate message is descriptive
| Optional.ofNullable(null) | ||
| ); | ||
|
|
||
| assertThrows(EntityNotFoundException.class, () -> interfaceFileService.getInterfaceFilesContent(1L)); |
There was a problem hiding this comment.
Needs to validate message is descriptive
| Optional.of(buildEntity(1L, uuid, Interface.BTECKOH_REPORT, Status.FAILED)) | ||
| ); | ||
|
|
||
| assertThrows( |
There was a problem hiding this comment.
Needs to validate message is descriptive
| when(blobStoreService.fetchInterfaceFile(eq(1L), eq(uuid), eq("bteckoh-report"))) | ||
| .thenThrow(BlobNotFoundException.class); | ||
|
|
||
| assertThrows(BlobNotFoundException.class, () -> interfaceFileService.getInterfaceFilesContent(1L)); |
There was a problem hiding this comment.
Needs to validate message is descriptive
| private static final int AZURITE_BLOB_PORT = 10000; | ||
| private static final String AZURITE_ACCOUNT_NAME = "devstoreaccount1"; | ||
| private static final String AZURITE_ACCOUNT_KEY = | ||
| "Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw=="; |
There was a problem hiding this comment.
Where has this account key come from?
Is this exclusively for local dev using our docker images?
Or has this come from our secret store?
If exclusively local can a comment be added to indicate indicate this so it does not get picked up in security reviews
| .buildClient(); | ||
| } | ||
|
|
||
| public String storeReport(String report, String containerName, String uuid) { |
There was a problem hiding this comment.
Can the names be made generic as we are not storing reports here rather we are string documents?
There was a problem hiding this comment.
Assuming this is a test file with no sensitive data?
| @@ -0,0 +1 @@ | |||
| DELETE FROM interface_files WHERE interface_file_id IN (1, 2, 3, 4); No newline at end of file | |||
There was a problem hiding this comment.
We can likely delete all interface files in this SQL instead of just select ons to clean them up
| ); | ||
|
|
||
| res.andExpect(status().isOk()) | ||
| .andExpect(content().contentType(MediaType.APPLICATION_OCTET_STREAM)); |
There was a problem hiding this comment.
Need to validate the returned content is what we are expecting
3 participants
### JIRA link ###
Implements the get interface file content api.
Does this PR introduce a breaking change? (check one with "x")