MODGOBI-232. Introduce DB and use it to store custom mappings

[SerhiiNosko]

Purpose

https://folio-org.atlassian.net/browse/MODGOBI-217

Approach

1. Introduce DB
2. Refactor Custom Order mappings to use DB
3. Delete unusefull cache
4. Intorduce test containers for integration tests
5. Migrate to junit 5

---

Pre-Review Checklist

• Self-reviewed Code — Reviewed code for issues, unnecessary parts, and overall quality.
• Change Notes — NEWS.md updated with clear description and issue key.
• Testing — Confirmed changes were tested locally or on dev environment.
• Logging: Confirmed that logging is appropriately handled.
• Breaking Changes — Handled all required actions if changes affect API, DB, or interface versions.
  • API/schema changes
  • Interface version updates
  • DB schema changes / migration scripts
• New Properties / Environment Variables — Updated README.md if new configs were added.

[BKadirkhodjaev]

Please add what model was used

[SerhiiNosko]

It was 3 models, some parts of each what have influence

[BKadirkhodjaev]

I think here you can just pass orderMapping directly without the if-else

[SerhiiNosko]

Code Review for DAO Refactoring - Custom Order Mappings

Overview

This pull request refactors the custom order mappings functionality to use a database-backed DAO pattern instead of calling the mod-configuration module. The changes introduce:

• New OrderMappingsDao interface and implementation for database access
• Removal of configuration module dependencies
• Migration from cached configuration entries to direct database queries
• Comprehensive test updates to use Testcontainers and PostgreSQL
• Integration tests using a more modern testing approach

Files Modified: 20 files
Lines Added: ~2,500
Lines Removed: ~800

---

Suggestions

🔧 Missing @OverRide annotation in DAO implementation

• Priority: 🟡 Medium
• File: src/main/java/org/folio/dao/OrderMappingsDaoImpl.java
• Details: The update method at line 56 is missing the @Override annotation while other methods have it.
• Suggested Change:

@Override
public Future<Void> update(String id, OrderMappings orderMappings, Conn conn) {
  return conn.update(TABLE_NAME, orderMappings, id)
    .onFailure(t -> log.error("update:: Failed to update order mapping with id: {}", id, t))
    .mapEmpty();
}

---

⛏️ Inconsistent error logging message in getByOrderType

• Priority: 🟢 Low
• File: src/main/java/org/folio/dao/OrderMappingsDaoImpl.java
• Details: Line 41 uses "findByOrderType" in the log message but the method is named "getByOrderType". This inconsistency could confuse developers during debugging.
• Suggested Change:

.onFailure(t -> log.error("getByOrderType:: Failed to find order mapping by orderType: {}", orderType, t));

---

🔧 Potential issue with CompletableFuture nesting in DataSourceResolver

• Priority: ⚠️ High
• File: src/main/java/org/folio/gobi/DataSourceResolver.java
• Details: Line 130 changed from thenApply to thenCompose. This is correct for flattening nested CompletableFutures, but there's a logic issue - the method returns CompletableFuture<Object> but applyTranslation returns CompletableFuture<?>. This could lead to type safety issues at runtime.
• Rationale: The fix correctly addresses the nested future problem, but consider adding explicit typing or casting to ensure type safety.
• Suggested Change:

if (translateDefValue) {
  return ((DataSourceResolver) defValue).resolve(doc)
    .thenCompose(v -> applyTranslation(v != null ? v.toString() : null));
} else {
  return ((DataSourceResolver) defValue).resolve(doc);
}

---

♻️ Unused imports in MappingHelper

• Priority: 🟢 Low
• File: src/main/java/org/folio/gobi/MappingHelper.java
• Details: Lines 22-23 removed JsonArray and JsonObject imports which were previously used. Good cleanup, but ensure no other usages were missed.

---

🔧 Missing null-safety check in MappingHelper.extractOrderMappings

• Priority: 🟡 Medium
• File: src/main/java/org/folio/gobi/MappingHelper.java
• Details: Line 105 checks if orderMapping == null and returns empty map, but doesn't check if orderMapping.getMappings() is null before iterating. This could cause NullPointerException.
• Suggested Change:

public Map<Mapping.Field, DataSourceResolver> extractOrderMappings(OrderMappings orderMapping) {
  Map<Mapping.Field, DataSourceResolver> map = new EnumMap<>(Mapping.Field.class);

  if (orderMapping == null || orderMapping.getMappings() == null) {
    return map;
  }
  for (Mapping mapping : orderMapping.getMappings()) {
    map.put(mapping.getField(), getDS(mapping, map));
  }

  return map;
}

---

🌱 Consider caching strategy for default mappings

• Priority: 🟡 Medium
• File: src/main/java/org/folio/rest/impl/PostGobiOrdersHelper.java
• Details: The refactoring removed the OrderMappingCache entirely. While the PR description mentions "cache is not needed", consider that default mappings are loaded from file system resources multiple times. The original code had static initialization for default mappings in MappingHelper which is good, but database queries for custom mappings could benefit from short-lived caching.
• Rationale: Every GOBI order request will trigger a database query even when no custom mapping exists. Consider implementing a TTL-based cache or memoization for frequently accessed default mappings.

---

🔧 Database connection management in GobiOrdersCustomMappingsImpl

• Priority: ⚠️ High
• File: src/main/java/org/folio/rest/impl/GobiOrdersCustomMappingsImpl.java
• Details: Each endpoint creates a new PostgresClient instance and OrderMappingsDaoImpl instance (lines 84-87). This is inefficient and could lead to connection pool exhaustion under load.
• Suggested Change: Consider using dependency injection or creating these instances at class initialization:

public class GobiOrdersCustomMappingsImpl extends BaseApi implements GobiOrdersCustomMappings {
  private final OrderMappingsDao orderMappingsDao = new OrderMappingsDaoImpl();

  private GobiCustomMappingsService getCustomMappingsService(Map<String, String> okapiHeaders, Context vertxContext) {
    String tenantId = TenantTool.tenantId(okapiHeaders);
    PostgresClient pgClient = PostgresClient.getInstance(vertxContext.owner(), tenantId);
    return new GobiCustomMappingsService(pgClient, orderMappingsDao);
  }
}

---

📝 RestClient cleanup removed useful query building methods

• Priority: 🟢 Low
• File: src/main/java/org/folio/rest/core/RestClient.java
• Details: Lines removed include handleGetRequest with query building, buildQueryParam, and encodeQuery methods. While these aren't needed for the current implementation, they might be useful utilities for other parts of the codebase.
• Rationale: If these methods are truly unused, the removal is justified. Otherwise, consider keeping them or documenting why they were removed.

---

🔧 Error handling in updateByOrderType could be more specific

• Priority: 🟡 Medium
• File: src/main/java/org/folio/dao/OrderMappingsDaoImpl.java
• Details: Lines 65-70 throw a generic RuntimeException when no records are found to update. Consider using a more specific exception type or HTTP status code mapping.
• Suggested Change:

@Override
public Future<Void> updateByOrderType(String orderType, OrderMappings orderMappings, Conn conn) {
  return conn.update(TABLE_NAME, orderMappings, getOrderTypeCriterion(orderType), true)
    .compose(updateResult -> {
      if (updateResult.size() == 0) {
        log.warn("updateByOrderType:: No mapping found for orderType {} to update", orderType);
        return Future.failedFuture(
          new HttpException(404, "No matching order mapping found to update for orderType: " + orderType)
        );
      }
      return Future.succeededFuture();
    })
    .onFailure(t -> log.error("updateByOrderType:: Failed to update order mapping for orderType: {}", orderType, t))
    .mapEmpty();
}

---

⛏️ Test class should be package-private

• Priority: 🟢 Low
• File: src/test/java/org/folio/dao/OrderMappingsDaoTest.java
• Details: Line 38 declares the test class as package-private (no modifier), which is good practice for JUnit 5 tests. However, ensure consistency across all test classes.

---

👍 Excellent test coverage for OrderMappingsDao

• Priority: N/A
• File: src/test/java/org/folio/dao/OrderMappingsDaoTest.java
• Details: The new test class provides comprehensive coverage including:
  • Success scenarios
  • Empty results
  • Failure cases
  • Pagination
  • All CRUD operations
• Rationale: This is exemplary test coverage and should be the standard for other DAO implementations.

---

🔧 BaseIntegrationTest has hardcoded module version

• Priority: 🟡 Medium
• File: src/test/java/org/folio/rest/impl/BaseIntegrationTest.java
• Details: Line 64 has "module_to": "mod-gobi-1.0.0" hardcoded. This should be read from the project version or a configuration file to avoid maintenance issues.
• Suggested Change:

.body(new JsonObject()
  .put("module_to", "mod-gobi-" + System.getProperty("project.version", "1.0.0"))
  .encode())

---

🔧 Integration test cleanup might not execute on test failure

• Priority: ⚠️ High
• File: src/test/java/org/folio/rest/impl/GobiOrdersCustomMappingsImplTest.java
• Details: Lines 46-60 implement cleanup in @AfterEach. If the test fails during execution or assertions, there's a risk the cleanup won't complete properly, leaving test data in the database for subsequent tests.
• Suggested Change: Consider using a try-finally pattern or wrapping in a robust error handler:

@AfterEach
void cleanup() {
  try {
    OrderMappingsViewCollection collection = RestAssured.with()
      .spec(spec)
      .get(MAPPINGS_PATH)
      .then()
      .statusCode(200)
      .contentType(ContentType.JSON)
      .extract().as(OrderMappingsViewCollection.class);

    collection.getOrderMappingsViews().stream()
      .filter(view -> MappingType.CUSTOM == view.getMappingType())
      .forEach(view -> {
        try {
          OrderType orderType = view.getOrderMappings().getOrderType();
          log.info("Cleaning up mapping for order type: {}", orderType);
          RestAssured.with()
            .spec(spec)
            .delete(MAPPINGS_PATH + "/" + orderType.value())
            .then()
            .statusCode(200);
        } catch (Exception e) {
          log.warn("Failed to delete mapping during cleanup", e);
        }
      });
  } catch (Exception e) {
    log.error("Cleanup failed", e);
  }
}

---

⛏️ Mock server test no longer validates configuration endpoint

• Priority: 🟢 Low
• File: src/test/java/org/folio/rest/impl/GOBIIntegrationServiceResourceImplTest.java
• Details: The mock server's handleGetConfigurationsEntries method was removed (lines deleted around 1300). Tests that previously validated the configuration retrieval path now have reduced coverage of that integration point.
• Rationale: This is expected since the configuration module is no longer used, but document that this testing gap exists.

---

🔧 Test uses @ExtendWith(MockitoExtension.class) but also calls MockitoAnnotations.openMocks

• Priority: 🟡 Medium
• File: src/test/java/org/folio/dao/OrderMappingsDaoTest.java
• Details: Lines 36 and 51 show both @ExtendWith(MockitoExtension.class) and manual MockitoAnnotations.openMocks(this). This is redundant - the extension handles mock initialization automatically.
• Suggested Change: Remove the manual initialization:

@ExtendWith(MockitoExtension.class)
@CopilotGenerated
class OrderMappingsDaoTest {
  // ...
  
  @BeforeEach
  void setUp() {
    dao = new OrderMappingsDaoImpl();
  }
  
  // Remove the closeable field and @AfterEach tearDown method
}

---

💭 Consider adding database migration scripts

• Priority: ⚠️ High
• File: src/main/resources/templates/db_scripts/schema.json (modified but not shown in diff)
• Details: The PR introduces a new database table order_mappings. Ensure that:
  1. The schema.json includes proper indexes on orderType field since it's used for lookups
  2. Migration scripts exist for existing tenants
  3. Consider adding a unique constraint on orderType to prevent duplicates
• Suggested Addition to schema:

{
  "tables": [
    {
      "tableName": "order_mappings",
      "withMetadata": true,
      "uniqueIndex": [
        {
          "fieldName": "orderType",
          "tOps": "ADD"
        }
      ]
    }
  ]
}

---

♻️ PostGobiOrdersHelper has reduced separation of concerns

• Priority: 🟡 Medium
• File: src/main/java/org/folio/rest/impl/PostGobiOrdersHelper.java
• Details: The helper now directly instantiates PostgresClient and OrderMappingsDaoImpl (lines 78-79). This creates tight coupling and makes unit testing harder.
• Suggested Change: Inject these dependencies through constructor:

public class PostGobiOrdersHelper {
  private final OrderMappingsDao orderMappingsDao;
  private final PostgresClient pgClient;
  
  public PostGobiOrdersHelper(
    Handler<AsyncResult<Response>> asyncResultHandler,
    Map<String, String> okapiHeaders,
    Context ctx,
    OrderMappingsDao orderMappingsDao,
    PostgresClient pgClient
  ) {
    // ...
    this.orderMappingsDao = orderMappingsDao != null 
      ? orderMappingsDao 
      : new OrderMappingsDaoImpl();
    this.pgClient = pgClient != null 
      ? pgClient 
      : PostgresClient.getInstance(ctx.owner(), tenantId);
  }
}

---

🔧 Removed method getDefaultMappingByOrderType might be used elsewhere

• Priority: 🟡 Medium
• File: src/main/java/org/folio/gobi/MappingHelper.java
• Details: Lines removed include the getDefaultMappingByOrderType static method. Ensure this method isn't used elsewhere in the codebase, or update callers accordingly.
• Rationale: Search the codebase for references to ensure no broken dependencies.

---

📝 GobiCustomMappingsService migration is well-executed

• Priority: N/A
• File: src/main/java/org/folio/rest/service/GobiCustomMappingsService.java
• Details: The service refactoring is well done:
  • Clean separation between database access (DAO) and business logic (Service)
  • Proper use of withConn for transaction management
  • Maintains backward compatibility in API responses
• Rationale: This is a good example of proper layering in the application architecture.

---

🔧 Test mocking in PostGobiOrdersHelperTest could be more realistic

• Priority: 🟡 Medium
• File: src/test/java/org/folio/rest/impl/PostGobiOrdersHelperTest.java
• Details: Lines 245-270 create a spy and mock the entire lookupOrderMappings method. While this works, it doesn't test the actual database interaction path. Consider adding integration tests that use a real database.
• Rationale: Unit tests with mocks are good, but integration tests provide higher confidence in the refactored code.

---

⛏️ @CopilotGenerated annotation usage

• Priority: 🟢 Low
• File: src/test/java/org/folio/dao/OrderMappingsDaoTest.java
• Details: Line 37 adds @CopilotGenerated annotation. Ensure this is a recognized annotation in your project and doesn't cause issues with code coverage or static analysis tools.

---

👍 Testcontainers integration is excellent

• Priority: N/A
• File: src/test/java/org/folio/rest/impl/BaseIntegrationTest.java
• Details: The use of Testcontainers with PostgreSQL (line 35) provides:
  • Isolated test environment
  • True integration testing
  • No external dependencies for CI/CD
• Rationale: This is a best practice for integration testing with databases.

---

💭 Consider adding performance tests

• Priority: 🟢 Low
• Details: The refactoring changes from cached configuration lookups to database queries. Consider adding performance benchmarks to ensure the change doesn't significantly impact response times, especially under load.
• Suggested Tests:
  • Benchmark time to lookup mapping with cache vs database
  • Load test with concurrent GOBI order submissions
  • Measure database connection pool utilization

---

🔧 Missing transaction boundaries in GobiCustomMappingsService

• Priority: ⚠️ High
• File: src/main/java/org/folio/rest/service/GobiCustomMappingsService.java
• Details: While the code uses pgClient.withConn(), there's no explicit transaction management. For operations like delete (lines 66-76), consider wrapping in a transaction to ensure consistency.
• Suggested Change:

public Future<Void> deleteCustomMapping(String orderType) {
  return pgClient.withTrans(conn -> 
    orderMappingsDao.getByOrderType(orderType, conn)
      .compose(orderMappings -> {
        if (orderMappings == null) {
          log.warn("deleteCustomMapping:: Mapping not found for orderType={}", orderType);
          return Future.failedFuture(new HttpException(404, ORDER_MAPPINGS_RECORD_NOT_FOUND));
        }
        String id = orderMappings.getId();
        return orderMappingsDao.delete(id, conn);
      })
  );
}

---

⛏️ Removed unnecessary blank lines in GobiOrdersCustomMappingsImplTest

• Priority: 🟢 Low
• File: src/test/java/org/folio/rest/impl/GobiOrdersCustomMappingsImplTest.java
• Details: Good code cleanup removing excessive blank lines throughout the file. This improves readability.

---

Summary

Critical Issues (🔥 Priority)

None identified.

High Priority Issues (⚠️)

1. CompletableFuture type safety in DataSourceResolver - ensure proper typing
2. Database connection management - avoid creating new instances for each request
3. Integration test cleanup - ensure cleanup executes even on test failures
4. Database schema - add proper indexes and constraints
5. Transaction boundaries - wrap multi-step operations in transactions

Medium Priority Issues (🟡)

8 issues related to:

• Missing @OverRide annotations
• Null safety checks
• Error handling specificity
• Dependency injection opportunities
• Test infrastructure improvements
• Caching strategy considerations

Low Priority Issues (🟢)

7 issues related to:

• Code consistency
• Documentation
• Minor refactoring opportunities
• Performance considerations

Positive Highlights (👍)

1. Excellent test coverage for DAO layer
2. Clean service layer refactoring
3. Proper use of Testcontainers for integration tests
4. Good separation of concerns between DAO and service layers
5. Comprehensive migration from configuration module to database

Overall Assessment

This refactoring represents a significant architectural improvement moving from a configuration-based approach to a proper database-backed DAO pattern. The code quality is generally high with comprehensive test coverage.

Key Strengths:

• Clean DAO pattern implementation
• Comprehensive test coverage
• Modern testing approach with Testcontainers
• Proper async/Future handling throughout

Areas for Improvement:

• Consider adding short-term caching for frequently accessed mappings
• Improve dependency injection to reduce coupling
• Add explicit transaction management for multi-step operations
• Ensure proper database indexes and constraints
• Add performance benchmarks to validate the refactoring doesn't degrade performance

Recommendation: ✅ Approve with minor changes

The high-priority issues should be addressed before merging, but they are relatively straightforward fixes. The medium and low priority issues can be addressed in follow-up work if needed.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
88.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud