Fix overly broad exception handling in Node_collection.health_check to catch SQLAlchemyError only

[Bornunique911]

The health_check() method in db.py currently uses a generic except Exception as a fallback after catching OperationalError. This can mask programming faults (e.g., AttributeError, TypeError) and make debugging harder. The fallback handler should catch only database‑layer exceptions that are expected when the database is unhealthy, specifically SQLAlchemyError.

Expected Behavior

Database connectivity issues should be reported as ok=False with appropriate reason.

Other errors (e.g., NameError, misused attribute) should not be caught silently; they should propagate to reveal bugs.

The fallback handler should only suppress SQLAlchemy‑specific exceptionsthat are notOperationalError` (though SQLAlchemyError covers them, we already catch OperationalError separately).

Actual Behavior

Any unexpected exception is caught and returns {"ok": False, "db_reachable": False, ...}, making it difficult to detect programming errors in the health check logic.

Proposed Fix

Add SQLAlchemyError to the import list from sqlalchemy.exc.

Replace except Exception: with except SQLAlchemyError:.

Updated code:

from sqlalchemy.exc import OperationalError, IntegrityError, SQLAlchemyError

# ... inside health_check ...
except SQLAlchemyError:  # pragma: no cover
    return {
        "ok": False,
        "db_reachable": False,
        "reason": "database health query failed",
    }

Reference

• This fix has been implemented in PR: cwe: add XXE search aliases and enforce prohibited CWE exclusions #939

[Bornunique911]

@coderabbitai review

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh issue view 940 --repo OWASP/OpenCRE --json title,body,labels,assignees,state,author,createdAt,comments

Length of output: 2389

---

🏁 Script executed:

#!/bin/bash
# Check PR `#939` details
gh pr view 939 --repo OWASP/OpenCRE --json title,body,state,files

# Find db.py in the repo
fd db.py

Length of output: 2046

---

🏁 Script executed:

#!/bin/bash
# Find health_check in db.py
grep -n "health_check\|OperationalError\|SQLAlchemyError\|except Exception\|except " application/database/db.py | head -60

Length of output: 687

---

🏁 Script executed:

#!/bin/bash
sed -n '2267,2310p' application/database/db.py

Length of output: 1944

---

---

@Bornunique911 Here is my review of issue #940:

---

Issue Assessment: ✅ Valid — with one correction needed

Problem Confirmation

The issue is correctly identified. In application/database/db.py (line 2292), the health_check() method uses an overly broad except Exception fallback:

except OperationalError:
    return {"ok": False, "db_reachable": False, "reason": "database unreachable"}
except Exception:  # pragma: no cover - defensive, never fail open
    return {"ok": False, "db_reachable": False, "reason": "database health query failed"}

This will silently swallow bugs like AttributeError, TypeError, or NameError within the health check logic itself — returning a structured {"ok": False} response instead of propagating the error, making such bugs very hard to detect.

---

Proposed Fix Assessment: ✅ Technically Sound

Replacing except Exception with except SQLAlchemyError is correct:

• SQLAlchemy hierarchy: SQLAlchemyError → DBAPIError → OperationalError, so OperationalError is a subclass of SQLAlchemyError. The two-clause pattern is valid Python — except OperationalError is checked first and handles connection-level errors, while except SQLAlchemyError catches any remaining SQLAlchemy-level errors.
• The from sqlalchemy.exc import OperationalError, IntegrityError import on line 26 needs SQLAlchemyError added, as proposed.

---

⚠️ Correction: PR #939 Reference is Incorrect

The issue body states:

"This fix has been implemented in PR: #939"

However, PR #939 is titled "cwe: add XXE search aliases and enforce prohibited CWE exclusions" — it is entirely unrelated to exception handling in health_check(). The fix described in this issue has not been implemented in any PR yet.

---

⚠️ Design Trade-off to Consider

The existing comment # pragma: no cover - defensive, never fail open explicitly signals the intent to always return a structured response from the health endpoint. With the proposed change, non-SQLAlchemy exceptions (e.g., a programming bug) will propagate as unhandled exceptions, potentially causing the health endpoint to return an HTTP 500 with a stack trace rather than a structured {"ok": False} response. This is arguably better for detectability during development/testing, but maintainers should be aware of this behavioral change in production.

---

Summary

Item	Status
Root cause correctly identified	✅
Fix (except SQLAlchemyError) is technically correct	✅
Import update required (SQLAlchemyError)	✅
PR #939 reference	❌ Incorrect — unrelated PR
"Never fail open" philosophy change	⚠️ Intentional trade-off, should be noted in PR

The fix is worth implementing. The PR reference in the issue body should be corrected, and whoever opens the implementing PR should document the behavioral change in the PR description.

[Bornunique911]

@coderabbitai Can you create a PR for it?

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
grep -n "from sqlalchemy.exc import" application/database/db.py

Length of output: 183

---

🏁 Script executed:

#!/bin/bash
sed -n '2280,2310p' application/database/db.py

Length of output: 1230

---

---

@Bornunique911, here are the exact diffs needed. You (or a contributor) can apply these and open the PR:

---

Change 1 — application/database/db.py, line 26 (add SQLAlchemyError to import):

-from sqlalchemy.exc import OperationalError, IntegrityError
+from sqlalchemy.exc import OperationalError, IntegrityError, SQLAlchemyError

---

Change 2 — application/database/db.py, lines 2292–2296 (narrow the catch clause):

-        except Exception:  # pragma: no cover - defensive, never fail open
+        except SQLAlchemyError:
             return {
                 "ok": False,
                 "db_reachable": False,
                 "reason": "database health query failed",
             }

---

Suggested PR description

Fix: Narrow except Exception to except SQLAlchemyError in health_check()

Closes #940

Problem: The broad except Exception in health_check() silently swallows programming bugs (AttributeError, TypeError, NameError, etc.), making them very hard to detect — they'd appear as a structured {"ok": False} health response instead of propagating.

Fix: Replace except Exception with except SQLAlchemyError. Since OperationalError is already caught first (it's a subclass of SQLAlchemyError), this clause now handles only remaining SQLAlchemy-level errors.

Behavioral change: Non-SQLAlchemy exceptions will now propagate naturally rather than being swallowed. This is intentional — programming bugs should surface, not be hidden behind a health-check response.

[Bornunique911]

@coderabbitai Created PR #941 for the fix.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh pr view 941 --repo OWASP/OpenCRE --json title,body,state,url,files

Length of output: 2779

---

---

@Bornunique911, great work! PR #941 looks exactly right — it contains both expected changes:

1. ✅ SQLAlchemyError added to the sqlalchemy.exc import
2. ✅ except Exception replaced with except SQLAlchemyError in health_check()

The PR description also clearly documents the behavioral change (non-SQLAlchemy exceptions will now propagate rather than being silently swallowed), which is important for reviewers to understand. It's well-structured and ready for review by the maintainers. Good luck with the merge! 🎉

[Bornunique911]

The issue is fixed by #939.

@coderabbitai review

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR `#939` files and diff
gh pr view 939 --repo OWASP/OpenCRE --json title,body,state,files,mergedAt

# Check if the fix is already in the current db.py
grep -n "SQLAlchemyError\|except Exception\|except SQLAlchemy\|health_check" application/database/db.py | head -30

Length of output: 2482

---

🏁 Script executed:

#!/bin/bash
# Get the actual diff of PR `#939`
gh pr diff 939 --repo OWASP/OpenCRE 2>/dev/null | grep -A10 -B5 "SQLAlchemy\|except\|health_check" | head -80

Length of output: 2064

---

---

@Bornunique911, you were right — I apologize for the earlier incorrect analysis. I only looked at the PR title and description of #939 and concluded it was unrelated, but I missed that the fix for issue #940 was bundled inside that PR alongside the CWE alias work.

The diff confirms both changes from #939 are now merged and live:

1. ✅ SQLAlchemyError added to the import on line 26
2. ✅ except Exception replaced with except SQLAlchemyError in health_check() on line 2343

Issue #940 is fully resolved by PR #939.

⚠️ Note: PR #941 that was opened based on my incorrect analysis is now a duplicate — it should be closed since the fix is already in the codebase.

[Bornunique911]

It is closed now. @coderabbitai

[coderabbitai]

@Bornunique911, perfect — thanks for closing #941! Everything is now clean: issue #940 is resolved by #939, and the duplicate PR is closed. 🎉