From e3e0bbd70097223a479f6d691580226ee3f9a918 Mon Sep 17 00:00:00 2001 From: Jeremy Bernard Date: Mon, 24 Mar 2025 12:51:03 +0100 Subject: [PATCH 1/2] Use CommandArgs instead of split parameters in methods arguments --- CHANGELOG.md | 1 + .../command/generic/CommandEngine.java | 31 ++++++------- .../command/generic/CommandStorage.java | 44 ++++++++----------- .../blockchain/chain/ChainConfigTest.java | 14 +++--- .../command/generic/CommandStorageTests.java | 14 +++--- .../finalize/TaskFinalizeServiceTests.java | 12 ++--- .../TaskInitializeServiceTests.java | 12 ++--- 7 files changed, 62 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45e84824..d0aa925a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ All notable changes to this project will be documented in this file. - Use no more `iexec-commons-poco` deprecated code in integration tests. (#163) - Fix Spring Security deprecations after Spring Boot 3.3.8 upgrade. (#169) - Harmonize YML internal variables to proper case. (#171) +- Use `CommandArgs` instead of split parameters in methods arguments. (#172) ### Breaking API changes diff --git a/src/main/java/com/iexec/blockchain/command/generic/CommandEngine.java b/src/main/java/com/iexec/blockchain/command/generic/CommandEngine.java index 80cc45bd..1af6614e 100644 --- a/src/main/java/com/iexec/blockchain/command/generic/CommandEngine.java +++ b/src/main/java/com/iexec/blockchain/command/generic/CommandEngine.java @@ -50,15 +50,14 @@ protected CommandEngine( * @return blockchain object ID if successful */ public String startBlockchainCommand(final A args, final boolean isPriority) { - final String chainObjectId = args.getChainObjectId(); - final String messageDetails = String.format("[chainObjectId:%s, commandArgs:%s]", chainObjectId, args); + final String messageDetails = String.format("chainObjectId:%s, commandArgs:%s", args.getChainObjectId(), args); if (!blockchainService.canSendBlockchainCommand(args)) { - log.error("Starting blockchain command failed (failing on-chain checks) {}", messageDetails); + log.error("Starting blockchain command failed (failing on-chain checks) [{}]", messageDetails); return ""; } if (!updaterService.updateToReceived(args)) { - log.error("Starting blockchain command failed (failing update to received) {}", messageDetails); + log.error("Starting blockchain command failed (failing update to received) [{}]", messageDetails); return ""; } log.info("Received command {}", messageDetails); @@ -66,7 +65,7 @@ public String startBlockchainCommand(final A args, final boolean isPriority) { final Runnable runnable = () -> triggerBlockchainCommand(args); queueService.addExecutionToQueue(runnable, isPriority); - return chainObjectId; + return args.getChainObjectId(); } /** @@ -77,33 +76,29 @@ public String startBlockchainCommand(final A args, final boolean isPriority) { * @param args input arguments for the blockchain command */ public void triggerBlockchainCommand(final A args) { - String chainObjectId = args.getChainObjectId(); - if (!updaterService.updateToProcessing(chainObjectId, args.getCommandName())) { - log.error("Triggering blockchain command failed (failing update" + - " to processing) [chainObjectId:{}, commandArgs:{}]", - chainObjectId, args); + final String messageDetails = String.format("chainObjectId:%s, commandArgs:%s", args.getChainObjectId(), args); + if (!updaterService.updateToProcessing(args)) { + log.error("Triggering blockchain command failed (failing update to processing) [{}]", messageDetails); return; } int attempt = 0; - log.info("Processing command [chainObjectId:{}, commandArgs:{}]", - chainObjectId, args); + log.info("Processing command [{}]", messageDetails); TransactionReceipt receipt = null; while (attempt < MAX_ATTEMPTS && receipt == null) { attempt++; try { receipt = blockchainService.sendBlockchainCommand(args); } catch (Exception e) { - log.error("Something wrong happened while triggering command [chainObjectId:{}, commandArgs:{}, attempt:{}]", - chainObjectId, args, attempt, e); + log.error("Something wrong happened while triggering command [{}, attempt:{}]", + messageDetails, attempt, e); } } if (receipt == null) { log.error("Triggering blockchain command failed " + - "(received null receipt after blockchain send) " + - "[chainObjectId:{}, commandArgs:{}, attempt:{}]", - chainObjectId, args, attempt); + "(received null receipt after blockchain send) [{}, attempt:{}]", + messageDetails, attempt); } - updaterService.updateToFinal(chainObjectId, args.getCommandName(), receipt); + updaterService.updateToFinal(args, receipt); } /** diff --git a/src/main/java/com/iexec/blockchain/command/generic/CommandStorage.java b/src/main/java/com/iexec/blockchain/command/generic/CommandStorage.java index 4d4bed79..ad2525c5 100644 --- a/src/main/java/com/iexec/blockchain/command/generic/CommandStorage.java +++ b/src/main/java/com/iexec/blockchain/command/generic/CommandStorage.java @@ -41,11 +41,10 @@ public CommandStorage(final MongoTemplate mongoTemplate) { } /** - * Locally set status to received and store blockchain command arguments for - * future use. + * Locally set status to received and store blockchain command arguments for future use. * * @param args input arguments for the blockchain command - * @return true on successful update + * @return true on successful update, false otherwise */ public boolean updateToReceived(final CommandArgs args) { final Command command = new Command(); @@ -66,14 +65,13 @@ public boolean updateToReceived(final CommandArgs args) { } /** - * Locally set status to processing just before sending the blockchain command + * Locally set status to processing just before sending the blockchain command. * - * @param chainObjectId blockchain object ID on which the blockchain command - * is performed - * @return true on successful update + * @param args Command arguments containing on-chain object ID and command to perform + * @return true on successful update, false otherwise */ - public boolean updateToProcessing(final String chainObjectId, final CommandName commandName) { - final Criteria criteria = createUpdateCriteria(chainObjectId, commandName, CommandStatus.RECEIVED); + public boolean updateToProcessing(final CommandArgs args) { + final Criteria criteria = createUpdateCriteria(args, CommandStatus.RECEIVED); final Update update = new Update(); update.set(STATUS_FIELD_NAME, CommandStatus.PROCESSING); update.set("processingDate", Instant.now()); @@ -82,20 +80,17 @@ public boolean updateToProcessing(final String chainObjectId, final CommandName } /** - * Locally set status both to success or failure, when blockchain command - * is completed. + * Locally set status both to success or failure, when blockchain command is completed. * - * @param chainObjectId blockchain object ID on which the blockchain command - * is performed - * @param receipt blockchain receipt + * @param args Command arguments containing on-chain object ID and command to perform + * @param receipt blockchain receipt + * @return true on successful update, false otherwise */ - public boolean updateToFinal(final String chainObjectId, - final CommandName commandName, - final TransactionReceipt receipt) { + public boolean updateToFinal(final CommandArgs args, final TransactionReceipt receipt) { final CommandStatus finalStatus = receipt != null && receipt.isStatusOK() ? CommandStatus.SUCCESS : CommandStatus.FAILURE; log.info("Command final status with transaction receipt [chainObjectId]:{}, command:{}, status:{}, receipt:{}]", - chainObjectId, commandName.name(), finalStatus, receipt); - final Criteria criteria = createUpdateCriteria(chainObjectId, commandName, CommandStatus.PROCESSING); + args.getChainObjectId(), args.getCommandName().name(), finalStatus, receipt); + final Criteria criteria = createUpdateCriteria(args, CommandStatus.PROCESSING); final Update update = new Update(); update.set(STATUS_FIELD_NAME, finalStatus); update.set("transactionReceipt", receipt); @@ -107,14 +102,13 @@ public boolean updateToFinal(final String chainObjectId, /** * Creates a criteria, the rule to lookup for a specific entry in the Mongo collection. * - * @param chainObjectId On-chain ID of the object to look for in collection - * @param commandName Name of the command applied to the on-chain object - * @param status Currently expected status for the on-chain object + * @param args Command arguments containing on-chain object ID and command to perform + * @param status Currently expected status for the on-chain object * @return A {@code Criteria} instance to use in {@code MongoTemplate} operations */ - private Criteria createUpdateCriteria(final String chainObjectId, final CommandName commandName, final CommandStatus status) { - return Criteria.where("chainObjectId").is(chainObjectId) - .and("commandName").is(commandName) + private Criteria createUpdateCriteria(final CommandArgs args, final CommandStatus status) { + return Criteria.where("chainObjectId").is(args.getChainObjectId()) + .and("commandName").is(args.getCommandName()) .and(STATUS_FIELD_NAME).is(status); } diff --git a/src/test/java/com/iexec/blockchain/chain/ChainConfigTest.java b/src/test/java/com/iexec/blockchain/chain/ChainConfigTest.java index 37d032cd..311bd995 100644 --- a/src/test/java/com/iexec/blockchain/chain/ChainConfigTest.java +++ b/src/test/java/com/iexec/blockchain/chain/ChainConfigTest.java @@ -16,7 +16,9 @@ package com.iexec.blockchain.chain; -import jakarta.validation.*; +import jakarta.validation.ConstraintViolation; +import jakarta.validation.Validation; +import jakarta.validation.ValidatorFactory; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -25,7 +27,7 @@ import java.util.Set; import java.util.stream.Stream; -import static org.assertj.core.api.Assertions.*; +import static org.assertj.core.api.Assertions.assertThat; class ChainConfigTest { private static final int DEFAULT_CHAIN_ID = 1; @@ -109,9 +111,9 @@ void shouldNotValidateChainId(int chainId) { static Stream invalidNodeAddresses() { return Stream.of( Arguments.of(null, "Node address must not be empty"), - Arguments.of ("", "Node address must not be empty"), - Arguments.of ("12345", "Node address must be a valid URL"), - Arguments.of ("0xBF6B2B07e47326B7c8bfCb4A5460bef9f0Fd2002", "Node address must be a valid URL") + Arguments.of("", "Node address must not be empty"), + Arguments.of("12345", "Node address must be a valid URL"), + Arguments.of("0xBF6B2B07e47326B7c8bfCb4A5460bef9f0Fd2002", "Node address must be a valid URL") ); } @@ -140,7 +142,7 @@ static Stream invalidBlockTimes() { Arguments.of(Duration.ofSeconds(0), "Block time must be greater than 100ms"), Arguments.of(Duration.ofSeconds(25), "Block time must be less than 20s"), Arguments.of(Duration.ofSeconds(-1), "Block time must be greater than 100ms"), - Arguments.of(null, "Block time must not be null") + Arguments.of(null, "Block time must not be null") ); } diff --git a/src/test/java/com/iexec/blockchain/command/generic/CommandStorageTests.java b/src/test/java/com/iexec/blockchain/command/generic/CommandStorageTests.java index b2839f67..5bb9ceda 100644 --- a/src/test/java/com/iexec/blockchain/command/generic/CommandStorageTests.java +++ b/src/test/java/com/iexec/blockchain/command/generic/CommandStorageTests.java @@ -84,7 +84,7 @@ void shouldNotSetReceivedSinceAlreadyPresent() { void shouldSetProcessing() { final TaskInitializeArgs args = getArgs(); Assertions.assertTrue(updaterService.updateToReceived(args)); - Assertions.assertTrue(updaterService.updateToProcessing(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE)); + Assertions.assertTrue(updaterService.updateToProcessing(args)); final CommandStatus status = updaterService.getStatusForCommand(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE).orElseThrow(); Assertions.assertEquals(CommandStatus.PROCESSING, status); } @@ -92,31 +92,34 @@ void shouldSetProcessing() { @ParameterizedTest @EnumSource(value = CommandStatus.class, names = "RECEIVED", mode = EnumSource.Mode.EXCLUDE) void shouldNotSetProcessingSinceBadStatus(final CommandStatus status) { + final TaskInitializeArgs args = getArgs(); final Command command = createCommand(status); mongoTemplate.insert(command); - Assertions.assertFalse(updaterService.updateToProcessing(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE)); + Assertions.assertFalse(updaterService.updateToProcessing(args)); } @Test void shouldSetFinalSuccess() { + final TaskInitializeArgs args = getArgs(); final TransactionReceipt receipt = new TransactionReceipt(); receipt.setStatus("0x1"); final Command command = createCommand(CommandStatus.PROCESSING); mongoTemplate.insert(command); - Assertions.assertTrue(updaterService.updateToFinal(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE, receipt)); + Assertions.assertTrue(updaterService.updateToFinal(args, receipt)); final CommandStatus status = updaterService.getStatusForCommand(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE).orElseThrow(); Assertions.assertEquals(CommandStatus.SUCCESS, status); } @Test void shouldSetFinalFailure() { + final TaskInitializeArgs args = getArgs(); final TransactionReceipt receipt = new TransactionReceipt(); receipt.setStatus("0x0"); final Command command = createCommand(CommandStatus.PROCESSING); mongoTemplate.insert(command); - Assertions.assertTrue(updaterService.updateToFinal(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE, receipt)); + Assertions.assertTrue(updaterService.updateToFinal(args, receipt)); final CommandStatus status = updaterService.getStatusForCommand(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE).orElseThrow(); Assertions.assertEquals(CommandStatus.FAILURE, status); } @@ -124,12 +127,13 @@ void shouldSetFinalFailure() { @ParameterizedTest @EnumSource(value = CommandStatus.class, names = "PROCESSING", mode = EnumSource.Mode.EXCLUDE) void shouldNotSetFinalSinceBadStatus(final CommandStatus status) { + final TaskInitializeArgs args = getArgs(); final TransactionReceipt receipt = new TransactionReceipt(); final Command taskInitialize = new Command(); taskInitialize.setStatus(status); mongoTemplate.insert(taskInitialize); - Assertions.assertFalse(updaterService.updateToFinal(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE, receipt)); + Assertions.assertFalse(updaterService.updateToFinal(args, receipt)); } private Command createCommand(final CommandStatus status) { diff --git a/src/test/java/com/iexec/blockchain/command/task/finalize/TaskFinalizeServiceTests.java b/src/test/java/com/iexec/blockchain/command/task/finalize/TaskFinalizeServiceTests.java index df89328f..d390ba92 100644 --- a/src/test/java/com/iexec/blockchain/command/task/finalize/TaskFinalizeServiceTests.java +++ b/src/test/java/com/iexec/blockchain/command/task/finalize/TaskFinalizeServiceTests.java @@ -106,30 +106,30 @@ void shouldNotFinalizeTaskSinceCannotUpdate() { @Test void triggerFinalizeTask() throws Exception { final TransactionReceipt receipt = mock(TransactionReceipt.class); - when(updaterService.updateToProcessing(CHAIN_TASK_ID, CommandName.TASK_FINALIZE)).thenReturn(true); + when(updaterService.updateToProcessing(args)).thenReturn(true); when(blockchainService.sendBlockchainCommand(args)).thenReturn(receipt); taskFinalizeService.triggerBlockchainCommand(args); - verify(updaterService).updateToFinal(CHAIN_TASK_ID, CommandName.TASK_FINALIZE, receipt); + verify(updaterService).updateToFinal(args, receipt); } @Test void shouldNotTriggerFinalizeTaskSinceCannotUpdate() { final TransactionReceipt receipt = mock(TransactionReceipt.class); - when(updaterService.updateToProcessing(CHAIN_TASK_ID, CommandName.TASK_FINALIZE)).thenReturn(false); + when(updaterService.updateToProcessing(args)).thenReturn(false); taskFinalizeService.triggerBlockchainCommand(args); - verify(updaterService, never()).updateToFinal(CHAIN_TASK_ID, CommandName.TASK_FINALIZE, receipt); + verify(updaterService, never()).updateToFinal(args, receipt); verifyNoInteractions(blockchainService); } @Test void shouldNotTriggerFinalizeTaskSinceReceiptIsNull() throws Exception { - when(updaterService.updateToProcessing(CHAIN_TASK_ID, CommandName.TASK_FINALIZE)).thenReturn(true); + when(updaterService.updateToProcessing(args)).thenReturn(true); when(blockchainService.sendBlockchainCommand(args)).thenReturn(null); taskFinalizeService.triggerBlockchainCommand(args); - verify(updaterService).updateToFinal(CHAIN_TASK_ID, CommandName.TASK_FINALIZE, null); + verify(updaterService).updateToFinal(args, null); } // endregion diff --git a/src/test/java/com/iexec/blockchain/command/task/initialize/TaskInitializeServiceTests.java b/src/test/java/com/iexec/blockchain/command/task/initialize/TaskInitializeServiceTests.java index 0db8ef38..1acaa6ea 100644 --- a/src/test/java/com/iexec/blockchain/command/task/initialize/TaskInitializeServiceTests.java +++ b/src/test/java/com/iexec/blockchain/command/task/initialize/TaskInitializeServiceTests.java @@ -108,30 +108,30 @@ void shouldNotInitializeTaskSinceCannotUpdate() { @Test void triggerInitializeTask() throws Exception { final TransactionReceipt receipt = mock(TransactionReceipt.class); - when(updaterService.updateToProcessing(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE)).thenReturn(true); + when(updaterService.updateToProcessing(args)).thenReturn(true); when(blockchainCheckerService.sendBlockchainCommand(args)).thenReturn(receipt); taskInitializeService.triggerBlockchainCommand(args); - verify(updaterService).updateToFinal(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE, receipt); + verify(updaterService).updateToFinal(args, receipt); } @Test void shouldNotTriggerInitializeTaskSinceCannotUpdate() { final TransactionReceipt receipt = mock(TransactionReceipt.class); - when(updaterService.updateToProcessing(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE)).thenReturn(false); + when(updaterService.updateToProcessing(args)).thenReturn(false); taskInitializeService.triggerBlockchainCommand(args); - verify(updaterService, never()).updateToFinal(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE, receipt); + verify(updaterService, never()).updateToFinal(args, receipt); verifyNoInteractions(blockchainCheckerService); } @Test void shouldNotTriggerInitializeTaskSinceReceiptIsNull() throws Exception { - when(updaterService.updateToProcessing(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE)).thenReturn(true); + when(updaterService.updateToProcessing(args)).thenReturn(true); when(blockchainCheckerService.sendBlockchainCommand(args)).thenReturn(null); taskInitializeService.triggerBlockchainCommand(args); - verify(updaterService).updateToFinal(CHAIN_TASK_ID, CommandName.TASK_INITIALIZE, null); + verify(updaterService).updateToFinal(args, null); } // endregion From d24c162cea9f641539e8d2e484dbbb597597a3b0 Mon Sep 17 00:00:00 2001 From: Jeremy Bernard Date: Mon, 24 Mar 2025 16:56:29 +0100 Subject: [PATCH 2/2] Fix logging message in CommandStorage --- .../com/iexec/blockchain/command/generic/CommandStorage.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/iexec/blockchain/command/generic/CommandStorage.java b/src/main/java/com/iexec/blockchain/command/generic/CommandStorage.java index ad2525c5..5fe4c01e 100644 --- a/src/main/java/com/iexec/blockchain/command/generic/CommandStorage.java +++ b/src/main/java/com/iexec/blockchain/command/generic/CommandStorage.java @@ -88,7 +88,7 @@ public boolean updateToProcessing(final CommandArgs args) { */ public boolean updateToFinal(final CommandArgs args, final TransactionReceipt receipt) { final CommandStatus finalStatus = receipt != null && receipt.isStatusOK() ? CommandStatus.SUCCESS : CommandStatus.FAILURE; - log.info("Command final status with transaction receipt [chainObjectId]:{}, command:{}, status:{}, receipt:{}]", + log.info("Command final status with transaction receipt [chainObjectId:{}, command:{}, status:{}, receipt:{}]", args.getChainObjectId(), args.getCommandName().name(), finalStatus, receipt); final Criteria criteria = createUpdateCriteria(args, CommandStatus.PROCESSING); final Update update = new Update();