feat(db): support MONGO_URI for db creation #254
Conversation
Reviewer's GuideEnhance MongoDB client registration to prioritize a full connection URI from environment variables (MONGO_URI) with optional database name override (MONGO_DBNAME), while retaining legacy component-based construction; added corresponding tests for these scenarios. Sequence diagram for MongoDB client creation with MONGO_URI and MONGO_DBNAME overridessequenceDiagram
participant App as Flask App
participant Env as Environment Variables
participant PyMongo as PyMongo
participant Logger as Logger
App->>Env: Get MONGO_URI
alt MONGO_URI is set
App->>Env: Get MONGO_DBNAME
alt MONGO_DBNAME is set
App->>App: Parse and override DB name in URI
else MONGO_DBNAME not set
App->>App: Use provided MONGO_URI as-is
end
App->>PyMongo: Register client with MONGO_URI
App->>Logger: Log registration with DB name
else MONGO_URI not set
App->>Env: Get legacy components (username, password, host, etc.)
App->>PyMongo: Register client with constructed URI
end
Class diagram for enhanced MongoDB client registrationclassDiagram
class Flask {
}
class PyMongo {
}
class MongoConfig {
}
class DBConfig {
}
class register_mongodb {
+_create_mongo_client(app, db, config)
}
Flask --> PyMongo : uses
register_mongodb --> Flask : receives app
register_mongodb --> PyMongo : creates client
register_mongodb --> MongoConfig : uses config
register_mongodb --> DBConfig : uses config
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The broad exception in the URI override block can mask unexpected issues—consider catching only specific parsing errors so other problems aren’t swallowed.
- Extract the URI parsing and database‐override logic into a dedicated helper to simplify and improve the readability of _create_mongo_client.
- It would help to log the actual overridden database name (or a sanitized form of the final URI) to make it easier to verify which connection string is in use.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The broad exception in the URI override block can mask unexpected issues—consider catching only specific parsing errors so other problems aren’t swallowed.
- Extract the URI parsing and database‐override logic into a dedicated helper to simplify and improve the readability of _create_mongo_client.
- It would help to log the actual overridden database name (or a sanitized form of the final URI) to make it easier to verify which connection string is in use.
## Individual Comments
### Comment 1
<location> `foca/database/register_mongodb.py:144` </location>
<code_context>
+ 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.
</code_context>
<issue_to_address>
Avoid broad exception handling for URI parsing.
Catching only relevant exceptions like ValueError will prevent unrelated errors from being hidden.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
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
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `foca/database/register_mongodb.py:150` </location>
<code_context>
+ pass
+ 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)
+ )
</code_context>
<issue_to_address>
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.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
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
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `tests/database/test_register_mongodb.py:84` </location>
<code_context>
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:pass@db1.example.com: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
+
+
</code_context>
<issue_to_address>
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.
</issue_to_address>
### Comment 4
<location> `tests/database/test_register_mongodb.py:95` </location>
<code_context>
+ assert app.config['MONGO_URI'] == mongo_uri
+
+
+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']
+
+
</code_context>
<issue_to_address>
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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| except Exception: | ||
| # In case of any parsing issues, fall back to using the | ||
| # provided URI as-is; PyMongo will validate later. | ||
| pass |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid broad exception handling for URI parsing.
Catching only relevant exceptions like ValueError will prevent unrelated errors from being hidden.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
-
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.
-
Catch only ValueError and log a warning: If you want to handle just URI parsing errors, catch
ValueErrorand 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 |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
| 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 |
There was a problem hiding this comment.
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'] |
There was a problem hiding this comment.
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.
… and dictionary definitions
uniqueg
changed the title
There was a problem hiding this comment.
Thanks, @anuragxxd! Please add the stuff we discussed and address the issues raised by Sourcery (and commented by me). Otherwise, looks good :)
| except Exception: | ||
| # In case of any parsing issues, fall back to using the | ||
| # provided URI as-is; PyMongo will validate later. | ||
| pass |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
2 participants
…DBNAME overrides
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist:
Summary by Sourcery
Enhance MongoDB client registration to support using a full MONGO_URI environment variable with optional database name overrides via MONGO_DBNAME, falling back to legacy component-based configuration if not provided.
New Features:
Tests: