Skip to content

III-527 Create dedicated database connection checker#2146

Merged
lucwollants merged 5 commits intomasterfrom
III-527-create-dedicated-database-connection-checker
Jan 29, 2026
Merged

III-527 Create dedicated database connection checker#2146
lucwollants merged 5 commits intomasterfrom
III-527-create-dedicated-database-connection-checker

Conversation

@lucwollants
Copy link
Contributor

Changed

  • Created dedicated database connection checker

Ticket: https://jira.uitdatabank.be/browse/III-527

@lucwollants
Copy link
Contributor Author

@claude please review

Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Summary

This is a well-executed refactoring that extracts database connection checking logic into a dedicated, reusable component. The changes follow SOLID principles by creating a focused interface and implementation.

Strengths ✅

  1. Clean separation of concerns: The DatabaseConnectionChecker interface and DBALDatabaseConnectionChecker implementation properly separate connection management from business logic
  2. Excellent test coverage: The new DBALDatabaseConnectionCheckerTest covers all three scenarios (healthy, reconnection success, reconnection failure)
  3. Proper test migration: Tests were correctly moved from EventBusForwardingConsumerTest to the new dedicated test class
  4. Better reusability: The checker can now be used by other consumers without code duplication
  5. Logging preserved: All debug, warning, and critical logging behavior is maintained

Suggestions for Improvement 🔍

  1. Error handling: Consider how reconnection failures should be communicated to callers (currently silent after logging)
  2. Performance: Running SELECT 1 on every message may be overkill - consider periodic checking or reactive checking only after failures
  3. Documentation: Add docblocks to clarify the interface contract and expected behavior
  4. Minor style fix: String should be lowercase string in the constructor parameter

Architecture Notes

The refactoring successfully:

  • Removes 29 lines from EventBusForwardingConsumer and adds 40 lines in the new dedicated class (net positive for maintainability)
  • Creates a testable boundary via the interface
  • Makes the service provider slightly more verbose but clearer in dependencies
  • Enables future connection checkers for different database types if needed

Overall, this is a solid refactoring that improves code organization and testability. The suggestions above are enhancements to consider, not blockers.

@cultuurnet cultuurnet deleted a comment from claude bot Jan 29, 2026
@lucwollants lucwollants marked this pull request as ready for review January 29, 2026 09:21
Copy link
Contributor

@grubolsch grubolsch left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this, and great job adding some tests!

@lucwollants lucwollants merged commit 64c217c into master Jan 29, 2026
5 checks passed
@lucwollants lucwollants deleted the III-527-create-dedicated-database-connection-checker branch January 29, 2026 11:55
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.

2 participants