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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
