Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
],
"cSpell.words": [
"embla",
"joinedload",
"shadcn",
"shinkansen"
],
Expand Down
39 changes: 33 additions & 6 deletions server/app/cruds/blocks.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from sqlalchemy import select
from sqlalchemy.ext.asyncio import AsyncSession
from sqlalchemy.orm import selectinload
from sqlalchemy.orm import joinedload

from app.cruds import locations as locations_cruds
from app.models import Block
Expand All @@ -9,9 +9,25 @@


def _block_with_relations():
"""Block を location / destination_location と一緒に 1 クエリで取る。

どちらも 1-to-1 なので joinedload で重複が増えない。
"""
return select(Block).options(
joinedload(Block.location),
joinedload(Block.destination_location),
)


def _block_with_page_and_relations():
"""Block + Page (trip_id 取得用) + location / destination_location を 1 クエリで取る。

認可 (Page.trip_id が必要) と本体取得を 1 ラウンドトリップに統合するため。
"""
return select(Block).options(
selectinload(Block.location),
selectinload(Block.destination_location),
joinedload(Block.page),
joinedload(Block.location),
joinedload(Block.destination_location),
)


Expand Down Expand Up @@ -62,8 +78,8 @@ async def create_block(db: AsyncSession, block: BlockCreate, page_id: int) -> Bl
stmt = (
select(Block)
.options(
selectinload(Block.location),
selectinload(Block.destination_location)
joinedload(Block.location),
joinedload(Block.destination_location),
)
.where(Block.id == db_block.id)
)
Expand All @@ -84,7 +100,7 @@ async def find_blocks(db: AsyncSession, page_id: int) -> list[Block]:
list[Block]: ブロックリスト
"""
result = await db.execute(
_block_with_relations().where(Block.page_id == page_id)
_block_with_relations().where(Block.page_id == page_id).order_by(Block.id)
)
return list(result.scalars().all())

Expand All @@ -104,6 +120,17 @@ async def get_block(db: AsyncSession, block_id: int) -> Block | None:
return result.scalar_one_or_none()


async def get_block_with_page(db: AsyncSession, block_id: int) -> Block | None:
"""ブロックを Page (trip_id 用) ごと 1 クエリで取得する。

ルーターで認可 + 本体取得を 1 ラウンドトリップで済ませるためのヘルパー。
"""
result = await db.execute(
_block_with_page_and_relations().where(Block.id == block_id)
)
return result.scalar_one_or_none()


async def _replace_block_location(
db: AsyncSession,
db_block: Block,
Expand Down
18 changes: 11 additions & 7 deletions server/app/cruds/pages.py
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
from sqlalchemy import select
from sqlalchemy.ext.asyncio import AsyncSession
from sqlalchemy.orm import selectinload
from sqlalchemy.orm import joinedload

from app.models import Block, Page
from app.schemas.page import PageCreate, PageUpdate


def _page_with_relations():
"""Page → Blocks → Locations を 1 クエリの LEFT JOIN で取得する。

1-to-many の連鎖により重複行が出るので、呼び出し側で `result.unique()` を挟む。
"""
return select(Page).options(
selectinload(Page.blocks).options(
selectinload(Block.location),
selectinload(Block.destination_location),
joinedload(Page.blocks).options(
joinedload(Block.location),
joinedload(Block.destination_location),
)
)

Expand Down Expand Up @@ -45,9 +49,9 @@ async def find_pages(db: AsyncSession, trip_id: int) -> list[Page]:
list[Page]: ページリスト
"""
result = await db.execute(
_page_with_relations().where(Page.trip_id == trip_id)
_page_with_relations().where(Page.trip_id == trip_id).order_by(Page.id)
)
return list(result.scalars().all())
return list(result.unique().scalars().all())


async def get_page(db: AsyncSession, page_id: int) -> Page | None:
Expand All @@ -62,7 +66,7 @@ async def get_page(db: AsyncSession, page_id: int) -> Page | None:
Page | None: 特定のページ、見つからない場合はNone
"""
result = await db.execute(_page_with_relations().where(Page.id == page_id))
return result.scalar_one_or_none()
return result.unique().scalar_one_or_none()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

概要

get_page_page_with_relations() を使用して常に blockslocationsjoinedload していますが、これが update_page(ページの更新処理)内でも呼び出されているため、更新時にも不要な関連データの取得(LEFT JOIN)が走ってしまいます。

詳細

ページの更新処理(update_page)では、通常 Page のスカラー値(titledate)のみを更新するため、関連データをロードする必要はありません。

対策案

update_page 内では、関連データを含まないシンプルなクエリで Page を取得するように変更することを推奨します。

async def update_page(db: AsyncSession, page_id: int, page: PageUpdate) -> Page | None:
    # 関連データを含まないシンプルな取得
    stmt = select(Page).where(Page.id == page_id)
    result = await db.execute(stmt)
    db_page = result.scalar_one_or_none()
    
    if db_page:
        for key, value in page.model_dump().items():
            setattr(db_page, key, value)
        await db.commit()

    return db_page
References
  1. リポジトリスタイルガイドの「パフォーマンス」原則(コードは効率的であるべき)に基づき、更新処理における不要な関連データのロードを避ける設計に改善します。 (link)



async def update_page(db: AsyncSession, page_id: int, page: PageUpdate) -> Page | None:
Expand Down
23 changes: 15 additions & 8 deletions server/app/cruds/trips.py
Original file line number Diff line number Diff line change
@@ -1,16 +1,23 @@
from sqlalchemy import select
from sqlalchemy.ext.asyncio import AsyncSession
from sqlalchemy.orm import selectinload
from sqlalchemy.orm import joinedload

from app.models import Block, Page, Trip
from app.schemas.trip import TripCreateIn, TripUpdate


def _trip_with_relations():
"""Trip → Pages → Blocks → Locations を 1 クエリの LEFT JOIN で取得する。

1-to-many を joinedload で連鎖させるため重複行が発生する。呼び出し側で
`result.unique()` を挟むこと。クエリ数を 4 から 1 に減らす目的。
"""
return select(Trip).options(
selectinload(Trip.pages).selectinload(Page.blocks).options(
selectinload(Block.location),
selectinload(Block.destination_location),
joinedload(Trip.pages)
.joinedload(Page.blocks)
.options(
joinedload(Block.location),
joinedload(Block.destination_location),
)
)

Expand Down Expand Up @@ -43,8 +50,8 @@ async def find_trips(db: AsyncSession) -> list[Trip]:
Returns:
list[Trip]: すべての旅行プラン
"""
result = await db.execute(_trip_with_relations())
return list(result.scalars().all())
result = await db.execute(_trip_with_relations().order_by(Trip.id))
return list(result.unique().scalars().all())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

概要

find_trips において、すべての旅行プラン(Trip)を一覧取得する際にも _trip_with_relations() を使用して Pages -> Blocks -> Locations までを joinedload で一括取得していますが、これはパフォーマンス上のボトルネックになる可能性があります。

詳細

  1. レスポンススキーマの確認: もし旅行プラン一覧取得 API(例: GET /trips)のレスポンスに pagesblocks の詳細が含まれていない場合、これらの大量の関連データを JOIN して取得することは無駄なクエリコストとなり、メモリやネットワーク帯域を圧迫します。
  2. データ量増加時の懸念: 全ての Trip に対して全 Page、全 Block を 1 クエリの LEFT JOIN で取得すると、レコード数が掛け算で増加し(巨大なデカルト積のような状態)、result.unique() による Python 側での重複排除処理の負荷も非常に高くなります。

対策案

一覧取得用の find_trips では、リレーションを含めないシンプルなクエリ(または必要な最小限のリレーションのみ)を使用するようにクエリを分離することを推奨します。

# 例: リレーションを含めないシンプルな一覧取得
async def find_trips(db: AsyncSession) -> list[Trip]:
    result = await db.execute(select(Trip))
    return list(result.scalars().all())
References
  1. リポジトリスタイルガイドの「パフォーマンス」原則(コードは効率的であるべき)に基づき、不要な関連データのロードを避ける設計に改善します。 (link)



async def get_trip(db: AsyncSession, trip_id: int) -> Trip | None:
Expand All @@ -59,7 +66,7 @@ async def get_trip(db: AsyncSession, trip_id: int) -> Trip | None:
Trip | None: 特定の旅行プラン、見つからない場合はNone
"""
result = await db.execute(_trip_with_relations().where(Trip.id == trip_id))
return result.scalar_one_or_none()
return result.unique().scalar_one_or_none()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

概要

get_trip_trip_with_relations() を使用して Pages -> Blocks -> Locations をすべて joinedload するようになったため、update_trip(旅行プランの更新処理)でもこの重いクエリが実行されてしまいます。

詳細

旅行プランの更新(update_trip)では Trip の基本情報(titledetail など)のみを更新するため、すべての関連データを JOIN して取得するのは非効率です。

対策案

update_trip 内では、関連データを含まないシンプルなクエリで Trip を取得するように変更することを推奨します。

async def update_trip(db: AsyncSession, trip_id: int, trip: TripUpdate) -> Trip | None:
    # 関連データを含まないシンプルな取得
    stmt = select(Trip).where(Trip.id == trip_id)
    result = await db.execute(stmt)
    db_trip = result.scalar_one_or_none()
    
    if db_trip:
        for key, value in trip.model_dump().items():
            setattr(db_trip, key, value)
        await db.commit()

    return db_trip
References
  1. リポジトリスタイルガイドの「パフォーマンス」原則(コードは効率的であるべき)に基づき、更新処理における不要な関連データのロードを避ける設計に改善します。 (link)



async def get_trip_by_url_id(db: AsyncSession, url_id: str) -> Trip | None:
Expand All @@ -74,7 +81,7 @@ async def get_trip_by_url_id(db: AsyncSession, url_id: str) -> Trip | None:
Trip | None: 特定の旅行プラン、見つからない場合はNone
"""
result = await db.execute(_trip_with_relations().where(Trip.url_id == url_id))
return result.scalar_one_or_none()
return result.unique().scalar_one_or_none()


async def update_trip(db: AsyncSession, trip_id: int, trip: TripUpdate) -> Trip | None:
Expand Down
16 changes: 12 additions & 4 deletions server/app/db_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,18 @@
# 非同期エンジンの作成
engine: AsyncEngine = create_async_engine(
settings.get_database_url(),
echo=False, # 本番環境では False
pool_pre_ping=True, # 接続の健全性チェック
pool_size=5, # 接続プールサイズ
max_overflow=10, # 最大オーバーフロー接続数
echo=False,
# Neon Free は 5 分アイドルで compute がサスペンドされ、復帰時に
# アプリ側のプール内コネクションが dead 化することがある。checkout 時の
# SELECT 1 (Pessimistic Disconnect Handling) で死活確認するのが安全。
# 1 RTT 分のコストが発生するが、クエリ最適化で減らせる RTT 数より接続
# 失敗による 500 のほうがコスト高なので有効のままにする。
pool_pre_ping=True,
pool_size=5,
max_overflow=10,
# 30 分超過した接続は次回 checkout 時にリサイクル。pool_pre_ping と
# 二重防御。短すぎると無駄な再確立が増え、長すぎると古い接続を掴むリスク。
pool_recycle=1800,
connect_args={"ssl": True} if settings.ssl_required else {},
)

Expand Down
2 changes: 2 additions & 0 deletions server/app/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ class Trip(Base):
cascade="all, delete-orphan",
lazy="raise",
passive_deletes=True,
order_by="Page.id",
)


Expand Down Expand Up @@ -117,6 +118,7 @@ class Page(Base):
cascade="all, delete-orphan",
lazy="raise",
passive_deletes=True,
order_by="Block.id",
)


Expand Down
31 changes: 23 additions & 8 deletions server/app/routers/blocks.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
from fastapi import APIRouter, Depends
from fastapi import APIRouter, Depends, Request
from sqlalchemy.ext.asyncio import AsyncSession

from app.auth import require_block_access, require_page_access
from app.auth import (
get_allowed_trip_ids,
require_block_access,
require_page_access,
)
from app.cruds import blocks as blocks_cruds
from app.cruds import pages as pages_cruds
from app.db_connection import get_db_session
from app.errors import NotFound
from app.errors import Forbidden, NotFound
from app.schemas.block import Block, BlockCreate, BlockUpdate

router = APIRouter(tags=["Blocks"])
Expand Down Expand Up @@ -39,15 +44,22 @@ async def create_block(
)
async def get_blocks(
page_id: int,
_: int = Depends(require_page_access),
request: Request,
db: AsyncSession = Depends(get_db_session),
) -> list[Block]:
"""
説明:

- 特定のページに関連するすべてのブロックを取得する
- 認可と本体取得を 1 クエリで済ませるため、Page を blocks 込みで取得し
その page.trip_id を Cookie で検証する
"""
return await blocks_cruds.find_blocks(db=db, page_id=page_id)
db_page = await pages_cruds.get_page(db=db, page_id=page_id)
if db_page is None:
raise NotFound(message="Page not found")
if db_page.trip_id not in get_allowed_trip_ids(request):
raise Forbidden()
return db_page.blocks


@router.get(
Expand All @@ -58,18 +70,21 @@ async def get_blocks(
)
async def get_block(
block_id: int,
_: int = Depends(require_block_access),
request: Request,
db: AsyncSession = Depends(get_db_session),
) -> Block:
"""
説明:

- IDで指定された単一のブロックを取得する
- 認可と本体取得を 1 クエリで済ませるため、Block を Page (trip_id 取得用) と
locations 込みで取得し、その page.trip_id を Cookie で検証する
"""
db_block = await blocks_cruds.get_block(db, block_id=block_id)
db_block = await blocks_cruds.get_block_with_page(db=db, block_id=block_id)
if db_block is None:
raise NotFound(message="Block not found")

if db_block.page.trip_id not in get_allowed_trip_ids(request):
raise Forbidden()
return db_block


Expand Down
17 changes: 12 additions & 5 deletions server/app/routers/pages.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
from fastapi import APIRouter, Depends
from fastapi import APIRouter, Depends, Request
from sqlalchemy.ext.asyncio import AsyncSession

from app.auth import require_trip_access, require_page_access
from app.auth import (
get_allowed_trip_ids,
require_page_access,
require_trip_access,
)
from app.cruds import pages as pages_cruds
from app.db_connection import get_db_session
from app.errors import NotFound
from app.errors import Forbidden, NotFound
from app.schemas.page import Page, PageCreate, PageCreateResponse, PageUpdate

# /trips/{trip_id}/pages で作成と一覧取得
Expand Down Expand Up @@ -59,18 +63,21 @@ async def get_pages(
)
async def get_page(
page_id: int,
_: int = Depends(require_page_access),
request: Request,
db: AsyncSession = Depends(get_db_session),
) -> Page:
"""
説明:

- IDで指定された単一のページを取得する
- 認可と本体取得を 1 クエリで済ませるため、Page を blocks / locations 込みで
取得し、その page.trip_id を Cookie で検証する
"""
db_page = await pages_cruds.get_page(db, page_id=page_id)
if db_page is None:
raise NotFound(message="Page not found")

if db_page.trip_id not in get_allowed_trip_ids(request):
raise Forbidden()
return db_page


Expand Down
Loading