-
Notifications
You must be signed in to change notification settings - Fork 1
[BAK-10] DB 연결 마법사 구현 #27
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.
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
Conversation
b28a1d9 to
36e816b
Compare
|
드라이버 설치 여부를 기존 환경 기반 탐색에서 → 명시적 의존성 관리 방식으로 변경했습니다. |
ChoiSeungWoo98
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다.
리뷰 확인 후 답변 부탁드립니다.
감사합니다.
app/api/connections.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전달 받은 str 값이 정상 범주 안에 있는 값인지 체크하는 로직이 존재하면 1차적으로 빠르게 데이터를 걸러 데이터 반환 시간을 단축할 수 있을 거 같습니다.
어떻게 생각하시나여??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 이해한게 맞는지 모르겠지만 해당 작업에 대해 말씀드리자면, 저희가 지원하는 DB 들은 총 6가지 (PostgreSQL, MySQL, MariaDB, SQLite, Oracle, SQL Server) 입니다.
사용자의 직접 입력이 아니라 UI에서 사용자에게 드라이버 종류를 선택하도록 하면서, 이미 6종의 DB로 제한되어 있어, 프론트엔드에서 기본적인 유효성 검증을 한 번 하고 들어오는 셈이라 말씀 주신 부분을 고려하지 못하였네요.
말씀주신 의견은 충분히 가치가 있다고 생각하여 추후 수정하도록 하겠습니다.
의견 감사합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음 restapi로 쏴도 못 받게 막아주는 부분이 존재하나요?
service 쪽 시작 부분에 작성해두신 검증 로직이 이 부분으로 오면 좋을 거 같다고 말씀 드린 거 입니다.
해당 부분은 객체를 만들어 객체 내부에서 구현해도 좋을 거 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 동적 포트 할당은 지난 번에 얘기했을 때 할지 말지 고민했던 거 같은데 어떻게 되었나요??
- get으로 /를 받는 부분은 메시지를 전달하는데 어떤 역할을 하나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 동적 할당 부분은 해당 브랜치 종료 후 수정예정입니다. 생각해둔 포트 번호는
39722입니다. 어떠신가요? - 해당 질문은 @app.get("/") 이 부분에 해당하는 걸까요?
@app.get("/")
async def read_root():
return {"message": "Hello, FastAPI Backend!"}
보시는 거와 같이 브라우저 첫 화면 역할입니다. 초기 개발 세팅 후 제거를 안한 흔적이죠.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 네, 좋습니다
- 필요 없는 부분이라면 제거 부탁 드립니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1,2 수정 및 제거 하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- app.include_router(api_router, prefix="/api")로 전체 엔드포인트에 api를 달아주고
- api_route.py를 만들어 모든 흐름제어를 한곳에서 하는 건 어떠신가요??
예시 입니다!
# app/api_router.py
from fastapi import APIRouter
from app.routers import users, items # 가정: items 라우터도 있다고 가정
# 최상위 라우터 객체 생성
api_router = APIRouter()
# 기능별 라우터를 여기에 포함 (이때는 prefix를 사용하지 않음)
api_router.include_router(users.router, prefix="/users")
api_router.include_router(items.router, prefix="/items")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1,2 확인했습니다. 해당 사항 공부하고 진행하겠습니다.
app/services/driver_info_provider.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 맨 처음 검증하는 로직은 앞단으로 빼주면 역할 분담이 더욱 명확해질 거 같습니다.
- 이 부분은 차라리 어제 설명해주신 객체쪽에서 수행하는 것도 좋아 보입니다.
- 비즈니스 로직은 데이터를 처리하고 결과를 반환할 때 어떻게 처리할지 정하는 역할만 수행하도록 수정
- for mod_name in module_names: 해당 부분은 왜 반복 돌리나요??
어차피 위에서 하나의 키만 가져오는 거 아닌가요?module_names = DRIVER_MAP.get(driver_key) - 디비 정보를 가져오는 로직이 구현되어 있는데 이 부분은 사용자에게 노출해주기 위해 사용되는 건가요??
- 디비 정보를 data로 반환하고 있는데 이것또한 객체로 만들어서 사용하는게 더 좋을 거 같습니다.
- 지금 모든 반환이 {}로 매번 입력해주고 있는데 이거 모델로 빼면 사용하기 편할 거 일관성도 보장될 거 같습니다.
노션
블로그
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가적으로 참고 부탁드립니다.
참고용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 검증 로직이란 '해당 드라이버 값이 맞는지'에 대한 로직인가요? 그럼 앞단으로 빼라는게 api 쪽으로 빼라는건지 궁금합니다.
- 여러 드라이버 후보 중에 하나라도 설치되어 있는지를 확인하기 위해 반복문으로 순회하면서 첫 번째 성공하는 모듈을 반환하려는 목적이였습니다. 하지만 저희 서비스는 설치된 환경이니 상관없겠네요. 의견감사합니다.
- 오 ! 감사합니다 사용자에게 직접 노출되는 목적이 아니라, 프론트엔드 로직 처리에만 사용하는 내부 데이터 전달 용도였는데 저렇게 되어있었네요 수정하겠습니다.
- 확인했습니다. 수정하겠습니다.
- 확인했습니다. 수정하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- api쪽으로 빼는 건 어떠한지 물어보는 거 입니다.
혼선이 생길가 미리 말씀드리면 api쪽에 작성해둔 거랑 동일한 내용 입니다. - 아래 코드 확인 하시면 하나의 값만 map형식으로 추출하는데 반복을 돌리는 이유가 없어 보입니다.
그리고 해당 부분은 키 값을 입력 받을 때 다중 키값을 받는게 아닌 거 같은데 의도하신게 맞는지 확인 부탁드립니다.
def db_driver_info(driver_id: str) -> dict:
driver_key = driver_id.lower()
module_names = DRIVER_MAP.get(driver_key)
- 내부 전달 용도가 무엇인가요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 확인했습니다.
- 원래는 지원하는 드라이버가 1-2개라고 생각하고 코드를 작성하느라 다중 값을 반환하여 반복을 돌리는 것이였는데 현재는 단일 값만 지정해서 하려고 합니다.
- 제가 질문자체를 잘못읽었던 것 같습니다. 해당 DB 드라이버 정보 노출건에 대해서 말씀하시는거죠? 해당부분은 저희 prototype 보시면 버전, 드라이버 명, 크기를 사용자에게 노출하고 있습니다. 그래서 프론트에게 전달하여 사용자에게 노출하기 위함입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맨처음 코드리뷰에 대한 수정 부분 답변드립니다.
- 모델 사용으로 api쪽에서 사용하게끔 변경하였습니다.
- 반복문 제거했습니다.
4, 5.마찬가지로 모델사용으로 수정하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- info라는 반환 객체를 만드는 것을 driver_info쪽으로 넘기는 팩토리 메서드 패턴 사용은 어떠신가요??
객체 생성에 필요한 로직을 객체 내부로 넘겨 책임과 역할을 명확하게 하고 가독성 및 재사용성을 향상시키기 위해 검토 부탁드립니다.
참고용 예시)
# app/schemas/driver_schemas.py
import importlib
import os
from pydantic import BaseModel
class DriverInfo(BaseModel):
db_type: str
is_installed: bool
driver_name: str | None
driver_version: str | None
driver_size_bytes: int | None
@classmethod
def from_module(cls, db_type: str, module_name: str):
"""모듈 이름으로부터 DriverInfo 객체를 생성하는 팩토리 메서드"""
# 서비스에 있던 로직을 이곳으로 이동
mod = importlib.import_module(module_name)
version = getattr(mod, "__version__", None)
path = getattr(mod.__spec__, "origin", None)
size = os.path.getsize(path) if path else None
# 자기 자신의 인스턴스를 생성하여 반환
return cls(
db_type=db_type,
is_installed=True,
driver_name=module_name,
driver_version=version,
driver_size_bytes=size,
)# As-is
info = DriverInfo(
db_type=db_type,
is_installed=True,
driver_name=module_name,
driver_version=version,
driver_size_bytes=size,
)
# To-be
info = DriverInfo.from_module(db_type, module_name)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 이런게 있군요. 수정하도록 하겠습니다.
ChoiSeungWoo98
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 짜느라 고생하셨습니다.
추가적으로 논의하면 좋을 거 같은 부분 댓글 남겼습니다.
감사합니다.
app/api/connect_driver.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 일전에 PEP 8, Linter(Ruff), Formatter(Black)으로 from 절마다 줄바꿈이 한 줄 추가되는 거로 이해했는데 제가 잘못 이해했을 까요??
- driver_id 말고 driver라고 쓰는 건 어떤가요?? 이후 이넘으로 받아오는 값을 객체로 받으면서 변수명을 value보다 명확하게 작성하는 건 어떠신가요??
- 이후 지금처럼 obj를 변수로 담고 넘기는 것이 아닌 객체로 넘기는 건 어떤가요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 저도 그녀석들의 마음을 잘 모르겠습니다. 하지만 뭔가 규칙이 있나봅니다.
- 아 맞습니다. API 명세에 그렇게 작성되어있는데 놓쳤네요. 수정하겠습니다. / value 말고 어떤게 있을까요? 흠. 고민을 해보겠습니다.
- 해당 말씀은 이해가 되지 않습니다. 회의때 자세히 설명 부탁드리겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app/assets/driver_enum.py에 파일을 빼서 작성하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return ResponseMessage.success(
value=db_driver_info(DriverInfo.from_driver_info(db_type=db_type_enum.name, driver_name=db_type_enum.value))
해당 부분 메서드 사용으로 객체로 넘기는 쪽으로 수정 완료했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- app.include_router(api_router, prefix="/api")로 전체 엔드포인트에 api를 달아주고
- api_route.py를 만들어 모든 흐름제어를 한곳에서 하는 건 어떠신가요??
예시 입니다!
# app/api_router.py
from fastapi import APIRouter
from app.routers import users, items # 가정: items 라우터도 있다고 가정
# 최상위 라우터 객체 생성
api_router = APIRouter()
# 기능별 라우터를 여기에 포함 (이때는 prefix를 사용하지 않음)
api_router.include_router(users.router, prefix="/users")
api_router.include_router(items.router, prefix="/items")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Enum은 효율적인 관리와 재사용성을 위해 따로 파일로 빼서 관리하는 건 어떠신가요?
- Enum 반환을 obj가 아닌 객체로 관리하는 건 어떠신가요??
- Response 또한 모든 객체에서 동일한 형식 유지를 위해 따로 빼서 관리하는 건 어떻게 생각하시나요?
이후 api에서 반환할 때 해당 객체를 사용하는 방향은 어떠신가요??
예시)
# app/schemas/response.py
from typing import Generic, TypeVar, Optional, Any
from pydantic import BaseModel
# T라는 이름의 제네릭 타입 변수 생성. 어떤 타입이든 될 수 있음을 의미.
T = TypeVar('T')
class ResponseMessage(BaseModel, Generic[T]):
"""공용 응답 모델"""
message: str
data: Optional[T] = None
@classmethod
def success(cls, value: T, msg: str = "성공적으로 불러왔습니다."):
return cls(message=msg, data=value)
@classmethod
def error(cls, e: Exception | None = None, msg: str = "처리 중 오류가 발생했습니다."):
if e:
# 실제 운영 환경에서는 logging 라이브러리 사용을 권장합니다.
print(f"[ERROR] {type(e).__name__}: {e}")
return cls(message=msg, data=None)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 사항 공부 좀 하고 다시 답변드리겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀 주신 그대로 수정하였습니다.
app/services/driver_info_provider.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- info라는 반환 객체를 만드는 것을 driver_info쪽으로 넘기는 팩토리 메서드 패턴 사용은 어떠신가요??
객체 생성에 필요한 로직을 객체 내부로 넘겨 책임과 역할을 명확하게 하고 가독성 및 재사용성을 향상시키기 위해 검토 부탁드립니다.
참고용 예시)
# app/schemas/driver_schemas.py
import importlib
import os
from pydantic import BaseModel
class DriverInfo(BaseModel):
db_type: str
is_installed: bool
driver_name: str | None
driver_version: str | None
driver_size_bytes: int | None
@classmethod
def from_module(cls, db_type: str, module_name: str):
"""모듈 이름으로부터 DriverInfo 객체를 생성하는 팩토리 메서드"""
# 서비스에 있던 로직을 이곳으로 이동
mod = importlib.import_module(module_name)
version = getattr(mod, "__version__", None)
path = getattr(mod.__spec__, "origin", None)
size = os.path.getsize(path) if path else None
# 자기 자신의 인스턴스를 생성하여 반환
return cls(
db_type=db_type,
is_installed=True,
driver_name=module_name,
driver_version=version,
driver_size_bytes=size,
)# As-is
info = DriverInfo(
db_type=db_type,
is_installed=True,
driver_name=module_name,
driver_version=version,
driver_size_bytes=size,
)
# To-be
info = DriverInfo.from_module(db_type, module_name)18c5fdd to
a5d622a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기에 왜 저렇게 줄바꿈이 여러개 생겼는지 모르겠네요. 참 이상허네.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀 주신 그대로 수정하였습니다.
app/api/connect_driver.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app/assets/driver_enum.py에 파일을 빼서 작성하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enum 이름(db_type) = 값(driver_name) 단일화 성공하였으나,
만약 이 상태에서 app/api/connect_driver.py def read_driver_info(driverId: str): 부분을 def read_driver_info(driverId: DBTypesEnum): 으로 사용할 경우 오류가 납니다. 왜냐면 enum은 이름과 값이 같아야 유효성 검사를 하기 때문이라고 합니다.
app/api/connect_driver.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return ResponseMessage.success(
value=db_driver_info(DriverInfo.from_driver_info(db_type=db_type_enum.name, driver_name=db_type_enum.value))
해당 부분 메서드 사용으로 객체로 넘기는 쪽으로 수정 완료했습니다.
JIRA Task 🔖
작업 내용 📌
기존 DB 받고 드라이버 설치 여부 확인 기능을 제거하고 DB 드라이버 의존성에 추가하여 드라이버 확인 하지 않고 정보 리턴 후 DB 연결
테스트 방법 🧑🏻🔬
작성한 테스트 코드가 실제 애플리케이션 코드의 어느 부분을 실행했는지 측정하는 방법을 안내합니다.
참고 사항 📂