Skip to content

Commit 1b5390a

Browse files
elnafatehelnafateh
committed
FINERACT-1420: Idempotency fallback with client retry safety
1 parent 2e11308 commit 1b5390a

5 files changed

Lines changed: 132 additions & 95 deletions

File tree

‎fineract-core/src/main/java/org/apache/fineract/commands/service/DeterministicIdempotencyKeyGenerator.java‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818
*/
1919
package org.apache.fineract.commands.service;
2020

21+
import com.fasterxml.jackson.core.JsonFactory;
22+
import com.fasterxml.jackson.core.JsonParser;
2123
import com.fasterxml.jackson.databind.JsonNode;
2224
import com.fasterxml.jackson.databind.ObjectMapper;
2325
import com.fasterxml.jackson.databind.node.ArrayNode;
@@ -29,14 +31,20 @@
2931
import java.util.Base64;
3032
import java.util.Collections;
3133
import java.util.List;
32-
import lombok.RequiredArgsConstructor;
3334
import org.springframework.stereotype.Component;
3435

3536
@Component
36-
@RequiredArgsConstructor
3737
public class DeterministicIdempotencyKeyGenerator {
3838

39-
private final ObjectMapper objectMapper;
39+
// Plain ObjectMapper for canonicalization — must NOT use the application ObjectMapper
40+
// which has custom serializers/deserializers that cause failures on certain JSON payloads
41+
private static final ObjectMapper CANONICAL_MAPPER;
42+
43+
static {
44+
JsonFactory factory = new JsonFactory();
45+
factory.enable(JsonParser.Feature.ALLOW_SINGLE_QUOTES);
46+
CANONICAL_MAPPER = new ObjectMapper(factory);
47+
}
4048

4149
public String generate(String json, String context) {
4250

@@ -52,17 +60,17 @@ public String generate(String json, String context) {
5260

5361
private String toCanonicalString(String json) {
5462
try {
55-
JsonNode node = objectMapper.readTree(json);
63+
JsonNode node = CANONICAL_MAPPER.readTree(json);
5664
JsonNode canonical = canonicalize(node);
57-
return objectMapper.writeValueAsString(canonical);
65+
return CANONICAL_MAPPER.writeValueAsString(canonical);
5866
} catch (Exception e) {
5967
throw new RuntimeException("Failed to canonicalize JSON", e);
6068
}
6169
}
6270

6371
private JsonNode canonicalize(JsonNode node) {
6472
if (node.isObject()) {
65-
ObjectNode sorted = objectMapper.createObjectNode();
73+
ObjectNode sorted = CANONICAL_MAPPER.createObjectNode();
6674

6775
List<String> fieldNames = new ArrayList<>();
6876
node.fieldNames().forEachRemaining(fieldNames::add);
@@ -76,7 +84,7 @@ private JsonNode canonicalize(JsonNode node) {
7684
}
7785

7886
if (node.isArray()) {
79-
ArrayNode arrayNode = objectMapper.createArrayNode();
87+
ArrayNode arrayNode = CANONICAL_MAPPER.createArrayNode();
8088
for (JsonNode element : node) {
8189
arrayNode.add(canonicalize(element)); // recursion inside array
8290
}

‎fineract-core/src/main/java/org/apache/fineract/commands/service/IdempotencyKeyResolver.java‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,25 @@ public ResolvedKey resolveWithMeta(CommandWrapper wrapper) {
6666
return new ResolvedKey(randomKeyGenerator.create(), false);
6767
}
6868

69-
// 6. Client/entity-scoped operation — generate deterministic key
69+
// 6. Job commands — always use random key since jobs must run every invocation
70+
// even with the same payload (e.g. same loan IDs for COB across different business dates)
71+
if (wrapper.getJobName() != null && !wrapper.getJobName().isBlank()) {
72+
return new ResolvedKey(randomKeyGenerator.create(), false);
73+
}
74+
75+
// 7. Account transfers — the ONLY operation where deterministic idempotency
76+
// is genuinely needed. A network timeout during a transfer means the client
77+
// cannot know if money was moved. Retrying with a random key would create
78+
// a duplicate transfer. Deterministic key ensures the retry returns the
79+
// cached result instead of moving money twice.
80+
String entityName = wrapper.getEntityName() != null ? wrapper.getEntityName().toUpperCase() : "";
81+
String actionName = wrapper.getActionName() != null ? wrapper.getActionName().toUpperCase() : "";
82+
boolean isAccountTransfer = actionName.equals("CREATE") && entityName.equals("ACCOUNTTRANSFER");
83+
if (!isAccountTransfer) {
84+
return new ResolvedKey(randomKeyGenerator.create(), false);
85+
}
86+
87+
// 8. Account transfer — generate deterministic key to prevent duplicate transfers
7088
String deterministicKey = deterministicGenerator.generate(wrapper.getJson(), buildContext(wrapper));
7189
fineractRequestContextHolder.setAttribute(SynchronousCommandProcessingService.IDEMPOTENCY_KEY_ATTRIBUTE, deterministicKey);
7290
return new ResolvedKey(deterministicKey, true);

‎fineract-core/src/test/java/org/apache/fineract/commands/service/DeterministicIdempotencyKeyGeneratorTest.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,11 @@
2222
import static org.junit.jupiter.api.Assertions.assertNotEquals;
2323
import static org.junit.jupiter.api.Assertions.assertThrows;
2424

25-
import com.fasterxml.jackson.databind.ObjectMapper;
2625
import org.junit.jupiter.api.Test;
2726

2827
class DeterministicIdempotencyKeyGeneratorTest {
2928

30-
private final DeterministicIdempotencyKeyGenerator underTest = new DeterministicIdempotencyKeyGenerator(new ObjectMapper());
29+
private final DeterministicIdempotencyKeyGenerator underTest = new DeterministicIdempotencyKeyGenerator();
3130

3231
@Test
3332
void shouldGenerateSameKeyForSameInputAndContext() {

‎fineract-provider/src/test/java/org/apache/fineract/commands/service/IdempotencyKeyResolverTest.java‎

Lines changed: 97 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,6 @@
1919
package org.apache.fineract.commands.service;
2020

2121
import static org.mockito.ArgumentMatchers.anyString;
22-
import static org.mockito.ArgumentMatchers.contains;
23-
import static org.mockito.ArgumentMatchers.eq;
2422
import static org.mockito.Mockito.mock;
2523
import static org.mockito.Mockito.never;
2624
import static org.mockito.Mockito.verify;
@@ -77,31 +75,18 @@ public void testIPKResolveFromRequest() {
7775
}
7876

7977
@Test
80-
void shouldGenerateKeyUsingContextWhenHeaderMissing() {
81-
CommandWrapper wrapper = mock(CommandWrapper.class);
82-
83-
when(wrapper.getIdempotencyKey()).thenReturn(null);
84-
when(wrapper.getJson()).thenReturn("{\"a\":1}");
85-
when(wrapper.getActionName()).thenReturn("action");
86-
when(wrapper.getEntityName()).thenReturn("entity");
87-
when(wrapper.getHref()).thenReturn("/endpoint");
88-
when(wrapper.getClientId()).thenReturn(1L);
89-
when(wrapper.getEntityId()).thenReturn(null);
90-
when(wrapper.getJobName()).thenReturn(null);
91-
92-
when(deterministicIdempotencyKeyGenerator.generate(anyString(), anyString())).thenReturn("generated-key");
93-
94-
String result = underTest.resolve(wrapper);
95-
96-
verify(deterministicIdempotencyKeyGenerator).generate(eq("{\"a\":1}"), contains("action:entity:/endpoint:1:"));
97-
Assertions.assertEquals("generated-key", result);
98-
78+
public void testIPKResolveFromWrapper() {
79+
String idk = "idk";
80+
CommandWrapper wrapper = new CommandWrapper(null, null, null, null, null, null, null, null, null, null, null, null, null, null,
81+
null, null, null, idk, null, null);
82+
String resolvedIdk = underTest.resolve(wrapper);
83+
Assertions.assertEquals(idk, resolvedIdk);
84+
verifyNoInteractions(deterministicIdempotencyKeyGenerator);
9985
}
10086

10187
@Test
10288
void shouldUseHeaderIdempotencyKeyWhenPresent() {
10389
CommandWrapper wrapper = mock(CommandWrapper.class);
104-
10590
when(wrapper.getIdempotencyKey()).thenReturn("header-key");
10691

10792
String result = underTest.resolve(wrapper);
@@ -113,22 +98,20 @@ void shouldUseHeaderIdempotencyKeyWhenPresent() {
11398
@Test
11499
void shouldFallbackToRandomWhenJsonMissing() {
115100
CommandWrapper wrapper = mock(CommandWrapper.class);
116-
117101
when(wrapper.getIdempotencyKey()).thenReturn(null);
118102
when(wrapper.getJson()).thenReturn(null);
119103
when(wrapper.getClientId()).thenReturn(1L);
120104
when(wrapper.getEntityId()).thenReturn(1L);
121105

122106
String result = underTest.resolve(wrapper);
123107

124-
Assertions.assertNotNull(result);
108+
Assertions.assertEquals("random-key", result);
125109
verifyNoInteractions(deterministicIdempotencyKeyGenerator);
126110
}
127111

128112
@Test
129113
void shouldFallbackToRandomWhenNoClientAndEntity() {
130114
CommandWrapper wrapper = mock(CommandWrapper.class);
131-
132115
when(wrapper.getIdempotencyKey()).thenReturn(null);
133116
when(wrapper.getJson()).thenReturn("{\"a\":1}");
134117
when(wrapper.getClientId()).thenReturn(null);
@@ -137,14 +120,13 @@ void shouldFallbackToRandomWhenNoClientAndEntity() {
137120

138121
String result = underTest.resolve(wrapper);
139122

140-
Assertions.assertNotNull(result);
123+
Assertions.assertEquals("random-key", result);
141124
verifyNoInteractions(deterministicIdempotencyKeyGenerator);
142125
}
143126

144127
@Test
145128
void shouldFallbackToRandomForConfigurationsEndpoint() {
146129
CommandWrapper wrapper = mock(CommandWrapper.class);
147-
148130
when(wrapper.getIdempotencyKey()).thenReturn(null);
149131
when(wrapper.getJson()).thenReturn("{\"a\":1}");
150132
when(wrapper.getClientId()).thenReturn(1L);
@@ -153,34 +135,67 @@ void shouldFallbackToRandomForConfigurationsEndpoint() {
153135

154136
String result = underTest.resolve(wrapper);
155137

156-
Assertions.assertNotNull(result);
138+
Assertions.assertEquals("random-key", result);
157139
verifyNoInteractions(deterministicIdempotencyKeyGenerator);
158140
}
159141

160142
@Test
161-
void shouldMarkDeterministicTrueWhenGenerated() {
143+
void shouldFallbackToRandomForJobCommands() {
162144
CommandWrapper wrapper = mock(CommandWrapper.class);
145+
when(wrapper.getIdempotencyKey()).thenReturn(null);
146+
when(wrapper.getJson()).thenReturn("{\"a\":1}");
147+
when(wrapper.getClientId()).thenReturn(null);
148+
when(wrapper.getEntityId()).thenReturn(null);
149+
when(wrapper.getJobName()).thenReturn("LOAN_CLOSE_OF_BUSINESS");
150+
151+
IdempotencyKeyResolver.ResolvedKey result = underTest.resolveWithMeta(wrapper);
163152

153+
Assertions.assertEquals("random-key", result.key());
154+
Assertions.assertFalse(result.isDeterministic());
155+
verifyNoInteractions(deterministicIdempotencyKeyGenerator);
156+
}
157+
158+
@Test
159+
void shouldFallbackToRandomForNonTransferCreate() {
160+
CommandWrapper wrapper = mock(CommandWrapper.class);
161+
when(wrapper.getIdempotencyKey()).thenReturn(null);
162+
when(wrapper.getJson()).thenReturn("{\"name\":\"Test\"}");
163+
when(wrapper.getClientId()).thenReturn(1L);
164+
when(wrapper.getEntityId()).thenReturn(null);
165+
when(wrapper.getActionName()).thenReturn("CREATE");
166+
when(wrapper.getEntityName()).thenReturn("CLIENT");
167+
when(wrapper.getHref()).thenReturn("/clients");
168+
when(wrapper.getJobName()).thenReturn(null);
169+
170+
IdempotencyKeyResolver.ResolvedKey result = underTest.resolveWithMeta(wrapper);
171+
172+
Assertions.assertEquals("random-key", result.key());
173+
Assertions.assertFalse(result.isDeterministic());
174+
verifyNoInteractions(deterministicIdempotencyKeyGenerator);
175+
}
176+
177+
@Test
178+
void shouldFallbackToRandomForNonCreateOperation() {
179+
CommandWrapper wrapper = mock(CommandWrapper.class);
164180
when(wrapper.getIdempotencyKey()).thenReturn(null);
165181
when(wrapper.getJson()).thenReturn("{\"a\":1}");
166182
when(wrapper.getClientId()).thenReturn(1L);
167183
when(wrapper.getEntityId()).thenReturn(1L);
168-
when(wrapper.getActionName()).thenReturn("act");
169-
when(wrapper.getEntityName()).thenReturn("ent");
170-
when(wrapper.getHref()).thenReturn("/test");
171-
172-
when(deterministicIdempotencyKeyGenerator.generate(anyString(), anyString())).thenReturn("det-key");
184+
when(wrapper.getActionName()).thenReturn("UPDATE");
185+
when(wrapper.getEntityName()).thenReturn("ACCOUNTTRANSFER");
186+
when(wrapper.getHref()).thenReturn("/accounttransfers/1");
187+
when(wrapper.getJobName()).thenReturn(null);
173188

174189
IdempotencyKeyResolver.ResolvedKey result = underTest.resolveWithMeta(wrapper);
175190

176-
Assertions.assertEquals("det-key", result.key());
177-
Assertions.assertTrue(result.isDeterministic());
191+
Assertions.assertEquals("random-key", result.key());
192+
Assertions.assertFalse(result.isDeterministic());
193+
verifyNoInteractions(deterministicIdempotencyKeyGenerator);
178194
}
179195

180196
@Test
181197
void shouldMarkDeterministicFalseForRandomFallback() {
182198
CommandWrapper wrapper = mock(CommandWrapper.class);
183-
184199
when(wrapper.getIdempotencyKey()).thenReturn(null);
185200
when(wrapper.getJson()).thenReturn(null);
186201

@@ -190,38 +205,61 @@ void shouldMarkDeterministicFalseForRandomFallback() {
190205
}
191206

192207
@Test
193-
void shouldUseDeterministicKeyWhenJobNamePresentEvenWithoutClientOrEntity() {
208+
void shouldUseDeterministicKeyForAccountTransfer() {
194209
CommandWrapper wrapper = mock(CommandWrapper.class);
195-
196210
when(wrapper.getIdempotencyKey()).thenReturn(null);
197-
when(wrapper.getJson()).thenReturn("{\"a\":1}");
198-
when(wrapper.getClientId()).thenReturn(null);
211+
when(wrapper.getJson()).thenReturn("{\"fromAccountId\":\"1\",\"toAccountId\":\"2\",\"transferAmount\":\"500\"}");
212+
when(wrapper.getClientId()).thenReturn(1L);
199213
when(wrapper.getEntityId()).thenReturn(null);
200-
when(wrapper.getJobName()).thenReturn("LOAN_CLOSE_OF_BUSINESS");
201-
when(wrapper.getActionName()).thenReturn("UPDATE");
202-
when(wrapper.getEntityName()).thenReturn("BATCH_BUSINESS_STEP");
203-
when(wrapper.getHref()).thenReturn("/jobs/LOAN_CLOSE_OF_BUSINESS/steps");
204-
205-
when(deterministicIdempotencyKeyGenerator.generate(anyString(), anyString())).thenReturn("job-key");
214+
when(wrapper.getActionName()).thenReturn("CREATE");
215+
when(wrapper.getEntityName()).thenReturn("ACCOUNTTRANSFER");
216+
when(wrapper.getHref()).thenReturn("/accounttransfers");
217+
when(wrapper.getJobName()).thenReturn(null);
218+
when(deterministicIdempotencyKeyGenerator.generate(anyString(), anyString())).thenReturn("transfer-det-key");
206219

207220
IdempotencyKeyResolver.ResolvedKey result = underTest.resolveWithMeta(wrapper);
208221

209-
Assertions.assertEquals("job-key", result.key());
222+
Assertions.assertEquals("transfer-det-key", result.key());
210223
Assertions.assertTrue(result.isDeterministic());
211-
verify(deterministicIdempotencyKeyGenerator).generate(anyString(), contains("LOAN_CLOSE_OF_BUSINESS"));
212-
// Verify the key is stored in context for internal retry reuse
213-
Assertions.assertEquals("job-key",
214-
fineractRequestContextHolder.getAttribute(SynchronousCommandProcessingService.IDEMPOTENCY_KEY_ATTRIBUTE));
224+
verify(deterministicIdempotencyKeyGenerator).generate(anyString(), anyString());
225+
}
226+
227+
@Test
228+
void shouldMarkDeterministicTrueForAccountTransfer() {
229+
CommandWrapper wrapper = mock(CommandWrapper.class);
230+
when(wrapper.getIdempotencyKey()).thenReturn(null);
231+
when(wrapper.getJson()).thenReturn("{\"fromAccountId\":\"1\",\"toAccountId\":\"2\",\"transferAmount\":\"100\"}");
232+
when(wrapper.getClientId()).thenReturn(1L);
233+
when(wrapper.getEntityId()).thenReturn(null);
234+
when(wrapper.getActionName()).thenReturn("CREATE");
235+
when(wrapper.getEntityName()).thenReturn("ACCOUNTTRANSFER");
236+
when(wrapper.getHref()).thenReturn("/accounttransfers");
237+
when(wrapper.getJobName()).thenReturn(null);
238+
when(deterministicIdempotencyKeyGenerator.generate(anyString(), anyString())).thenReturn("det-key");
239+
240+
IdempotencyKeyResolver.ResolvedKey result = underTest.resolveWithMeta(wrapper);
215241

242+
Assertions.assertEquals("det-key", result.key());
243+
Assertions.assertTrue(result.isDeterministic());
216244
}
217245

218246
@Test
219-
public void testIPKResolveFromWrapper() {
220-
String idk = "idk";
221-
CommandWrapper wrapper = new CommandWrapper(null, null, null, null, null, null, null, null, null, null, null, null, null, null,
222-
null, null, null, idk, null, null);
223-
String resolvedIdk = underTest.resolve(wrapper);
224-
Assertions.assertEquals(idk, resolvedIdk);
225-
verifyNoInteractions(deterministicIdempotencyKeyGenerator);
247+
void shouldReturnSameDeterministicKeyForRetryOfSameTransfer() {
248+
CommandWrapper wrapper = mock(CommandWrapper.class);
249+
when(wrapper.getIdempotencyKey()).thenReturn(null);
250+
when(wrapper.getJson()).thenReturn("{\"fromAccountId\":\"1\",\"toAccountId\":\"2\",\"transferAmount\":\"500\"}");
251+
when(wrapper.getClientId()).thenReturn(1L);
252+
when(wrapper.getEntityId()).thenReturn(null);
253+
when(wrapper.getActionName()).thenReturn("CREATE");
254+
when(wrapper.getEntityName()).thenReturn("ACCOUNTTRANSFER");
255+
when(wrapper.getHref()).thenReturn("/accounttransfers");
256+
when(wrapper.getJobName()).thenReturn(null);
257+
when(deterministicIdempotencyKeyGenerator.generate(anyString(), anyString())).thenReturn("same-key");
258+
259+
String key1 = underTest.resolve(wrapper);
260+
fineractRequestContextHolder.setAttribute(SynchronousCommandProcessingService.IDEMPOTENCY_KEY_ATTRIBUTE, null);
261+
String key2 = underTest.resolve(wrapper);
262+
263+
Assertions.assertEquals(key1, key2);
226264
}
227265
}

0 commit comments

Comments
 (0)