Fix/concurrent requests

[burl21]

This PR fixes an issue where concurrency was not correctly handled when requests shared the same idempotency key. Previously, both requests were executing the business logic because they were treated as new requests; this behavior was present across all store implementations. An upsert was being performed, overwriting the key instead of enforcing a strict insert.

Conflict handling is now managed in the core by catching the exception and, when it occurs, treating the request as already existing.

This PR also adds waitForCompletion handling to IdempotentService, centralizing the logic in a single place for both the aspect and the service.

Additionally, properties have been extracted from IdempotentAspect into a separate class. Unfortunately, this introduced a breaking change (see //FIXME in core.ConfigurationProcessorTest).

I tried to group the changes logically by commit 🙂

[arun0009]

can we fix this?

[arun0009]

Since IdempotentService provides a useful programmatic API, we should probably add it to IdempotentCoreAutoConfiguration so users can easily inject it as a bean.

[burl21]

Hi @arun0009
Thanks. What do you think about the solution based on exceptions? Do you think it’s better to go in this direction, or should we change the store() method so that it returns a boolean instead?

Regarding the breaking change fix, do you mean implementing it now or reverting to the previous behavior? It might make more sense to handle it in a major version and refactor the properties across the modules.

For the IdempotentService, would it make sense to add an overloaded method that accepts a waitStrategy parameter as well?

[arun0009]

Thanks. What do you think about the solution based on exceptions? Do you think it’s better to go in this direction, or should we change the store() method so that it returns a boolean instead?

Regarding the breaking change fix, do you mean implementing it now or reverting to the previous behavior? It might make more sense to handle it in a major version and refactor the properties across the modules.

For the IdempotentService, would it make sense to add an overloaded method that accepts a waitStrategy parameter as well?

1. Exceptions vs. Boolean: Stick with the exception-based logic; it’s more descriptive and fits the Spring context well.
2. Breaking Change: Let’s fix the property naming now by reverting to the original camelCase. This avoids breaking existing user configs without having to wait for a major version.
3. Overloaded execute: Yes, adding a WaitStrategy parameter to the execute methods is a great idea for custom timeout control.
4. Type Safety (Important): Removing operation.get().getClass() fixed the redundant execution, but the raw cast
   (T) will likely cause ClassCastException with stores like NATS/Dynamo?
   • Fix: Add Class responseType to the execute methods so the store knows exactly how to deserialize the cached result safely.

[burl21]

1. Exceptions vs. Boolean: Stick with the exception-based logic; it’s more descriptive and fits the Spring context well.

ok

2. Breaking Change: Let’s fix the property naming now by reverting to the original camelCase. This avoids breaking existing user configs without having to wait for a major version.

ok

3. Overloaded execute: Yes, adding a WaitStrategy parameter to the execute methods is a great idea for custom timeout control.

ok

4. Type Safety (Important): Removing operation.get().getClass() fixed the redundant execution, but the raw cast
   (T) will likely cause ClassCastException with stores like NATS/Dynamo?

   • Fix: Add Class responseType to the execute methods so the store knows exactly how to deserialize the cached result safely.

Now it’s not a problem because the type is saved by Jackson in the various store implementations. See commit: DynamoIdempotentStore for DynamoDB. However, I can still add it if needed.

[arun0009]

@burl21 - you should have access to create branches on this repo now. Its easier for both of us to create PRs/review.

[burl21]

@arun0009
Unfortunately, I need to roll back on a couple of points:

2. Breaking Change: Let’s fix the property naming now by reverting to the original camelCase. This avoids breaking existing user configs without having to wait for a major version.

Currently, it is not a breaking change; in fact, it’s an improvement because the binding supports both camelCase and kebab-case. I added a binding test and removed the .json file because it is now auto-generated by the configuration processor.

3. Overloaded execute: Yes, adding a WaitStrategy parameter to the execute methods is a great idea for custom timeout control.

I’ll add it in a separate PR since it requires changes to both the service and the completion awaiter. For a custom WaitStrategy, it’s already possible to instantiate the service by providing a custom wait strategy.

[burl21]

@arun0009 gentle ping 🙂

[arun0009]

Also we should bump the version to 2.2.0 as part of this PR

[burl21]

@arun0009

• Polish code after review
• Throw error in case of wait timeout in both aspect and service instead of returning null. Because the operation would still be executed multiple times
• Updated to version 2.2.0

[arun0009]

LGTM

[arun0009]

@burl21 : 2.2.0 released successfully! Great work and nicely handled it all in core.