Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat : switch from sql to mongodb for event publication storage #389

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

rajadilipkolli
Copy link
Owner

@rajadilipkolli rajadilipkolli commented Nov 16, 2024

Summary by CodeRabbit

  • New Features

    • Introduced a new mongo-express service for easier MongoDB management, accessible via port 8081.
  • Configuration Changes

    • Updated MongoDB service settings, reinstating the restart policy and retaining health check parameters.
    • Added HikariCP connection pooling configuration to enhance database connection management.
  • Dependency Updates

    • Shifted from JPA to MongoDB for data persistence.
    • Added Grafana dependency for improved testing capabilities.
  • Property Additions

    • New properties for bean management and event handling introduced in the application configuration.
  • Database Changes

    • Removed the event_publication table and associated indexes from the database schema.

@rajadilipkolli rajadilipkolli self-assigned this Nov 16, 2024
Copy link
Contributor

coderabbitai bot commented Nov 16, 2024

Walkthrough

The changes in this pull request involve modifications to several configuration files related to Docker services, dependencies, and application properties. Key updates include the addition of a new mongo-express service in the docker-compose.yml, a shift from JPA to MongoDB in the pom.xml, and enhancements to connection pooling in application-local.yml. Additionally, new properties for bean management and event handling were introduced in application.properties, while an SQL migration file defining the event_publication table was removed. These changes collectively reflect a transition towards MongoDB and improved service configurations.

Changes

File Change Summary
docker/docker-compose.yml - Added mongo-express service with specific environment variables and port configuration.
- Updated grafanalgtm service: ENABLE_LOGS_ALL changed from true to false.
- Modified mongodb service: reinstated restart policy as unless-stopped and retained healthcheck parameters.
- Updated pgadmin and postgresqldb services to include restart policy as unless-stopped.
pom.xml - Updated dependency from spring-modulith-starter-jpa to spring-modulith-starter-mongodb.
- Added new test dependency for grafana.
src/main/resources/application-local.yml - Added HikariCP configuration: hikari.maximum-pool-size set to 25.
src/main/resources/application.properties - Added properties: spring.main.allow-bean-definition-overriding=true, spring.modulith.events.completion-mode=archive, and spring.data.mongodb.repositories.type=none.
src/main/resources/db/changelog/migration/11-event-publication.sql - Removed event_publication table definition and related indexes.
src/test/java/com/app/folioman/common/NoSQLContainersConfig.java - Added method lgtmContainer() to define a new bean for LgtmStackContainer.

Possibly related PRs

🐰 In a world of code and data streams,
I hop along with joyful dreams.
Mongo's here, and Grafana too,
With mongo-express, our tasks are new!
Hikari pools, oh what a sight,
In the land of logs, we take flight! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (6)
src/main/resources/application-local.yml (1)

7-8: Review the high connection pool size

The maximum pool size of 100 seems quite high for a default configuration. High pool sizes can lead to resource exhaustion and might mask connection leaks.

Consider:

  1. Starting with a lower value (e.g., 10-20) and adjusting based on metrics
  2. Documenting the justification for this pool size if it's based on specific performance requirements
src/test/java/com/app/folioman/common/NoSQLContainersConfig.java (1)

Line range hint 1-33: Consider documenting container configuration

Since this class is crucial for test infrastructure and now manages multiple containers (MongoDB, Redis, and LGTM stack), it would be beneficial to add class-level documentation explaining:

  1. The purpose of each container
  2. Any specific configuration requirements
  3. How these containers interact in the test environment

Add a class-level Javadoc:

+/**
+ * Test configuration for NoSQL and observability containers.
+ * <p>
+ * Manages the following test containers:
+ * <ul>
+ *   <li>Redis Stack - For caching and data structure operations</li>
+ *   <li>MongoDB - Primary data store for event publications</li>
+ *   <li>LGTM Stack - Observability stack (Loki, Grafana, Tempo, Mimir)</li>
+ * </ul>
+ * All containers are configured with service connection support for automatic
+ * property binding in the test context.
+ */
 @TestConfiguration(proxyBeanMethods = false)
 public class NoSQLContainersConfig {
docker/docker-compose.yml (2)

Line range hint 73-85: Enhance MongoDB security and production readiness

Several critical configurations are missing:

  1. Authentication is not enabled - MongoDB is accessible without credentials
  2. Network isolation was removed - the service is accessible from any network
  3. Single-node replica set configuration is not suitable for production

Consider applying these security enhancements:

 mongodb:
   hostname: mongodb
   container_name: mongodb
   image: mongo:8.0.3
+  environment:
+    - MONGO_INITDB_ROOT_USERNAME=admin
+    - MONGO_INITDB_ROOT_PASSWORD=secret
   volumes:
     - mongodb:/data/db
   ports:
     - "27017:27017"
+  networks:
+    - folioman-network
   healthcheck:
     test: |
-      mongosh --eval "try { rs.status().ok } catch (e) { rs.initiate({ _id: 'rs0', members: [{ _id: 0, host: 'localhost:27017' }] }).ok }"
+      mongosh -u admin -p secret --eval "try { rs.status().ok } catch (e) { rs.initiate().ok }"
     interval: 10s
     start_period: 30s
-  command: [ "/usr/bin/mongod", "--replSet", "rs0", "--bind_ip_all" ]
+  command: [ "/usr/bin/mongod", "--replSet", "rs0", "--bind_ip_all", "--auth" ]

Line range hint 1-105: Document the database migration strategy

The compose file maintains both PostgreSQL and MongoDB services. To avoid confusion:

  1. Add comments explaining the ongoing migration from PostgreSQL to MongoDB
  2. Document which services are being deprecated
  3. Consider creating separate compose files for different environments (e.g., development, migration, production)

Would you like assistance in creating separate compose files or documentation for the migration strategy?

src/main/resources/application.properties (1)

Line range hint 25-40: Clean up SQL configurations and enhance MongoDB settings

Since we're switching to MongoDB for event storage:

  1. Consider removing or commenting out unused JPA configurations
  2. Add recommended MongoDB configurations for optimal performance:
    • Connection pool settings
    • Write concern
    • Read preference
    • Retry writes
- spring.jpa.show-sql=false
- spring.jpa.open-in-view=false
- spring.data.jpa.repositories.bootstrap-mode=deferred
- spring.datasource.hikari.auto-commit=false
- # ... remove other JPA configurations if not needed

+ # MongoDB configurations
+ spring.data.mongodb.connection-pool.max-size=50
+ spring.data.mongodb.connection-pool.min-size=5
+ spring.data.mongodb.write-concern=MAJORITY
+ spring.data.mongodb.read-preference=PRIMARY_PREFERRED
+ spring.data.mongodb.retry-writes=true

Also applies to: 61-62

pom.xml (1)

Spring Boot 3.4.0-RC1 and Spring Modulith 1.3.0-RC1 should be replaced with stable versions

The project is using release candidate versions which are not recommended for production:

  • Spring Boot 3.4.0-RC1 (latest stable version is 3.2.x)
  • Spring Modulith 1.3.0-RC1 (latest stable version is 1.2.5)

Consider:

  1. Downgrading to Spring Boot 3.2.x and Spring Modulith 1.2.5 for production stability
  2. If specific RC features are required, document the dependencies and create a tracking issue for upgrading to stable versions once released
🔗 Analysis chain

Line range hint 27-29: Consider using stable versions for production

The project is using Spring Boot 3.4.0-RC1 and spring-modulith 1.3.0-RC1. While these versions provide the latest features, they might not be suitable for production use.

Let's check for any known issues:

Consider:

  1. Using the latest stable versions for production
  2. Creating an issue to track the upgrade to stable versions once released
  3. Documenting any specific features requiring these RC versions

Let me gather more information about the specific versions used in the project.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for stable versions and known issues

# Check Spring Boot milestones page for latest stable version
curl -s https://spring.io/projects/spring-boot#learn | grep -o "Current Version: [0-9.]*"

# Check Spring Modulith releases
curl -s "https://repo1.maven.org/maven2/org/springframework/modulith/spring-modulith-bom/maven-metadata.xml" | grep -o "<version>[^<]*</version>" | tail -n 5

Length of output: 374


Script:

#!/bin/bash
# Check the actual Spring Boot and Spring Modulith versions in pom.xml
echo "Spring Boot version:"
grep -A 1 "<spring-boot.version>" pom.xml || echo "Not found in parent"
grep -A 1 "<version.spring-boot>" pom.xml || echo "Not found in properties"

echo -e "\nSpring Modulith version:"
grep -A 1 "<spring-modulith.version>" pom.xml || echo "Not found"

echo -e "\nOther RC/milestone dependencies:"
grep -B 1 -A 1 "\-RC\|\-M" pom.xml || echo "No RC/milestone versions found"

Length of output: 897

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8177a79 and 29bbb44.

📒 Files selected for processing (6)
  • docker/docker-compose.yml (2 hunks)
  • pom.xml (2 hunks)
  • src/main/resources/application-local.yml (1 hunks)
  • src/main/resources/application.properties (2 hunks)
  • src/main/resources/db/changelog/migration/11-event-publication.sql (0 hunks)
  • src/test/java/com/app/folioman/common/NoSQLContainersConfig.java (2 hunks)
💤 Files with no reviewable changes (1)
  • src/main/resources/db/changelog/migration/11-event-publication.sql
🔇 Additional comments (8)
src/main/resources/application-local.yml (1)

Line range hint 1-6: Verify if PostgreSQL configuration should be retained

The PR objective indicates switching from SQL to MongoDB for event publication storage, but the PostgreSQL datasource configuration is still present. Please clarify if:

  1. This is intentional (hybrid approach where both SQL and MongoDB are needed)
  2. Or if this configuration should be removed as part of the MongoDB migration

Let's check if there are any remaining SQL dependencies or usage:

✅ Verification successful

The results show significant SQL/JPA usage across the codebase, particularly in the mutual fund schemes and portfolio modules. Let's verify if these entities are still actively used by checking for their repository implementations and service layer usage:


PostgreSQL configuration must be retained - Only event publication is moving to MongoDB

The verification reveals that PostgreSQL is actively used throughout the application:

  • Multiple JPA entities and repositories exist for core business functionality (mutual funds, portfolios)
  • Active service layer implementations depend on these SQL-based repositories
  • Only the event publication storage is being migrated to MongoDB, not the entire application

The PostgreSQL datasource configuration in application-local.yml is required and should be retained for the hybrid SQL-MongoDB approach.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for SQL-related dependencies in pom.xml
echo "Checking for SQL/JPA dependencies..."
rg -A 5 "postgresql|jdbc|jpa|hibernate" pom.xml

# Check for remaining SQL/JPA annotations or imports
echo "Checking for SQL/JPA usage in code..."
rg -l "javax.persistence|jakarta.persistence|@Entity|@Table|@Column" src/

Length of output: 1992


Script:

#!/bin/bash
# Check for JPA repository usage
echo "Checking for JPA repository implementations..."
rg -l "extends.*JpaRepository|extends.*CrudRepository" src/

# Check for entity usage in services
echo "Checking for entity usage in service layer..."
rg -l "MfFundScheme|UserPortfolioValue|UserSchemeDetails|UserFolioDetails" src/main/java/com/app/folioman/*/service/

Length of output: 2107

src/test/java/com/app/folioman/common/NoSQLContainersConfig.java (2)

4-4: LGTM: Clean import additions

The new imports for Duration and LgtmStackContainer are appropriately placed and necessary for the new functionality.

Also applies to: 9-9


28-33: Verify LGTM stack container version and configuration

The implementation looks good, but there are a few points to consider:

  1. The LGTM stack container version is pinned to 0.7.8. Let's verify if this is the latest stable version.
  2. The 2-minute startup timeout seems reasonable, but we should verify if this is sufficient in CI environments.

Consider adding the following to improve the container configuration:

  1. Enable container reuse like the Redis container to speed up tests
  2. Add health checks to ensure services are ready
  3. Consider exposing metrics ports if needed for monitoring during tests

Example enhancement:

 @Bean
 @ServiceConnection
 LgtmStackContainer lgtmContainer() {
     return new LgtmStackContainer(DockerImageName.parse("grafana/otel-lgtm:0.7.8"))
-            .withStartupTimeout(Duration.ofMinutes(2));
+            .withStartupTimeout(Duration.ofMinutes(2))
+            .withReuse(true)
+            .waitingFor(Wait.forHealthcheck());
 }
✅ Verification successful

LGTM stack container version is up-to-date, timeout configuration appears reasonable

The version 0.7.8 used in the code matches the latest stable version available in the Docker Hub registry (excluding the 'latest' tag). No timeout-related issues were found in the codebase, suggesting the 2-minute timeout is working well.

Note: I've removed the architecture advice from the original review as it was speculative without evidence of need in the codebase. The current implementation with basic timeout configuration appears sufficient based on the verification results.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for the latest LGTM stack container version and typical startup times

# Check latest version
echo "Checking latest grafana/otel-lgtm versions..."
curl -s "https://registry.hub.docker.com/v2/repositories/grafana/otel-lgtm/tags?page_size=5" | jq -r '.results[].name'

# Look for any timeout related issues in the codebase
echo "Checking for any timeout related issues in test logs..."
rg -i "timeout.*lgtm|lgtm.*timeout" 

Length of output: 411

docker/docker-compose.yml (1)

66-66: Verify logging requirements during migration

Reducing log verbosity by setting ENABLE_LOGS_ALL=false might impact observability during the SQL to MongoDB migration phase. Consider maintaining detailed logging until the migration is stable.

src/main/resources/application.properties (2)

20-20: Verify event archival configuration

The archive completion mode is appropriate for persistent event storage in MongoDB. However, ensure that:

  1. Event cleanup/retention policies are properly configured
  2. MongoDB indexes are created for efficient event querying
#!/bin/bash
# Description: Search for event-related configurations and indexes

# Search for event-related configurations
echo "Searching for event configurations..."
rg -g "*.properties" -g "*.yml" -e "spring.modulith.events" -e "mongodb.events"

# Search for MongoDB index definitions
echo "Searching for MongoDB index definitions..."
rg -g "*.java" -e "@Index" -e "createIndex" -e "CompoundIndex"

# Search for event cleanup/retention policies
echo "Searching for cleanup/retention configurations..."
rg -g "*.java" -g "*.properties" -g "*.yml" -e "retention" -e "cleanup" -e "TTL"

4-4: Verify if bean definition overriding is necessary

Enabling bean definition overriding can lead to unexpected behavior and is generally considered a code smell. Consider identifying and resolving the bean conflicts that necessitate this setting.

✅ Verification successful

Let me gather more specific information about the database configuration changes.


Based on the search results, I can now provide a final response. The codebase shows a transition in progress from JPA/SQL to MongoDB, with evidence of:

  1. Multiple JPA repositories still in use
  2. JPA auditing configuration still active
  3. SQL-specific configurations present (LazyConnectionDataSourceProxy, FlexyPool)
  4. No MongoDB configurations or repositories found yet

Bean definition overriding is necessary during this transition phase

The spring.main.allow-bean-definition-overriding=true setting is justified as it's needed to handle the coexistence of both SQL and MongoDB configurations during the migration. This allows Spring to properly manage potential bean conflicts between:

  • JPA/Hibernate auto-configurations
  • MongoDB auto-configurations
  • Custom database connection management beans (LazyConnectionDataSourceProxy, FlexyPool)

Keep this setting until the migration to MongoDB is complete and all SQL-related configurations are removed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for potential bean conflicts in the codebase
# Look for multiple bean definitions that might be causing conflicts

# Search for @Bean annotations
echo "Searching for @Bean definitions..."
rg "@Bean" -A 5

# Search for @Component, @Service, @Repository annotations
echo "Searching for Spring stereotype annotations..."
rg -e "@Component" -e "@Service" -e "@Repository" -A 5

# Search for MongoDB and SQL related configurations
echo "Searching for database configurations..."
rg -e "MongoDB" -e "JPA" -e "Repository" -A 5

Length of output: 4915


Script:

#!/bin/bash
# Search for MongoDB and SQL related configuration classes
echo "Searching for database configuration classes..."
rg -g "*.java" -e "MongoConfig" -e "DataSource" -e "@EnableMongo" -e "@EnableJpa" -A 10

# Search for repository implementations
echo "Searching for repository implementations..."
rg -g "*.java" -e "extends MongoRepository" -e "extends JpaRepository" -A 3

# Look for potential bean conflicts in configuration
echo "Searching for configuration beans..."
ast-grep --pattern 'class $_ {
  @Bean
  $_ $_($$$) {
    $$$
  }
}'

Length of output: 27933

pom.xml (2)

209-213: Clarify the purpose of adding Grafana test container

The addition of the Grafana test container appears unrelated to the MongoDB migration. Given the existing observability stack (micrometer, prometheus), please clarify:

Let's check the test usage:

Consider:

  1. Documenting the test scenarios requiring Grafana
  2. Ensuring proper integration with existing observability setup
  3. Adding relevant Grafana dashboard configurations if missing
✅ Verification successful

Grafana test container is part of observability testing infrastructure

The Grafana test container is properly integrated as part of the LGTM (Loki, Grafana, Tempo, Mimir) observability stack:

  • Used in NoSQLContainersConfig as LgtmStackContainer for testing
  • Matches the production setup in docker-compose.yml using the same grafana/otel-lgtm:0.7.8 image
  • Integrated with application metrics configuration (management.metrics.tags)

The addition is not related to MongoDB migration but rather supports observability testing with the existing monitoring infrastructure.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify Grafana test container usage and configuration

# Look for Grafana test container configuration
echo "Checking Grafana test container usage..."
rg -l "GrafanaContainer|@Container.*Grafana"

# Check for Grafana dashboards or configuration
echo "Checking for Grafana configuration files..."
fd -e json -e yaml -e yml | rg -l "grafana|dashboard"

Length of output: 388


Script:

#!/bin/bash
# Let's try a broader search for Grafana-related code and tests

# Search for Grafana-related imports and usage
echo "Searching for Grafana-related code..."
rg -i "grafana" -A 5 -B 5

# Search for test files that might use Grafana
echo "Searching for test files with Grafana..."
fd -e java -e kt --full-path ".*[Tt]est.*" | xargs rg -i "grafana"

# Look for monitoring/metrics configuration
echo "Checking monitoring configuration..."
rg -i "metrics|prometheus|monitoring" -g "application*.{yml,yaml,properties}"

Length of output: 4117


140-140: Verify the coexistence strategy of JPA and MongoDB

The change from spring-modulith-starter-jpa to spring-modulith-starter-mongodb aligns with the PR objective. However, I notice that both JPA and MongoDB dependencies are present in the project.

Let's verify the usage patterns to ensure clean separation:

Consider:

  1. Clearly documenting the migration strategy
  2. Using separate packages for JPA and MongoDB repositories
  3. Planning the complete migration timeline if this is a temporary state
✅ Verification successful

MongoDB is only used for event publication storage, while core business logic remains in PostgreSQL

Based on the verification results, I can confirm that:

  1. The application maintains a clean separation between JPA and MongoDB usage:
    • All business entities (MfFundScheme, UserTransactionDetails, etc.) use JPA/PostgreSQL
    • No mixed usage of @document and @entity annotations was found
    • MongoDB is configured specifically for the jobrunr database, which aligns with event publication storage
  2. The change to spring-modulith-starter-mongodb is appropriate as it only affects the event publication infrastructure while preserving the existing JPA-based domain model.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential conflicts in repository/entity usage

# Look for JPA and MongoDB annotations in the same files
echo "Checking for mixed persistence annotations..."
rg -l "@Entity|@Table" | xargs rg -l "@Document|@Collection"

# Look for Repository interfaces extending both JPA and Mongo repositories
echo "Checking for mixed repository types..."
rg -l "extends.*JpaRepository|extends.*CrudRepository" | xargs rg -l "extends.*MongoRepository"

Length of output: 388


Script:

#!/bin/bash
# Let's verify the persistence setup more thoroughly

# Check for JPA entities
echo "=== JPA Entity Usage ==="
rg "@Entity" -A 5

# Check for MongoDB documents
echo "=== MongoDB Document Usage ==="
rg "@Document" -A 5

# Check for repository implementations
echo "=== Repository Implementations ==="
rg "extends.*Repository" -A 2

# Check for persistence configuration
echo "=== Persistence Configuration ==="
fd -e java -e properties -e yml | xargs rg "spring\.(jpa|data\.mongodb)"

Length of output: 15708

Comment on lines 88 to 100
mongo-express:
image: mongo-express
container_name: mongo-express
ports:
- "8081:8081"
environment:
# - ME_CONFIG_MONGODB_ADMINUSERNAME=admin
# - ME_CONFIG_MONGODB_ADMINPASSWORD=passcode
- ME_CONFIG_MONGODB_SERVER=mongodb
- ME_CONFIG_BASICAUTH_USERNAME=admin
- ME_CONFIG_BASICAUTH_PASSWORD=pass
- ME_CONFIG_MONGODB_ENABLE_ADMIN=true
restart: unless-stopped
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix security vulnerabilities in mongo-express configuration

Several security and configuration issues need attention:

  1. MongoDB connection lacks authentication
  2. Basic auth credentials are in plaintext
  3. Missing dependency on MongoDB service
  4. Network isolation not configured
  5. Commented configuration suggests incomplete setup

Apply these security fixes:

   mongo-express:
     image: mongo-express
     container_name: mongo-express
     ports:
       - "8081:8081"
     environment:
-      #      - ME_CONFIG_MONGODB_ADMINUSERNAME=admin
-      #      - ME_CONFIG_MONGODB_ADMINPASSWORD=passcode
+      - ME_CONFIG_MONGODB_ADMINUSERNAME=${MONGO_ADMIN_USERNAME:-admin}
+      - ME_CONFIG_MONGODB_ADMINPASSWORD=${MONGO_ADMIN_PASSWORD:-secret}
       - ME_CONFIG_MONGODB_SERVER=mongodb
-      - ME_CONFIG_BASICAUTH_USERNAME=admin
-      - ME_CONFIG_BASICAUTH_PASSWORD=pass
+      - ME_CONFIG_BASICAUTH_USERNAME=${ME_BASIC_AUTH_USERNAME}
+      - ME_CONFIG_BASICAUTH_PASSWORD=${ME_BASIC_AUTH_PASSWORD}
       - ME_CONFIG_MONGODB_ENABLE_ADMIN=true
     restart: unless-stopped
+    networks:
+      - folioman-network
+    depends_on:
+      mongodb:
+        condition: service_healthy

Create a .env file for sensitive credentials:

MONGO_ADMIN_USERNAME=admin
MONGO_ADMIN_PASSWORD=secret
ME_BASIC_AUTH_USERNAME=admin
ME_BASIC_AUTH_PASSWORD=strong-password

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
pom.xml (1)

Line range hint 58-77: Architecture Consideration: Dual Database Configuration

The presence of both JPA and MongoDB starters suggests a hybrid database approach. While this might be intentional for a gradual migration, consider:

  1. The impact on application complexity
  2. Potential maintenance overhead
  3. Transaction management across different databases

Consider documenting:

  • Which parts of the application use which database
  • The migration strategy and timeline
  • Transaction boundaries and data consistency requirements

Also applies to: 140-140

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 75e4487 and a868411.

📒 Files selected for processing (2)
  • docker/docker-compose.yml (3 hunks)
  • pom.xml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • docker/docker-compose.yml
🔇 Additional comments (3)
pom.xml (3)

140-140: LGTM: MongoDB starter change aligns with migration objective

The change from spring-modulith-starter-jpa to spring-modulith-starter-mongodb aligns with the PR objective of switching to MongoDB for event publication storage.


209-213: LGTM: Added Grafana test container support

Adding Grafana test container support will help with monitoring and observability during testing.


Line range hint 58-77: Verify if JPA dependencies are still needed

The project still includes JPA-related dependencies (spring-boot-starter-data-jpa, liquibase-core) alongside MongoDB. If the migration to MongoDB is complete for event publication storage, consider:

  1. Whether other parts of the application still require JPA
  2. If not, these dependencies could be removed

Let's check if JPA is still being used elsewhere in the codebase:

✅ Verification successful

JPA and Liquibase dependencies are actively used and required

The codebase shows extensive usage of JPA:

  • 13 entity classes using JPA annotations in mfschemes and portfolio packages
  • 11 JPA repositories extending JpaRepository
  • Active database migration setup with 15 Liquibase changelog files managing schema evolution

These dependencies are essential for the application's core functionality and should be retained.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for JPA annotations and repository usage
echo "Searching for JPA usage..."
rg -l "javax.persistence|jakarta.persistence|extends.*JpaRepository|@Entity|@Table" 

# Search for Liquibase changelog files
echo "Searching for Liquibase changelogs..."
fd -e yaml -e xml -e sql | rg -i "changelog|liquibase"

Length of output: 3183

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
docker/docker-compose.yml (1)

94-111: ⚠️ Potential issue

Fix security vulnerabilities in mongo-express configuration

The security concerns from the previous review are still valid.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a868411 and ad8aa0a.

📒 Files selected for processing (3)
  • docker/docker-compose.yml (3 hunks)
  • pom.xml (2 hunks)
  • src/test/java/com/app/folioman/common/NoSQLContainersConfig.java (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/test/java/com/app/folioman/common/NoSQLContainersConfig.java
🧰 Additional context used
🪛 yamllint (1.35.1)
docker/docker-compose.yml

[error] 82-82: duplication of key "restart" in mapping

(key-duplicates)

🔇 Additional comments (2)
docker/docker-compose.yml (1)

66-66: Verify impact of reduced logging

Disabling all logs might affect debugging capabilities in case of issues.

✅ Verification successful

Setting ENABLE_LOGS_ALL=false is safe for production

The change to disable all logs in the Grafana service is safe because:

  • The application's logging is configured independently through logback-spring.xml with appropriate log levels
  • Debug logs are enabled for the main application package (com.app.folioman)
  • Critical logging for Spring, database queries, and other components is properly configured
  • The ENABLE_LOGS_ALL variable only affects the Grafana service container and doesn't impact the application's logging capabilities
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there are any specific log configurations in the application
rg -g '*.properties' -g '*.yml' 'logging.level|ENABLE_LOGS'

Length of output: 196


Script:

#!/bin/bash
# Check for any references to ENABLE_LOGS_ALL in the codebase
rg "ENABLE_LOGS_ALL" -B 2 -A 2

# Check for any logging configuration files or patterns
fd -e yaml -e yml -e properties -e xml | xargs grep -l "log" 2>/dev/null

# Look for any logging setup in application code
ast-grep --pattern 'logging.$_'

Length of output: 1459


Script:

#!/bin/bash
# Check the logging configuration in logback files
cat src/main/resources/logback-spring.xml
cat src/test/resources/logback-test.xml

# Check if ENABLE_LOGS_ALL is used in any other docker or configuration files
fd -e dockerfile -e yml -e yaml -e conf | xargs grep -l "ENABLE_LOGS" 2>/dev/null

Length of output: 1885

pom.xml (1)

209-213: LGTM! Grafana test container dependency properly configured

The dependency is correctly scoped for testing and aligns with the existing observability setup.

@@ -79,6 +79,7 @@ services:
- mongodb:/data/db
ports:
- "27017:27017"
restart: unless-stopped
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove duplicate restart policy

The restart: unless-stopped configuration is duplicated for the mongodb service.

Apply this fix:

  mongodb:
    hostname: mongodb
    container_name: mongodb
    image: mongo:8.0.4
    restart: unless-stopped
    volumes:
      - mongodb:/data/db
    ports:
      - "27017:27017"
-   restart: unless-stopped
    networks:
      - folioman-network
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
restart: unless-stopped
restart: unless-stopped
volumes:
- mongodb:/data/db
ports:
- "27017:27017"
networks:
- folioman-network
🧰 Tools
🪛 yamllint (1.35.1)

[error] 82-82: duplication of key "restart" in mapping

(key-duplicates)

@@ -137,7 +137,7 @@
</dependency>
<dependency>
<groupId>org.springframework.modulith</groupId>
<artifactId>spring-modulith-starter-jpa</artifactId>
<artifactId>spring-modulith-starter-mongodb</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Migration to MongoDB is incomplete - JPA dependencies and entities still present

The codebase still heavily relies on JPA/Hibernate with multiple entity classes using Jakarta Persistence annotations:

  • 12 JPA entity classes found with @Entity annotations
  • Extensive usage of JPA annotations like @Table, @Column, @Id, etc.
  • No MongoDB annotations (@Document) or repositories found

The change of dependency to spring-modulith-starter-mongodb appears premature as the codebase is still using JPA/Hibernate for persistence.

🔗 Analysis chain

Verify complete migration from JPA to MongoDB

While the Spring Modulith starter has been changed to MongoDB, there might be remaining JPA dependencies or code that needs to be updated.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any remaining JPA annotations or repositories
ast-grep --pattern 'import javax.persistence.$_'
ast-grep --pattern 'import jakarta.persistence.$_'
ast-grep --pattern '@Entity'
ast-grep --pattern 'extends JpaRepository'

# Check for any MongoDB related code to ensure proper migration
ast-grep --pattern 'import org.springframework.data.mongodb.$_'
ast-grep --pattern '@Document'
ast-grep --pattern 'extends MongoRepository'

Length of output: 16289

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant