feat : use testcontainers in dev mode (#6)#2337
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request standardizes Testcontainers usage across multiple modules by introducing centralized configuration classes and leveraging @Serviceconnection for simplified property binding. It also implements missing update and delete functionality in the boot-start-routes module and adds database timezone support to the R2DBC configuration. Review feedback identifies the need to use specific version tags for Docker images instead of the latest tag to ensure build reproducibility. Additionally, a concurrency issue was found in the boot-start-routes repository implementation where the update logic is non-atomic and prone to race conditions.
| @Bean | ||
| @ServiceConnection(name = "redis") | ||
| GenericContainer<?> redisContainer() { | ||
| return new GenericContainer<>(DockerImageName.parse("redis:latest")).withExposedPorts(6379); |
There was a problem hiding this comment.
Using the latest tag for Docker images can lead to non-reproducible builds and unexpected failures when the image is updated. It is recommended to use a specific version tag (e.g., redis:7.2-alpine).
| return new GenericContainer<>(DockerImageName.parse("redis:latest")).withExposedPorts(6379); | |
| return new GenericContainer<>(DockerImageName.parse("redis:7.2-alpine")).withExposedPorts(6379); |
| @Bean | ||
| @ServiceConnection(name = "cassandra") | ||
| CassandraContainer cassandraContainer() { | ||
| return new CassandraContainer("cassandra") |
There was a problem hiding this comment.
It is recommended to specify a fixed version for the Cassandra image instead of relying on the default latest tag to ensure build reproducibility and stability across different environments.
| return new CassandraContainer("cassandra") | |
| return new CassandraContainer("cassandra:4.1") |
| Mono<Post> update(UUID id, Post post) { | ||
| return Mono.justOrEmpty(DATA.stream().filter(p -> p.getId().equals(id)).findFirst()) | ||
| .flatMap(existing -> { | ||
| Post updated = Post.builder() | ||
| .id(id) | ||
| .title(post.getTitle()) | ||
| .content(post.getContent()) | ||
| .build(); | ||
| int index = DATA.indexOf(existing); | ||
| if (index >= 0) { | ||
| DATA.set(index, updated); | ||
| } | ||
| return Mono.just(updated); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The update method implementation in PostRepository is not atomic and contains a race condition. Between finding the existing post and calling indexOf/set, the list could have been modified by another thread. Furthermore, since Post uses @Data, indexOf(existing) relies on an equality check of all fields; if another thread updates any field of that post in the meantime, indexOf will return -1, causing the update to fail silently. For a repository implementation, using a ConcurrentMap<UUID, Post> would be more robust and provide atomic update operations.
* Upgrade missing module to Springboot 4.0.6 - removes unnecessary nbactions.xml * implement code review comment
|
Hi @hantsy , Can you please check and provide your feedback |
1 participant
New Features
Tests
Chores