From eb30c164cdbb1b2873f2bae4d48476cff4a2a8f0 Mon Sep 17 00:00:00 2001 From: Avgustin Marinov Date: Fri, 31 Jan 2025 12:20:17 +0200 Subject: [PATCH] [#1249] Don't create action statuses to log range downloads In case of range request: * there could be too many action checks * there could be too many action statuses - filling the db and hitting the quote So, this PR skip action existing check and skip logging (following the approach from #1331 PR by @zyga) Signed-off-by: Avgustin Marinov --- .../ddi/rest/resource/DdiRootController.java | 38 +++++++++---------- .../resource/DdiArtifactDownloadTest.java | 30 +++++++-------- .../rest/resource/DdiDeploymentBaseTest.java | 2 +- .../rest/resource/DdiInstalledBaseTest.java | 30 +++++++-------- .../hawkbit/rest/util/FileStreamingUtil.java | 3 +- 5 files changed, 49 insertions(+), 54 deletions(-) diff --git a/hawkbit-ddi/hawkbit-ddi-resource/src/main/java/org/eclipse/hawkbit/ddi/rest/resource/DdiRootController.java b/hawkbit-ddi/hawkbit-ddi-resource/src/main/java/org/eclipse/hawkbit/ddi/rest/resource/DdiRootController.java index 1699501bb6..3bb73fc5b6 100644 --- a/hawkbit-ddi/hawkbit-ddi-resource/src/main/java/org/eclipse/hawkbit/ddi/rest/resource/DdiRootController.java +++ b/hawkbit-ddi/hawkbit-ddi-resource/src/main/java/org/eclipse/hawkbit/ddi/rest/resource/DdiRootController.java @@ -203,15 +203,21 @@ public ResponseEntity downloadArtifact( if (ifMatch != null && !HttpUtil.matchesHttpHeader(ifMatch, artifact.getSha1Hash())) { result = new ResponseEntity<>(HttpStatus.PRECONDITION_FAILED); } else { - final ActionStatus action = checkAndLogDownload(RequestResponseContextHolder.getHttpServletRequest(), target, module.getId()); - final Long statusId = action.getId(); result = FileStreamingUtil.writeFileResponse(file, artifact.getFilename(), artifact.getCreatedAt(), RequestResponseContextHolder.getHttpServletResponse(), RequestResponseContextHolder.getHttpServletRequest(), - (length, shippedSinceLastEvent, - total) -> eventPublisher.publishEvent(new DownloadProgressEvent( - tenantAware.getCurrentTenant(), statusId, shippedSinceLastEvent, - serviceMatcher != null ? serviceMatcher.getBusId() : bus.getId()))); + (length, shippedSinceLastEvent, total) -> { + if (RequestResponseContextHolder.getHttpServletRequest().getHeader("Range") != null) { + // range request - could have too many - so doesn't log action status or push events + return; + } + + final ActionStatus actionStatus = logDownload( + RequestResponseContextHolder.getHttpServletRequest(), target, module.getId()); + eventPublisher.publishEvent(new DownloadProgressEvent( + tenantAware.getCurrentTenant(), actionStatus.getId(), shippedSinceLastEvent, + serviceMatcher != null ? serviceMatcher.getBusId() : bus.getId())); + }); } } return result; @@ -238,10 +244,9 @@ public ResponseEntity downloadArtifactMd5( final Artifact artifact = module.getArtifactByFilename(fileName) .orElseThrow(() -> new EntityNotFoundException(Artifact.class, fileName)); - checkAndLogDownload(RequestResponseContextHolder.getHttpServletRequest(), target, module.getId()); - try { writeMD5FileResponse(RequestResponseContextHolder.getHttpServletResponse(), artifact.getMd5Hash(), fileName); + logDownload(RequestResponseContextHolder.getHttpServletRequest(), target, module.getId()); } catch (final IOException e) { log.error("Failed to stream MD5 File", e); return new ResponseEntity<>(HttpStatus.INTERNAL_SERVER_ERROR); @@ -609,22 +614,15 @@ private static void writeMD5FileResponse(final HttpServletResponse response, fin response.getOutputStream().write(content); } - private ActionStatus checkAndLogDownload(final HttpServletRequest request, final Target target, final Long module) { + private ActionStatus logDownload(final HttpServletRequest request, final Target target, final Long module) { final Action action = controllerManagement .getActionForDownloadByTargetAndSoftwareModule(target.getControllerId(), module) .orElseThrow(() -> new SoftwareModuleNotAssignedToTargetException(module, target.getControllerId())); - final String range = request.getHeader("Range"); - - final String message; - if (range != null) { - message = RepositoryConstants.SERVER_MESSAGE_PREFIX + - "Target downloads range " + range + " of: " + request.getRequestURI(); - } else { - message = RepositoryConstants.SERVER_MESSAGE_PREFIX + "Target downloads " + request.getRequestURI(); - } - return controllerManagement.addInformationalActionStatus( - entityFactory.actionStatus().create(action.getId()).status(Status.DOWNLOAD).message(message)); + entityFactory.actionStatus() + .create(action.getId()) + .status(Status.DOWNLOAD) + .message(RepositoryConstants.SERVER_MESSAGE_PREFIX + "Target downloads " + request.getRequestURI())); } private ActionStatusCreate generateUpdateStatus(final DdiActionFeedback feedback, final String controllerId, final Long actionId) { diff --git a/hawkbit-ddi/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiArtifactDownloadTest.java b/hawkbit-ddi/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiArtifactDownloadTest.java index 33c261d63e..4d8b4f9d58 100644 --- a/hawkbit-ddi/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiArtifactDownloadTest.java +++ b/hawkbit-ddi/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiArtifactDownloadTest.java @@ -57,21 +57,21 @@ @Feature("Component Tests - Direct Device Integration API") @Story("Artifact Download Resource") @SpringBootTest(classes = { DownloadTestConfiguration.class }) -public class DdiArtifactDownloadTest extends AbstractDDiApiIntegrationTest { +class DdiArtifactDownloadTest extends AbstractDDiApiIntegrationTest { - private static volatile int downLoadProgress = 0; + private static volatile int downloadProgress = 0; private static volatile long shippedBytes = 0; private final SimpleDateFormat dateFormat = new SimpleDateFormat("EEE, dd MMM yyyy HH:mm:ss zzz", Locale.ENGLISH); @BeforeEach - public void setup() { + void setup() { dateFormat.setTimeZone(TimeZone.getTimeZone("GMT")); } @Test @Description("Tests non allowed requests on the artifact ressource, e.g. invalid URI, wrong if-match, wrong command.") - public void invalidRequestsOnArtifactResource() throws Exception { + void invalidRequestsOnArtifactResource() throws Exception { // create target final Target target = testdataFactory.createTarget(); final List targets = Collections.singletonList(target); @@ -160,8 +160,8 @@ public void invalidRequestsOnArtifactResource() throws Exception { @Test @WithUser(principal = "4712", authorities = "ROLE_CONTROLLER", allSpPermissions = true) @Description("Tests valid downloads through the artifact resource by identifying the artifact not by ID but file name.") - public void downloadArtifactThroughFileName() throws Exception { - downLoadProgress = 1; + void downloadArtifactThroughFileName() throws Exception { + downloadProgress = 1; shippedBytes = 0; assertThat(softwareModuleManagement.findAll(PAGE)).isEmpty(); @@ -199,13 +199,13 @@ public void downloadArtifactThroughFileName() throws Exception { "The same file that was uploaded is expected when downloaded"); // download complete - assertThat(downLoadProgress).isEqualTo(10); + assertThat(downloadProgress).isEqualTo(10); assertThat(shippedBytes).isEqualTo(artifactSize); } @Test @Description("Tests valid MD5SUm file downloads through the artifact resource by identifying the artifact by ID.") - public void downloadMd5sumThroughControllerApi() throws Exception { + void downloadMd5sumThroughControllerApi() throws Exception { // create target final Target target = testdataFactory.createTarget(); @@ -236,7 +236,7 @@ public void downloadMd5sumThroughControllerApi() throws Exception { @Test @WithUser(principal = TestdataFactory.DEFAULT_CONTROLLER_ID, authorities = "ROLE_CONTROLLER", allSpPermissions = true) @Description("Test various HTTP range requests for artifact download, e.g. chunk download or download resume.") - public void rangeDownloadArtifact() throws Exception { + void rangeDownloadArtifact() throws Exception { // create target final Target target = testdataFactory.createTarget(); final List targets = Collections.singletonList(target); @@ -312,8 +312,7 @@ public void rangeDownloadArtifact() throws Exception { .andExpect(header().string("Accept-Ranges", "bytes")) .andExpect(header().string("Last-Modified", dateFormat.format(new Date(artifact.getCreatedAt())))) .andExpect(header().longValue("Content-Length", resultLength - 1000)) - .andExpect(header().string("Content-Range", - "bytes " + 1000 + "-" + (resultLength - 1) + "/" + resultLength)) + .andExpect(header().string("Content-Range", "bytes " + 1000 + "-" + (resultLength - 1) + "/" + resultLength)) .andExpect(header().string("Content-Disposition", "attachment;filename=file1")) .andReturn(); @@ -361,10 +360,10 @@ public void rangeDownloadArtifact() throws Exception { } @Configuration - public static class DownloadTestConfiguration { + static class DownloadTestConfiguration { @Bean - public Listener cancelEventHandlerStubBean() { + Listener cancelEventHandlerStubBean() { return new Listener(); } @@ -373,10 +372,9 @@ public Listener cancelEventHandlerStubBean() { private static class Listener { @EventListener(classes = DownloadProgressEvent.class) - public static void listen(final DownloadProgressEvent event) { - downLoadProgress++; + void listen(final DownloadProgressEvent event) { + downloadProgress++; shippedBytes += event.getShippedBytesSinceLast(); - } } } \ No newline at end of file diff --git a/hawkbit-ddi/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiDeploymentBaseTest.java b/hawkbit-ddi/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiDeploymentBaseTest.java index 8dc162253b..bc0ff8eb7e 100644 --- a/hawkbit-ddi/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiDeploymentBaseTest.java +++ b/hawkbit-ddi/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiDeploymentBaseTest.java @@ -800,7 +800,7 @@ private static class ActionStatusCondition extends Condition { private final Action.Status status; - public ActionStatusCondition(final Action.Status status) { + private ActionStatusCondition(final Action.Status status) { this.status = status; } diff --git a/hawkbit-ddi/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiInstalledBaseTest.java b/hawkbit-ddi/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiInstalledBaseTest.java index e0993ad1ca..460790538f 100644 --- a/hawkbit-ddi/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiInstalledBaseTest.java +++ b/hawkbit-ddi/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DdiInstalledBaseTest.java @@ -69,7 +69,7 @@ */ @Feature("Component Tests - Direct Device Integration API") @Story("Installed Base Resource") -public class DdiInstalledBaseTest extends AbstractDDiApiIntegrationTest { +class DdiInstalledBaseTest extends AbstractDDiApiIntegrationTest { @Autowired ActionStatusRepository actionStatusRepository; @@ -78,7 +78,7 @@ public class DdiInstalledBaseTest extends AbstractDDiApiIntegrationTest { @Test @Description("Ensure that the installed base resource is available as CBOR") - public void installedBaseResourceCbor() throws Exception { + void installedBaseResourceCbor() throws Exception { final Target target = testdataFactory.createTarget(); final DistributionSet ds = testdataFactory.createDistributionSet(""); @@ -100,7 +100,7 @@ public void installedBaseResourceCbor() throws Exception { @Test @Description("Ensure that assigned version is self assigned version") - public void installedVersion() throws Exception { + void installedVersion() throws Exception { final Target target = createTargetAndAssertNoActiveActions(); final DistributionSet ds = testdataFactory.createDistributionSet(""); @@ -115,7 +115,7 @@ public void installedVersion() throws Exception { @Test @Description("Ensure that installedVersion is version self assigned") - public void installedVersionNotExist() throws Exception { + void installedVersionNotExist() throws Exception { final Target target = createTargetAndAssertNoActiveActions(); final String dsName = "unknown"; final String dsVersion = "1.0.0"; @@ -128,7 +128,7 @@ public void installedVersionNotExist() throws Exception { @Test @Description("Test several deployments to a controller. Checks that action is represented as installedBase after installation.") - public void deploymentSeveralActionsInInstalledBase() throws Exception { + void deploymentSeveralActionsInInstalledBase() throws Exception { // Prepare test data final Target target = createTargetAndAssertNoActiveActions(); @@ -196,7 +196,7 @@ public void deploymentSeveralActionsInInstalledBase() throws Exception { @Test @Description("Test several deployments of same ds to a controller. Checks that cancelled action in history is not linked as installedBase.") - public void deploymentActionsOfSameDsWithCancelledActionInHistory() throws Exception { + void deploymentActionsOfSameDsWithCancelledActionInHistory() throws Exception { // Prepare test data final Target target = createTargetAndAssertNoActiveActions(); @@ -252,7 +252,7 @@ public void deploymentActionsOfSameDsWithCancelledActionInHistory() throws Excep @Test @Description("Test several deployments of same ds to a controller. Checks that latest cancelled action does not override actual installed ds.") - public void deploymentActionsOfSameDsWithCancelledAction() throws Exception { + void deploymentActionsOfSameDsWithCancelledAction() throws Exception { // Prepare test data final Target target = createTargetAndAssertNoActiveActions(); @@ -309,7 +309,7 @@ public void deploymentActionsOfSameDsWithCancelledAction() throws Exception { @Test @Description("Test several deployments of same ds to a controller. Checks that latest running action does not override actual installed ds.") - public void deploymentActionsOfSameDsWithRunningAction() throws Exception { + void deploymentActionsOfSameDsWithRunningAction() throws Exception { // Prepare test data final Target target = createTargetAndAssertNoActiveActions(); @@ -362,7 +362,7 @@ public void deploymentActionsOfSameDsWithRunningAction() throws Exception { @Test @Description("Test open deployment to a controller. Checks that installedBase returns 404 for a pending action.") - public void installedBaseReturns404ForPendingAction() throws Exception { + void installedBaseReturns404ForPendingAction() throws Exception { // Prepare test data final Target target = createTargetAndAssertNoActiveActions(); final DistributionSet ds = testdataFactory.createDistributionSet(""); @@ -385,7 +385,7 @@ public void installedBaseReturns404ForPendingAction() throws Exception { @Test @Description("Ensures that artifacts are found, after the action was already closed.") - public void artifactsOfInstalledActionExist() throws Exception { + void artifactsOfInstalledActionExist() throws Exception { final Target target = createTargetAndAssertNoActiveActions(); final DistributionSet ds = testdataFactory.createDistributionSet(""); @@ -419,7 +419,7 @@ public void artifactsOfInstalledActionExist() throws Exception { @Expect(type = TargetUpdatedEvent.class, count = 2), @Expect(type = TargetAttributesRequestedEvent.class, count = 1), @Expect(type = TargetPollEvent.class, count = 1) }) - public void deploymentActionInInstalledBase(final Action.ActionType actionType) throws Exception { + void deploymentActionInInstalledBase(final Action.ActionType actionType) throws Exception { // Prepare test data final Target target = createTargetAndAssertNoActiveActions(); final DistributionSet ds = testdataFactory.createDistributionSet("", true); @@ -472,7 +472,7 @@ public void deploymentActionInInstalledBase(final Action.ActionType actionType) @Expect(type = TargetUpdatedEvent.class, count = 2), @Expect(type = TargetAttributesRequestedEvent.class, count = 1), @Expect(type = TargetPollEvent.class, count = 2) }) - public void deploymentDownloadOnlyActionNotInInstalledBase() throws Exception { + void deploymentDownloadOnlyActionNotInInstalledBase() throws Exception { // Prepare test data final Target target = testdataFactory.createTarget(); final DistributionSet ds = testdataFactory.createDistributionSet(""); @@ -501,7 +501,7 @@ public void deploymentDownloadOnlyActionNotInInstalledBase() throws Exception { @ParameterizedTest @MethodSource("org.eclipse.hawkbit.ddi.rest.resource.DdiInstalledBaseTest#actionTypeForDeployment") @Description("Test a failed deployment to a controller. Checks that closed action is not represented as installedBase.") - public void deploymentActionFailedNotInInstalledBase(final Action.ActionType actionType) throws Exception { + void deploymentActionFailedNotInInstalledBase(final Action.ActionType actionType) throws Exception { // Prepare test data final Target target = testdataFactory.createTarget(); final DistributionSet ds = testdataFactory.createDistributionSet(""); @@ -534,7 +534,7 @@ public void deploymentActionFailedNotInInstalledBase(final Action.ActionType act @Test @Description("Test to verify that only a specific count of messages are returned based on the input actionHistory for getControllerInstalledAction endpoint.") - public void testActionHistoryCount() throws Exception { + void testActionHistoryCount() throws Exception { final DistributionSet ds = testdataFactory.createDistributionSet(""); Target savedTarget = testdataFactory.createTarget("911"); savedTarget = getFirstAssignedTarget(assignDistributionSet(ds.getId(), savedTarget.getControllerId())); @@ -585,7 +585,7 @@ public void testActionHistoryCount() throws Exception { @Test @Description("Test various invalid access attempts to the installed resource und the expected behaviour of the server.") - public void badInstalledAction() throws Exception { + void badInstalledAction() throws Exception { final Target target = testdataFactory.createTarget(CONTROLLER_ID); // not allowed methods diff --git a/hawkbit-rest-core/src/main/java/org/eclipse/hawkbit/rest/util/FileStreamingUtil.java b/hawkbit-rest-core/src/main/java/org/eclipse/hawkbit/rest/util/FileStreamingUtil.java index 4694123de0..a408e94429 100644 --- a/hawkbit-rest-core/src/main/java/org/eclipse/hawkbit/rest/util/FileStreamingUtil.java +++ b/hawkbit-rest-core/src/main/java/org/eclipse/hawkbit/rest/util/FileStreamingUtil.java @@ -102,8 +102,7 @@ public static ResponseEntity writeFileResponse(final DbArtifact art return new ResponseEntity<>(HttpStatus.REQUESTED_RANGE_NOT_SATISFIABLE); } - // RFC: if the representation is unchanged, send me the part(s) that - // I am requesting in + // RFC: if the representation is unchanged, send me the part(s) that I am requesting in // Range; otherwise, send me the entire representation. checkForShortcut(request, etag, lastModified, full, ranges);