You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2022/11/02 11:09:57 UTC
[GitHub] [fineract] galovics commented on a diff in pull request #2718: FINEARCT-1744 add command execution 2 step transaction handling
galovics commented on code in PR #2718:
URL: https://github.com/apache/fineract/pull/2718#discussion_r1011570061
##########
fineract-provider/src/main/java/org/apache/fineract/commands/service/SynchronousCommandProcessingService.java:
##########
@@ -69,45 +76,43 @@ public class SynchronousCommandProcessingService implements CommandProcessingSer
private final IdempotencyKeyGenerator idempotencyKeyGenerator;
private final FineractProperties fineractProperties;
+ private final InitialCommandSaver initialCommandSaver;
+
@Transactional
@Override
public CommandProcessingResult executeCommand(final CommandWrapper wrapper, final JsonCommand command,
final boolean isApprovedByChecker) {
final boolean rollbackTransaction = configurationDomainService.isMakerCheckerEnabledForTask(wrapper.taskPermissionName());
+ String idempotencyKey = getIdempotencyKey(wrapper);
+ CommandSource existingCommand = commandSourceRepository.findByActionNameAndEntityNameAndIdempotencyKey(wrapper.actionName(),
+ wrapper.entityName(), idempotencyKey);
+ final CommandProcessingResult result;
+ if (existingCommand != null) {
+ if (UNDER_PROCESSING.getValue().equals(existingCommand.getStatus())) {
+ throw new DuplicateCommandException(wrapper.actionName(), wrapper.entityName(), wrapper.getIdempotencyKey(),
+ wrapper.getJson(), UNDER_PROCESSING.getValue());
+ } else if (ERROR.getValue().equals(existingCommand.getStatus())) {
+ throw new DuplicateCommandException(wrapper.actionName(), wrapper.entityName(), wrapper.getIdempotencyKey(),
Review Comment:
I don't think this is right. In an error case, we want to return the original error result from the command execution.
##########
fineract-provider/src/main/java/org/apache/fineract/commands/service/SynchronousCommandProcessingService.java:
##########
@@ -69,45 +76,43 @@ public class SynchronousCommandProcessingService implements CommandProcessingSer
private final IdempotencyKeyGenerator idempotencyKeyGenerator;
private final FineractProperties fineractProperties;
+ private final InitialCommandSaver initialCommandSaver;
+
@Transactional
@Override
public CommandProcessingResult executeCommand(final CommandWrapper wrapper, final JsonCommand command,
final boolean isApprovedByChecker) {
final boolean rollbackTransaction = configurationDomainService.isMakerCheckerEnabledForTask(wrapper.taskPermissionName());
+ String idempotencyKey = getIdempotencyKey(wrapper);
+ CommandSource existingCommand = commandSourceRepository.findByActionNameAndEntityNameAndIdempotencyKey(wrapper.actionName(),
+ wrapper.entityName(), idempotencyKey);
+ final CommandProcessingResult result;
+ if (existingCommand != null) {
+ if (UNDER_PROCESSING.getValue().equals(existingCommand.getStatus())) {
+ throw new DuplicateCommandException(wrapper.actionName(), wrapper.entityName(), wrapper.getIdempotencyKey(),
+ wrapper.getJson(), UNDER_PROCESSING.getValue());
+ } else if (ERROR.getValue().equals(existingCommand.getStatus())) {
+ throw new DuplicateCommandException(wrapper.actionName(), wrapper.entityName(), wrapper.getIdempotencyKey(),
+ existingCommand.getResult(), ERROR.getValue());
+ } else if (PROCESSED.getValue().equals(existingCommand.getStatus())) {
+ throw new DuplicateCommandException(wrapper.actionName(), wrapper.entityName(), wrapper.getIdempotencyKey(),
Review Comment:
Not right. In case it's been already processed, we should return the original successful result.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/exception/DuplicateCommandException.java:
##########
@@ -0,0 +1,54 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.core.exception;
+
+/**
+ * Exception thrown when command is sent with same action, entity and idempotency key
+ */
+public class DuplicateCommandException extends AbstractPlatformException {
+
+ private final String action;
+ private final String entity;
+ private final String idempotencyKey;
+ private final String response;
+ private final int processingResult;
+
+ public DuplicateCommandException(String action, String entity, String idempotencyKey, String response, int processingResult) {
+ super(null, null);
+ this.action = action;
+ this.entity = entity;
+ this.idempotencyKey = idempotencyKey;
+ this.response = response;
+ this.processingResult = processingResult;
+ }
+
+ @Override
+ public String toString() {
Review Comment:
Replace with Lombok ToString pls.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/exception/DuplicateCommandException.java:
##########
@@ -0,0 +1,54 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.core.exception;
+
+/**
+ * Exception thrown when command is sent with same action, entity and idempotency key
+ */
+public class DuplicateCommandException extends AbstractPlatformException {
+
+ private final String action;
+ private final String entity;
+ private final String idempotencyKey;
+ private final String response;
+ private final int processingResult;
+
+ public DuplicateCommandException(String action, String entity, String idempotencyKey, String response, int processingResult) {
+ super(null, null);
+ this.action = action;
+ this.entity = entity;
+ this.idempotencyKey = idempotencyKey;
+ this.response = response;
+ this.processingResult = processingResult;
+ }
+
+ @Override
+ public String toString() {
+ return "DuplicateCommandException{" + "action='" + action + '\'' + ", entity='" + entity + '\'' + ", idempotencyKey='"
+ + idempotencyKey + '\'' + ", processingResult=" + processingResult + '}';
+ }
+
+ public int getProcessingResult() {
+ return processingResult;
+ }
+
+ public String getResponse() {
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/exceptionmapper/DuplicateCommandExceptionMapper.java:
##########
@@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.core.exceptionmapper;
+
+import static org.apache.fineract.commands.domain.CommandProcessingResultType.ERROR;
+import static org.apache.fineract.commands.domain.CommandProcessingResultType.PROCESSED;
+import static org.apache.fineract.commands.domain.CommandProcessingResultType.UNDER_PROCESSING;
+
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import javax.ws.rs.core.Response.Status;
+import javax.ws.rs.ext.ExceptionMapper;
+import javax.ws.rs.ext.Provider;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.fineract.infrastructure.core.exception.DuplicateCommandException;
+import org.springframework.stereotype.Component;
+
+@Provider
+@Component
+@Slf4j
+public class DuplicateCommandExceptionMapper implements ExceptionMapper<DuplicateCommandException> {
+
+ @Override
+ public Response toResponse(final DuplicateCommandException exception) {
+ log.debug("Duplicate request: {}", exception.toString());
+ if (PROCESSED.getValue() == exception.getProcessingResult() || ERROR.getValue() == exception.getProcessingResult()) {
Review Comment:
Please don't do this. :-)
Rather create different exceptions for the different use-cases.
##########
fineract-provider/src/main/java/org/apache/fineract/commands/service/SynchronousCommandProcessingService.java:
##########
@@ -69,45 +76,43 @@ public class SynchronousCommandProcessingService implements CommandProcessingSer
private final IdempotencyKeyGenerator idempotencyKeyGenerator;
private final FineractProperties fineractProperties;
+ private final InitialCommandSaver initialCommandSaver;
+
@Transactional
@Override
public CommandProcessingResult executeCommand(final CommandWrapper wrapper, final JsonCommand command,
final boolean isApprovedByChecker) {
final boolean rollbackTransaction = configurationDomainService.isMakerCheckerEnabledForTask(wrapper.taskPermissionName());
+ String idempotencyKey = getIdempotencyKey(wrapper);
+ CommandSource existingCommand = commandSourceRepository.findByActionNameAndEntityNameAndIdempotencyKey(wrapper.actionName(),
+ wrapper.entityName(), idempotencyKey);
+ final CommandProcessingResult result;
+ if (existingCommand != null) {
+ if (UNDER_PROCESSING.getValue().equals(existingCommand.getStatus())) {
+ throw new DuplicateCommandException(wrapper.actionName(), wrapper.entityName(), wrapper.getIdempotencyKey(),
+ wrapper.getJson(), UNDER_PROCESSING.getValue());
+ } else if (ERROR.getValue().equals(existingCommand.getStatus())) {
+ throw new DuplicateCommandException(wrapper.actionName(), wrapper.entityName(), wrapper.getIdempotencyKey(),
+ existingCommand.getResult(), ERROR.getValue());
+ } else if (PROCESSED.getValue().equals(existingCommand.getStatus())) {
+ throw new DuplicateCommandException(wrapper.actionName(), wrapper.entityName(), wrapper.getIdempotencyKey(),
+ existingCommand.getResult(), PROCESSED.getValue());
+ }
+ }
- final NewCommandSourceHandler handler = findCommandHandler(wrapper);
+ final AppUser maker = context.authenticatedUser(wrapper);
+ CommandSource commandSourceResult = getInitialCommandSource(wrapper, command, maker, idempotencyKey);
+ initialCommandSaver.save(commandSourceResult);
- final CommandProcessingResult result;
try {
- result = handler.processCommand(command);
+ result = findCommandHandler(wrapper).processCommand(command);
} catch (Throwable t) {
publishHookErrorEvent(wrapper, command, t);
throw t;
}
- final AppUser maker = context.authenticatedUser(wrapper);
-
- CommandSource commandSourceResult;
- if (command.commandId() != null) {
- commandSourceResult = commandSourceRepository.findById(command.commandId())
- .orElseThrow(() -> new CommandNotFoundException(command.commandId()));
- commandSourceResult.markAsChecked(maker);
- } else {
- String requestIdempotencyKey = null;
- RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();
- if (requestAttributes != null) {
- if (requestAttributes instanceof ServletRequestAttributes) {
- requestIdempotencyKey = ((ServletRequestAttributes) requestAttributes).getRequest()
- .getHeader(fineractProperties.getIdempotencyKeyHeaderName());
- }
- }
-
- commandSourceResult = CommandSource.fullEntryFrom(wrapper, command, maker,
- wrapper.getIdempotencyKey() == null
- ? (requestIdempotencyKey == null ? idempotencyKeyGenerator.create() : requestIdempotencyKey)
- : wrapper.getIdempotencyKey());
- }
+ commandSourceResult.setResult(toApiJsonSerializer.serializeResult(result)); // TODO
Review Comment:
Explain the TODO please.
##########
fineract-provider/src/main/java/org/apache/fineract/commands/service/SynchronousCommandProcessingService.java:
##########
@@ -147,6 +151,34 @@ public CommandProcessingResult executeCommand(final CommandWrapper wrapper, fina
return result;
}
+ private String getIdempotencyKey(CommandWrapper wrapper) {
Review Comment:
Don't we want to extract this method to a separate class just for the sake of making this class easier to test?
##########
fineract-provider/src/main/java/org/apache/fineract/commands/service/SynchronousCommandProcessingService.java:
##########
@@ -69,45 +76,43 @@ public class SynchronousCommandProcessingService implements CommandProcessingSer
private final IdempotencyKeyGenerator idempotencyKeyGenerator;
private final FineractProperties fineractProperties;
+ private final InitialCommandSaver initialCommandSaver;
+
@Transactional
@Override
public CommandProcessingResult executeCommand(final CommandWrapper wrapper, final JsonCommand command,
final boolean isApprovedByChecker) {
final boolean rollbackTransaction = configurationDomainService.isMakerCheckerEnabledForTask(wrapper.taskPermissionName());
+ String idempotencyKey = getIdempotencyKey(wrapper);
+ CommandSource existingCommand = commandSourceRepository.findByActionNameAndEntityNameAndIdempotencyKey(wrapper.actionName(),
+ wrapper.entityName(), idempotencyKey);
+ final CommandProcessingResult result;
+ if (existingCommand != null) {
+ if (UNDER_PROCESSING.getValue().equals(existingCommand.getStatus())) {
+ throw new DuplicateCommandException(wrapper.actionName(), wrapper.entityName(), wrapper.getIdempotencyKey(),
+ wrapper.getJson(), UNDER_PROCESSING.getValue());
+ } else if (ERROR.getValue().equals(existingCommand.getStatus())) {
+ throw new DuplicateCommandException(wrapper.actionName(), wrapper.entityName(), wrapper.getIdempotencyKey(),
+ existingCommand.getResult(), ERROR.getValue());
+ } else if (PROCESSED.getValue().equals(existingCommand.getStatus())) {
+ throw new DuplicateCommandException(wrapper.actionName(), wrapper.entityName(), wrapper.getIdempotencyKey(),
+ existingCommand.getResult(), PROCESSED.getValue());
+ }
+ }
- final NewCommandSourceHandler handler = findCommandHandler(wrapper);
+ final AppUser maker = context.authenticatedUser(wrapper);
+ CommandSource commandSourceResult = getInitialCommandSource(wrapper, command, maker, idempotencyKey);
+ initialCommandSaver.save(commandSourceResult);
Review Comment:
Couldn't we make this separation better? Just for creating the InitialCommandSaver, to work with the declarative transaction, it's fishy.
Instead why don't we move the whole initial command creation logic into there (including the getInitialCommandSource method).
That way the implementation would be more robust by just calling a "initialCommandCreator.create" method which would create and save the new command.
Thoughts?
Or... you could create a separate repository method which always saves the entity in a new transaction (using the Custom Spring repository way for example) and then you don't need to have this InitialCommandSaver.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/exception/DuplicateCommandException.java:
##########
@@ -0,0 +1,54 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.core.exception;
+
+/**
+ * Exception thrown when command is sent with same action, entity and idempotency key
+ */
+public class DuplicateCommandException extends AbstractPlatformException {
+
+ private final String action;
+ private final String entity;
+ private final String idempotencyKey;
+ private final String response;
+ private final int processingResult;
+
+ public DuplicateCommandException(String action, String entity, String idempotencyKey, String response, int processingResult) {
+ super(null, null);
+ this.action = action;
+ this.entity = entity;
+ this.idempotencyKey = idempotencyKey;
+ this.response = response;
+ this.processingResult = processingResult;
+ }
+
+ @Override
+ public String toString() {
+ return "DuplicateCommandException{" + "action='" + action + '\'' + ", entity='" + entity + '\'' + ", idempotencyKey='"
+ + idempotencyKey + '\'' + ", processingResult=" + processingResult + '}';
+ }
+
+ public int getProcessingResult() {
Review Comment:
Lombok Getter pls.
##########
fineract-provider/src/main/java/org/apache/fineract/commands/data/AuditSearchData.java:
##########
@@ -34,5 +34,5 @@ public final class AuditSearchData {
private final Collection<AppUserData> appUsers;
private final List<String> actionNames;
private final List<String> entityNames;
- private final Collection<ProcessingResultLookup> processingResults;
+ private final Collection<ProcessingResultLookup> status;
Review Comment:
status**es**, plural.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org