Conversation
Jungminchae
commented
Oct 28, 2025
Jungminchae
commented
- to put session, engine config, made parameter for seesion, engine configuration
Summary of ChangesHello @Jungminchae, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant improvements to the application's configuration and database interaction layers. It modernizes environment variable management by migrating to Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the database configuration to allow passing engine and session parameters, and switches from python-decouple to pydantic-settings for configuration management. My review found a critical issue in the settings template file where the switch to pydantic-settings was incomplete, which will cause the application to crash. I also found a high-severity issue in the database configuration class related to the use of mutable default arguments, which could lead to subtle bugs. I've provided suggestions to fix both issues.
| DEBUG: bool = config("DEBUG", cast=bool, default=True) | ||
| OPENAPI_URL: str | None = "/openapi.json" if DEBUG else None |
There was a problem hiding this comment.
These lines will cause NameError exceptions at runtime.
- On line 8, the
configfunction is no longer defined sincepython-decouplewas removed. Withpydantic-settings, you can just declare the attribute with a default value. - On line 9,
DEBUGis not a defined variable in the class scope, so it cannot be used to defineOPENAPI_URL.
To fix this and preserve the intended logic, you should use a Pydantic model_validator. This requires importing model_validator from pydantic.
Here's an example of the full implementation:
from pathlib import Path
from zoneinfo import ZoneInfo
from pydantic import model_validator
from pydantic_settings import BaseSettings, SettingsConfigDict
class AppSettings(BaseSettings):
model_config = SettingsConfigDict(env_file=".env", env_file_encoding="utf-8")
DEBUG: bool = True
OPENAPI_URL: str | None = None
API_PREFIX: str = "/api/v1"
# ... other settings
@model_validator(mode='after')
def set_openapi_url(self) -> 'AppSettings':
if self.DEBUG:
self.OPENAPI_URL = "/openapi.json"
return selfThe suggested code below provides a minimal fix to make the template parsable, but I strongly recommend implementing the validator pattern for correct behavior.
DEBUG: bool = True
OPENAPI_URL: str | None = "/openapi.json"
| def __init__(self, db_url: str, meta: MetaData = None, engine_config: dict = {}, session_config: dict = {}) -> None: | ||
| self.db_url = db_url | ||
| self.meta = meta | ||
| self.__engine = create_async_engine(db_url) | ||
| self.__async_session = async_sessionmaker(self.__engine, **kwargs) | ||
| self.__engine = create_async_engine(db_url, **engine_config) | ||
| self.__async_session = async_sessionmaker(self.__engine, **session_config) |
There was a problem hiding this comment.
Using mutable default arguments like {} for engine_config and session_config can lead to unexpected behavior. The same dictionary object will be reused across different calls to the constructor if no value is provided, which can cause side effects if the dictionary is modified in-place. The recommended practice is to use None as the default and create a new dictionary inside the method if one isn't provided.
| def __init__(self, db_url: str, meta: MetaData = None, engine_config: dict = {}, session_config: dict = {}) -> None: | |
| self.db_url = db_url | |
| self.meta = meta | |
| self.__engine = create_async_engine(db_url) | |
| self.__async_session = async_sessionmaker(self.__engine, **kwargs) | |
| self.__engine = create_async_engine(db_url, **engine_config) | |
| self.__async_session = async_sessionmaker(self.__engine, **session_config) | |
| def __init__(self, db_url: str, meta: MetaData = None, engine_config: dict | None = None, session_config: dict | None = None) -> None: | |
| self.db_url = db_url | |
| self.meta = meta | |
| self.__engine = create_async_engine(db_url, **(engine_config or {})) | |
| self.__async_session = async_sessionmaker(self.__engine, **(session_config or {})) |
1 participant
