Skip to content
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

developブランチをmainに取り込む #993

Merged
merged 107 commits into from
Sep 23, 2024
Merged

developブランチをmainに取り込む #993

merged 107 commits into from
Sep 23, 2024

Conversation

ikura-hamu
Copy link
Member

@ikura-hamu ikura-hamu commented Sep 23, 2024

User description

APIの後方互換性の問題から新しいAPIをmainに入れられなかったが、APIの仕様を変えて互換性を保つようにしたので、入れても問題ない


PR Type

enhancement, tests


Description

  • ゲームの可視性とジャンルに関するテストケースを追加し、テストの精度を向上。
  • CreateGame関数にジャンルの処理を追加し、ゲーム作成時のジャンル管理を強化。
  • ゲーム取得関数に可視性とジャンルのフィルタリングを追加し、取得条件を柔軟に。
  • 新しいチェッカー関数NotImplementedCheckerを追加し、未実装の可視性チェックを処理。

Changes walkthrough 📝

Relevant files
Tests
2 files
game_test.go
ゲームの可視性とジャンルに関するテストの追加と更新                                                               

src/handler/v2/game_test.go

  • 新しいテストケースを追加し、ゲームの可視性とジャンルの処理を確認
  • テストケースの説明を日本語に変更
  • テストの期待される結果を更新
  • +961/-464
    game_genre_test.go
    ゲームジャンルに関する新しいテスト関数の追加                                                                     

    src/repository/gorm2/game_genre_test.go

  • TestGetGameGenresWithNames関数を追加
  • TestSaveGameGenres関数を追加
  • TestRegisterGenresToGame関数を追加
  • +768/-189
    Enhancement
    2 files
    game.go
    ゲーム作成と取得におけるジャンルと可視性の処理追加                                                               

    src/service/v2/game.go

  • CreateGame関数にジャンルの処理を追加
  • ゲーム取得関数に可視性とジャンルのフィルタリングを追加
  • エラーハンドリングを強化
  • +150/-48
    checker.go
    可視性に関するチェッカーの追加                                                                                   

    src/handler/v2/checker.go

    • 新しいチェッカー関数NotImplementedCheckerを追加
    • 可視性に関するチェッカーを追加
    +9/-0     
    Additional files (token-limit)
    47 files
    v2_game_test.go
    ...                                                                                                           

    src/repository/gorm2/v2_game_test.go

    ...

    +670/-600
    game_test.go
    ...                                                                                                           

    src/service/v2/game_test.go

    ...

    +767/-151
    game_management_role_test.go
    ...                                                                                                           

    src/repository/gorm2/game_management_role_test.go

    ...

    +168/-89
    game_genre_test.go
    ...                                                                                                           

    src/handler/v2/game_genre_test.go

    ...

    +511/-2 
    v2_game_version_test.go
    ...                                                                                                           

    src/repository/gorm2/v2_game_version_test.go

    ...

    +91/-12 
    v2_game.go
    ...                                                                                                           

    src/repository/gorm2/v2_game.go

    ...

    +152/-74
    game.go
    ...                                                                                                           

    src/handler/v2/game.go

    ...

    +159/-39
    wire_gen.go
    ...                                                                                                           

    src/wire/wire_gen.go

    ...

    +41/-92 
    game_genre_test.go
    ...                                                                                                           

    src/service/v2/game_genre_test.go

    ...

    +252/-1 
    v2_game_image_test.go
    ...                                                                                                           

    src/repository/gorm2/v2_game_image_test.go

    ...

    +68/-35 
    v2_game_video_test.go
    ...                                                                                                           

    src/repository/gorm2/v2_game_video_test.go

    ...

    +62/-29 
    game_genre.go
    ...                                                                                                           

    src/handler/v2/game_genre.go

    ...

    +135/-8 
    v2_game_file_test.go
    ...                                                                                                           

    src/repository/gorm2/v2_game_file_test.go

    ...

    +63/-15 
    game_genre.go
    ...                                                                                                           

    src/repository/gorm2/game_genre.go

    ...

    +154/-0 
    v11.go
    ...                                                                                                           

    src/repository/gorm2/migrate/v11.go

    ...

    +139/-0 
    v2_game.go
    ...                                                                                                           

    src/service/v2_game.go

    ...

    +26/-5   
    game_genre.go
    ...                                                                                                           

    src/service/v2/game_genre.go

    ...

    +89/-1   
    v13.go
    ...                                                                                                           

    src/repository/gorm2/migrate/v13.go

    ...

    +85/-0   
    v2_game.go
    ...                                                                                                           

    src/repository/v2_game.go

    ...

    +32/-12 
    v12.go
    ...                                                                                                           

    src/repository/gorm2/migrate/v12.go

    ...

    +66/-0   
    service.go
    ...                                                                                                           

    src/wire/service.go

    ...

    +25/-22 
    api.go
    ...                                                                                                           

    src/handler/api.go

    ...

    +16/-21 
    game_genre.go
    ...                                                                                                           

    src/repository/game_genre.go

    ...

    +22/-0   
    current.go
    ...                                                                                                           

    src/repository/gorm2/migrate/current.go

    ...

    +20/-13 
    repository.go
    ...                                                                                                           

    src/wire/repository.go

    ...

    +14/-14 
    handler.go
    ...                                                                                                           

    src/wire/handler.go

    ...

    +18/-18 
    game_image_test.go
    ...                                                                                                           

    src/service/v2/game_image_test.go

    ...

    +4/-4     
    game_genre.go
    ...                                                                                                           

    src/service/game_genre.go

    ...

    +14/-0   
    users_test.go
    ...                                                                                                           

    src/handler/v2/users_test.go

    ...

    +5/-5     
    game.go
    ...                                                                                                           

    src/domain/game.go

    ...

    +31/-0   
    game_version_test.go
    ...                                                                                                           

    src/service/v2/game_version_test.go

    ...

    +3/-3     
    game_image.go
    ...                                                                                                           

    src/service/v2/game_image.go

    ...

    +2/-3     
    v10.go
    ...                                                                                                           

    src/repository/gorm2/migrate/v10.go

    ...

    +4/-4     
    migrate.go
    ...                                                                                                           

    src/repository/gorm2/migrate/migrate.go

    ...

    +3/-0     
    game_version.go
    ...                                                                                                           

    src/service/v2/game_version.go

    ...

    +3/-4     
    errors.go
    ...                                                                                                           

    src/repository/errors.go

    ...

    +6/-4     
    game.go
    ...                                                                                                           

    src/domain/values/game.go

    ...

    +7/-0     
    v2_game_version.go
    ...                                                                                                           

    src/repository/gorm2/v2_game_version.go

    ...

    +8/-0     
    db.go
    ...                                                                                                           

    src/repository/gorm2/db.go

    ...

    +1/-1     
    errors.go
    ...                                                                                                           

    src/service/errors.go

    ...

    +2/-0     
    v2.yaml
    ...                                                                                                           

    docs/openapi/v2.yaml

    ...

    +97/-7   
    game_genre_relations.md
    ...                                                                                                           

    docs/db_schema/game_genre_relations.md

    ...

    +14/-4   
    README.md
    ...                                                                                                           

    docs/db_schema/README.md

    ...

    +4/-3     
    .tbls.yml
    ...                                                                                                           

    .tbls.yml

    ...

    +18/-0   
    game_visibility_types.md
    ...                                                                                                           

    docs/db_schema/game_visibility_types.md

    ...

    +50/-0   
    games.md
    ...                                                                                                           

    docs/db_schema/games.md

    ...

    +7/-1     
    game_genres.md
    ...                                                                                                           

    docs/db_schema/game_genres.md

    ...

    +5/-3     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    ikura-hamu and others added 30 commits October 15, 2023 17:34
    Co-authored-by: ikura-hamu <ikura-hamu@users.noreply.github.com>
    Co-authored-by: ikura-hamu <ikura-hamu@users.noreply.github.com>
    🔥 visibility追加、v1API削除
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    マイグレーションロジック
    マイグレーションのロールバック処理で、gameTable2V12gameGenreTableV12のAutoMigrateを呼び出していますが、これらの構造体は定義されていないようです。適切なバージョンのテーブル構造体を使用するか、存在しない場合は定義する必要があります。

    マイグレーションID
    マイグレーションIDが '13' と設定されていますが、他のマイグレーションファイルとIDが重複していないか確認してください。重複するとマイグレーションの実行順序に影響を与える可能性があります。

    Copy link

    github-actions bot commented Sep 23, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    パラメータが nil の場合にパニックを防ぐための条件分岐を追加する。

    **GetMyGames メソッドの呼び出しで、testCase.params.LimittestCase.params.Offset の値が nil の場合に
    int(testCase.params.Limit)int(testCase.params.Offset) でパニックが発生する可能性があります。これらの値が
    nil でないことを確認するか、デフォルト値を設定するように修正してください。

    src/handler/v2/game_test.go [476-481]

    -mockGameService.
    -  EXPECT().
    -  GetMyGames(
    -    gomock.Any(), gomock.Not(gomock.Nil()),
    -    int(*testCase.params.Limit), int(*testCase.params.Offset), gomock.Any(),
    -    gomock.InAnyOrder([]values.GameVisibility{values.GameVisibilityTypeLimited, values.GameVisibilityTypePrivate, values.GameVisibilityTypePublic}),
    -    gomock.Any(), gomock.Any()).
    -  Return(testCase.gamesNumber, testCase.games, testCase.GetMyGamesErr)
    +if testCase.params.Limit != nil && testCase.params.Offset != nil {
    +  mockGameService.
    +    EXPECT().
    +    GetMyGames(
    +      gomock.Any(), gomock.Not(gomock.Nil()),
    +      int(*testCase.params.Limit), int(*testCase.params.Offset), gomock.Any(),
    +      gomock.InAnyOrder([]values.GameVisibility{values.GameVisibilityTypeLimited, values.GameVisibilityTypePrivate, values.GameVisibilityTypePublic}),
    +      gomock.Any(), gomock.Any()).
    +    Return(testCase.gamesNumber, testCase.games, testCase.GetMyGamesErr)
    +}
     
    Suggestion importance[1-10]: 10

    Why: This suggestion addresses a critical bug where dereferencing a nil pointer could cause a panic. Adding a nil check is essential to ensure the stability of the code.

    10
    nil チェックを追加してパニックを防ぐ。

    assert.Len 関数を使用して games[i].Genres の長さを検証していますが、games[i].Genres または
    testCase.apiGames.Games[i].Genresnil の場合にパニックが発生する可能性があります。nil
    チェックを追加することをお勧めします。

    src/handler/v2/game_test.go [523]

    -assert.Len(t, *games[i].Genres, len(*testCase.apiGames.Games[i].Genres))
    +if games[i].Genres != nil && testCase.apiGames.Games[i].Genres != nil {
    +  assert.Len(t, *games[i].Genres, len(*testCase.apiGames.Games[i].Genres))
    +}
     
    Suggestion importance[1-10]: 10

    Why: The suggestion is crucial as it prevents a potential panic by adding a nil check before asserting the length, ensuring the test does not fail unexpectedly.

    10
    モックの呼び出しをフラグの値に基づいて条件分岐させることで、意図しないモックの呼び出しを防ぐ。

    executeGetGamesexecuteGetMyGames のフラグを使って、GetGamesGetMyGames
    の呼び出しを制御していますが、これらのフラグが false の場合にも mockGameService.EXPECT()
    が呼び出されてしまいます。これは意図しないモックの呼び出しを引き起こす可能性があります。executeGetGamesexecuteGetMyGames
    の値に基づいて、モックのセットアップを条件分岐させることをお勧めします。

    src/handler/v2/game_test.go [465-470]

    -mockGameService.
    -  EXPECT().
    -  GetGames(
    -    gomock.Any(), limit, offset,
    -    gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
    -  Return(testCase.gamesNumber, testCase.games, testCase.GetGamesErr)
    +if testCase.executeGetGames {
    +  mockGameService.
    +    EXPECT().
    +    GetGames(
    +      gomock.Any(), limit, offset,
    +      gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
    +    Return(testCase.gamesNumber, testCase.games, testCase.GetGamesErr)
    +}
     
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a potential bug where mock expectations are set regardless of the flags' values, which could lead to unintended mock calls. Implementing the suggestion would prevent this issue.

    9
    Security
    認証されていないユーザーのアクセス制限を強化する


    GetGames関数内でauthSessionnilの場合にisAllfalseに設定することを検討してください。これにより、認証されていないユーザーがすべてのゲーム情報にアクセスするのを防ぐことができます。

    src/handler/v2/game.go [44-49]

     if params.All != nil && authSession != nil {
         isAll = *params.All
     } else {
    -    isAll = true
    +    isAll = false // 認証されていないユーザーは限定された情報のみアクセス可能
     }
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential security issue by ensuring that unauthenticated users cannot access all game information, which is crucial for maintaining data privacy.

    9
    認証されていないユーザーへのアクセス制御を強化する


    GetGame関数で、認証セッションが存在しない場合にエラーを返さずに処理を続行するように変更していますが、この変更によりセキュリティリスクが発生する可能性があります。認証されていないユーザーに対してどの情報を公開するかを慎重に検討し、必要に応じてアクセス制御を強化してください。

    src/handler/v2/game.go [324-327]

     authSession, err := g.session.getAuthSession(session)
     if err != nil {
    -    // 部員以外でも、管理者情報以外は取得できるようにするので、エラーは返さない。
    -    authSession = nil
    +    // 認証されていないユーザーには限定的な情報のみ提供
    +    return echo.NewHTTPError(http.StatusUnauthorized, "認証が必要です")
     }
     
    Suggestion importance[1-10]: 8

    Why: Strengthening access control for unauthenticated users is important for security, ensuring that sensitive information is not exposed inadvertently.

    8
    Enhancement
    ゲームの可視性のデフォルト値を設定する


    ゲームの可視性を設定する際に、GameVisibilityのデフォルト値を設定することを検討してください。これにより、明示的に可視性が設定されていない場合にデフォルトの可視性が適用されます。

    src/domain/game.go [13]

    -visibility  values.GameVisibility
    +visibility  values.GameVisibility = DefaultVisibility // DefaultVisibilityは適切なデフォルト値に置き換えてください
     
    Suggestion importance[1-10]: 7

    Why: Setting a default value for game visibility can improve code robustness by ensuring that a default visibility is applied when none is explicitly set. However, the suggestion lacks specific implementation details for the default value.

    7
    t.Fatalft.Errorに変更して、テストの継続を可能にします。


    common.NewSessionのエラーハンドリングを改善して、t.Fatalfの代わりにt.Errorを使用し、テストの実行を続けることができるようにします。これにより、他のテストケースも実行され、より多くの情報を得ることができます。

    src/handler/v2/game_genre_test.go [141-144]

     sess, err := common.NewSession(mockConf)
     if err != nil {
    -  t.Fatalf("failed to create session: %v", err)
    +  t.Error("failed to create session:", err)
       return
     }
     
    Suggestion importance[1-10]: 7

    Why: Changing t.Fatalf to t.Error allows the test suite to continue running other tests even if one fails, which can provide more comprehensive test results. However, it may not be suitable for all scenarios where immediate termination is desired.

    7
    レスポンスのデコードでエラーハンドリングを追加して、テストの堅牢性を向上させます。


    json.NewDecoderを使用してレスポンスのデコードを行う際に、エラーハンドリングを追加して、デコード失敗時にテストが適切に失敗するようにします。これにより、デコードの問題を早期に発見し、より堅牢なテストが可能になります。

    src/handler/v2/game_genre_test.go [280-282]

     err = json.NewDecoder(rec.Body).Decode(&res)
     if err != nil {
    -  t.Fatalf("failed to decode response: %v", err)
    +  t.Error("failed to decode response:", err)
    +  return
     }
     
    Suggestion importance[1-10]: 7

    Why: Adding error handling with t.Error instead of t.Fatalf allows the test to continue running, which can be beneficial for identifying multiple issues in a single test run. However, it may not be suitable if immediate termination is required.

    7
    ソートタイプの値が有効であることを検証する。

    sortTypeCreatedAtsortTypeLatestVersion の値を設定していますが、これらの値が
    openapi.GetGamesParamsSort
    型の有効な値であることを確認していません。不正な値が設定されるとAPIの動作が期待通りでなくなる可能性があります。値の検証を追加することをお勧めします。

    src/handler/v2/game_test.go [88-89]

     sortTypeCreatedAt := openapi.CreatedAt
    +if !isValidSortType(sortTypeCreatedAt) {
    +  log.Fatalf("Invalid sort type: %v", sortTypeCreatedAt)
    +}
     sortTypeLatestVersion := openapi.LatestVersion
    +if !isValidSortType(sortTypeLatestVersion) {
    +  log.Fatalf("Invalid sort type: %v", sortTypeLatestVersion)
    +}
     
    Suggestion importance[1-10]: 7

    Why: While the suggestion enhances the code by adding validation for sort types, it addresses a potential issue rather than a critical bug. It improves robustness but is not immediately necessary for functionality.

    7
    エラーメッセージの明確化


    PostGame関数内でジャンル名の検証を行う際に、エラーメッセージをもっと具体的にすることを検討してください。これにより、APIの使用者がエラーの原因をより容易に理解できるようになります。

    src/handler/v2/game.go [213-221]

     if errors.Is(err, values.ErrGameGenreNameEmpty) {
    -    return echo.NewHTTPError(http.StatusBadRequest, "game genre name is empty")
    +    return echo.NewHTTPError(http.StatusBadRequest, "ジャンル名が空です")
     }
     if errors.Is(err, values.ErrGameGenreNameTooLong) {
    -    return echo.NewHTTPError(http.StatusBadRequest, "game genre name is too long")
    +    return echo.NewHTTPError(http.StatusBadRequest, "ジャンル名が長すぎます")
     }
     if err != nil {
    -    log.Printf("failed to validate game genre name: %v\n", genreName)
    -    return echo.NewHTTPError(http.StatusInternalServerError, "failed to validate game genre name")
    -}
    +    log.Printf("ジャンル名の検証に失敗しました: %v\n", genreName)
    +    return echo.NewHTTPError(http.StatusInternalServerError, "ジャンル名の検証に失敗しました")
     
    Suggestion importance[1-10]: 5

    Why: Improving error messages enhances user experience by making it easier to understand the cause of errors, but it is a minor enhancement in terms of functionality.

    5
    Possible issue
    リクエストボディの生成方法を修正して、テストの意図を正確に反映させます。


    strings.NewReaderの使用を見直し、正しいフォーマットでリクエストボディを作成するようにします。これにより、テストの意図に合ったリクエストが生成され、テストの正確性が向上します。

    src/handler/v2/game_genre_test.go [509-511]

    -_, err := strings.NewReader("invalid request body").WriteTo(reqBody)
    +_, err := strings.NewReader(`{"invalid": "request body"}`).WriteTo(reqBody)
     if err != nil {
    -  t.Fatalf("failed to write to request body: %v", err)
    +  t.Error("failed to write to request body:", err)
    +  return
     }
     
    Suggestion importance[1-10]: 6

    Why: The suggestion improves the readability and correctness of the test by using a valid JSON format for the request body, which aligns better with typical API request structures.

    6

    Copy link

    codecov bot commented Sep 23, 2024

    Codecov Report

    Attention: Patch coverage is 70.60932% with 82 lines in your changes missing coverage. Please review.

    Project coverage is 50.64%. Comparing base (32e624c) to head (8ea56c3).
    Report is 108 commits behind head on main.

    Files with missing lines Patch % Lines
    src/handler/v2/checker.go 10.52% 17 Missing ⚠️
    src/domain/game.go 0.00% 13 Missing ⚠️
    src/handler/v2/openapi/openapi.gen.go 0.00% 13 Missing ⚠️
    src/handler/v2/game.go 91.20% 9 Missing and 2 partials ⚠️
    src/handler/v2/game_genre.go 89.28% 7 Missing and 2 partials ⚠️
    src/handler/api.go 0.00% 8 Missing ⚠️
    src/repository/gorm2/edition.go 0.00% 5 Missing ⚠️
    src/cache/ristretto/seat.go 0.00% 2 Missing ⚠️
    src/handler/v2/edition_auth.go 0.00% 2 Missing ⚠️
    src/handler/v2/api.go 0.00% 1 Missing ⚠️
    ... and 1 more
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##             main     #993      +/-   ##
    ==========================================
    - Coverage   55.09%   50.64%   -4.46%     
    ==========================================
      Files         159      120      -39     
      Lines       11694     8649    -3045     
    ==========================================
    - Hits         6443     4380    -2063     
    + Misses       4829     3951     -878     
    + Partials      422      318     -104     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @ikura-hamu ikura-hamu merged commit ee77dc7 into main Sep 23, 2024
    9 of 10 checks passed
    @ikura-hamu ikura-hamu deleted the develop branch September 23, 2024 07:10
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants