Skip to content

Add search endpoint logic.#141

Merged
yu23ki14 merged 12 commits intomainfrom
feature/138
Feb 6, 2025
Merged

Add search endpoint logic.#141
yu23ki14 merged 12 commits intomainfrom
feature/138

Conversation

@yu23ki14
Copy link
Collaborator

@yu23ki14 yu23ki14 commented Jan 26, 2025

NoteとPostを統合的に検索する /api/v1/data/search を実装。

変更点

  • APIディレクトリにエンドポイントルートを追加

@router.get("/search", description=V1DataSearchDocs.description, response_model=SearchResponse)

  • Commonディレクトリに実際の検索ロジックと、Paginationのためのカウントロジックを追加

def search_notes_with_posts(

def count_search_results(

テストコードは ./tests/test_search.py に記述

メモ

  • デプロイ環境でテストするために、productionの時にcommonのブランチをfeature/138にしているので、マージする前に変更する必要がある。

@yu23ki14 yu23ki14 requested a review from osoken January 27, 2025 00:05
]
prod=[
"birdxplorer_common @ git+https://github.com/codeforjapan/BirdXplorer.git@main#subdirectory=common",
"birdxplorer_common @ git+https://github.com/codeforjapan/BirdXplorer.git@feature/138#subdirectory=common",
Copy link
Collaborator

Choose a reason for hiding this comment

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

ここはもとに戻してからマージですね。

Copy link
Collaborator

Choose a reason for hiding this comment

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

このコメントアウトは意図通りですか?

Copy link
Collaborator

Choose a reason for hiding this comment

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

search_notes_with_postscount_search_results のフィルタを適用する部分は private 関数に切り出してまとめてもいいかもしれません。(他のところにもありますので、一気にまとめたくなるかもしれませんが、一旦これだけやる想定です)

Copy link
Collaborator

@sushichan044 sushichan044 left a comment

Choose a reason for hiding this comment

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

@yu23ki14
予期せぬ挙動を起こす可能性があるため、修正したほうが良さそうな部分についてコメントしました 🙏

Comment on lines +513 to +522
# Generate pagination URLs
base_url = str(request.url).split("?")[0]
next_offset = offset + limit
prev_offset = max(offset - limit, 0)
next_url = None
if next_offset < total_count:
next_url = f"{base_url}?offset={next_offset}&limit={limit}"
prev_url = None
if offset > 0:
prev_url = f"{base_url}?offset={prev_offset}&limit={limit}"
Copy link
Collaborator

@sushichan044 sushichan044 Feb 5, 2025

Choose a reason for hiding this comment

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

[MUST] (Viewer ユーザー影響あり)
?post_includes_text=地震&offset=0&limit=4 のように limit / offset 以外が指定されたクエリの場合、
next が ?offset=4&limit=4 となってしまうので urllib.parse を使うなどして現在のクエリをマージする実装にしないと予想外の挙動になりそうです!

Comment on lines +453 to +456
if note_created_at_from is not None and isinstance(note_created_at_from, str):
note_created_at_from = ensure_twitter_timestamp(note_created_at_from)
if note_created_at_to is not None and isinstance(note_created_at_to, str):
note_created_at_to = ensure_twitter_timestamp(note_created_at_to)
Copy link
Collaborator

Choose a reason for hiding this comment

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

[MUST]
ensure_twitter_timestamp に現在時刻より未来の値を渡すと
OverflowError: signed integer is greater than maximum が出ていそうです。

これは入力されうる異常値なので、ハンドリングして DB クエリに 異常値を渡さないように変更したほうが良さそうです。

[imo]
ensure_twitter_timestamp からは一般的な Error を投げて API Handler 側で HTTPException を投げ直すのが見通しが良さそうに思いました!

@yu23ki14 yu23ki14 merged commit d4b497f into main Feb 6, 2025
3 checks passed
@ayuki-joto ayuki-joto deleted the feature/138 branch August 18, 2025 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants