Skip to content

feat(db): support MONGO_URI for db creation #254

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

feat(db): support MONGO_URI for db creation #254

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ea6800
feat: enhance MongoDB client creation to support MONGO_URI and MONGO_…
anuragxxd Aug 25, 2025
577e923
refactor: improve code readability by formatting multiline conditions…
anuragxxd Aug 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions foca/database/register_mongodb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from flask import Flask
from flask_pymongo import PyMongo
from foca.models.config import MongoConfig, DBConfig
from urllib.parse import urlparse, urlunparse

# Get logger instance
logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -123,6 +124,37 @@ def _create_mongo_client(
Returns:
MongoDB client for Flask application instance.
"""
# If full URI provided, prefer it over component-based construction
provided_uri = os.environ.get('MONGO_URI')
if provided_uri:
db_override = os.environ.get('MONGO_DBNAME')
if db_override:
try:
parsed = urlparse(provided_uri)
# Replace/insert path component with overridden db name
new_path = f"/{db_override}"
provided_uri = urlunparse((
parsed.scheme,
parsed.netloc,
new_path,
parsed.params,
parsed.query,
parsed.fragment,
))
except Exception:
# In case of any parsing issues, fall back to using the
# provided URI as-is; PyMongo will validate later.
pass
Comment on lines +144 to +147
Copy link

Choose a reason for hiding this comment

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

suggestion (bug_risk): Avoid broad exception handling for URI parsing.

Catching only relevant exceptions like ValueError will prevent unrelated errors from being hidden.

Suggested change
except Exception:
# In case of any parsing issues, fall back to using the
# provided URI as-is; PyMongo will validate later.
pass
except ValueError:
# In case of any parsing issues, fall back to using the
# provided URI as-is; PyMongo will validate later.
pass

Copy link
Member

Choose a reason for hiding this comment

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

Actually, given that FOCA will manage error handling gracefully, I don't think the try-except block is not needed at all here - certainly not if we just catch a broad exception and then just silently ignore it. So either catch a ValueError and log an appropriate warning OR just remove the try/catch altogether.

Copy link

Choose a reason for hiding this comment

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

You're absolutely right—silently catching all exceptions can mask real issues and make debugging harder. Since FOCA will handle errors gracefully, you have two solid options:

  1. Remove the try/except block entirely: If the URI is malformed, let the error propagate. PyMongo or FOCA's error handling will catch and log it appropriately. This keeps the code clean and transparent.

  2. Catch only ValueError and log a warning: If you want to handle just URI parsing errors, catch ValueError and log a warning so users know their override was ignored. For example:

try:
    parsed = urlparse(provided_uri)
    new_path = f"/{db_override}"
    provided_uri = urlunparse((
        parsed.scheme,
        parsed.netloc,
        new_path,
        parsed.params,
        parsed.query,
        parsed.fragment,
    ))
except ValueError:
    logger.warning(
        "Failed to parse MONGO_URI for DBNAME override; using original URI."
    )

Both approaches are valid, but logging a warning is more user-friendly if you expect misconfiguration to happen. If you prefer simplicity and trust downstream error handling, removing the block is fine too.

Let me know if you'd like a specific code suggestion for either approach!

app.config['MONGO_URI'] = provided_uri
mongo = PyMongo(app)
logger.info(
"Registered database '{db}' using provided MONGO_URI.".format(
db=os.environ.get('MONGO_DBNAME', db)
)
)
return mongo
Comment on lines +148 to +155
Copy link

Choose a reason for hiding this comment

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

suggestion: Consider logging the actual URI or its components for traceability.

Including more details from the URI (excluding sensitive information) in the log will improve traceability and aid in debugging connection problems.

Suggested change
app.config['MONGO_URI'] = provided_uri
mongo = PyMongo(app)
logger.info(
"Registered database '{db}' using provided MONGO_URI.".format(
db=os.environ.get('MONGO_DBNAME', db)
)
)
return mongo
app.config['MONGO_URI'] = provided_uri
mongo = PyMongo(app)
# Parse URI for traceability (excluding credentials)
try:
from urllib.parse import urlparse
parsed_uri = urlparse(provided_uri)
host_info = parsed_uri.hostname
port_info = parsed_uri.port
db_name = os.environ.get('MONGO_DBNAME', db)
logger.info(
"Registered database '%s' using MONGO_URI host='%s', port='%s', db='%s'.",
db_name,
host_info,
port_info,
parsed_uri.path.lstrip('/') if parsed_uri.path else db_name
)
except Exception as e:
logger.info(
"Registered database '%s' using provided MONGO_URI (could not parse components: %s).",
os.environ.get('MONGO_DBNAME', db),
str(e)
)
return mongo

Copy link
Member

Choose a reason for hiding this comment

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

I agree that logging is very useful, but the suggested implementation is quite clunky. See whether you can sanitize the MONGO_URI in a way that printing it without errors is ensured (so that the try-catch is not needed). Also, use f-strings instead of %s.

Copy link

Choose a reason for hiding this comment

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

You're absolutely right—logging should be both informative and clean. You can sanitize the MONGO_URI for logging by parsing it and reconstructing a version that omits credentials, then log the relevant components using f-strings. Since urlparse does not raise exceptions for malformed URIs, you can avoid the try-catch block entirely.

Here's a suggested approach:

from urllib.parse import urlparse

app.config['MONGO_URI'] = provided_uri
mongo = PyMongo(app)
parsed = urlparse(provided_uri)

# Sanitize URI for logging (omit username and password)
host_info = parsed.hostname or 'unknown-host'
port_info = parsed.port or 'default-port'
db_name = os.environ.get('MONGO_DBNAME', db)
uri_scheme = parsed.scheme or 'mongodb'
uri_path = parsed.path.lstrip('/') or db_name

logger.info(
    f"Registered database '{db_name}' using {uri_scheme}://{host_info}:{port_info}/{uri_path} (sanitized)."
)

This will ensure the log is both readable and safe, and uses f-strings for clarity. Let me know if you'd like further refinements!


# Fall back to legacy component-based construction
auth = ''
user = os.environ.get('MONGO_USERNAME')
if user is not None and user != "":
Expand Down
23 changes: 23 additions & 0 deletions tests/database/test_register_mongodb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,29 @@ def test_register_mongodb_no_database():
assert isinstance(res, MongoConfig)


def test__create_mongo_client_with_mongo_uri(monkeypatch):
"""When MONGO_URI environment variable IS defined, prefer it."""
# SRV style and options should be accepted without modification
mongo_uri = "mongodb://user:[email protected]:27017,db2.example.com:27017/admin?replicaSet=rs0&ssl=true"
monkeypatch.setenv("MONGO_URI", mongo_uri)
app = Flask(__name__)
res = _create_mongo_client(app)
assert isinstance(res, PyMongo)
assert app.config['MONGO_URI'] == mongo_uri
Copy link

Choose a reason for hiding this comment

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

suggestion (testing): Missing test for invalid MONGO_URI values.

Add a test with an invalid MONGO_URI to confirm the function handles errors and falls back correctly.



def test__create_mongo_client_with_mongo_uri_and_db_override(monkeypatch):
"""When MONGO_URI and MONGO_DBNAME are both defined, override db name."""
mongo_uri = "mongodb://localhost:27017/old_db?retryWrites=true&w=majority"
monkeypatch.setenv("MONGO_URI", mongo_uri)
monkeypatch.setenv("MONGO_DBNAME", "new_db")
app = Flask(__name__)
res = _create_mongo_client(app)
assert isinstance(res, PyMongo)
assert app.config['MONGO_URI'].startswith("mongodb://localhost:27017/new_db")
assert "retryWrites=true" in app.config['MONGO_URI']
Copy link

Choose a reason for hiding this comment

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

suggestion (testing): Missing test for MONGO_DBNAME override when MONGO_URI has no path.

Add a test case where MONGO_URI lacks a database path and MONGO_DBNAME is set to verify correct URI construction.



def test_register_mongodb_no_collections():
"""Register MongoDB database without any collections"""
app = Flask(__name__)
Expand Down
Loading