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

mp4以外の動画形式も許容する #1083

Merged
merged 12 commits into from
Dec 30, 2024
Merged

mp4以外の動画形式も許容する #1083

merged 12 commits into from
Dec 30, 2024

Conversation

ikura-hamu
Copy link
Member

@ikura-hamu ikura-hamu commented Dec 28, 2024

This pull request includes changes to add support for new video types (m4v and mkv) across the API and corresponding handler functions. The changes include updates to the OpenAPI specification, domain values, handler functions, and tests.

OpenAPI Specification Updates:

  • Added video/m4v and video/mkv to the list of supported MIME types in the OpenAPI specification (docs/openapi/v2.yaml).

Domain Values Updates:

  • Added GameVideoTypeM4v and GameVideoTypeMkv constants to the GameVideoType enumeration (src/domain/values/game_video.go).

Handler Function Updates:

  • Introduced convertVideoType function to handle conversion of GameVideoType to GameVideoMime and updated handler functions to use this new function (src/handler/v2/game_video.go). [1] [2] [3] [4]

Test Updates:

  • Added new test cases to cover the new video types (m4v and mkv) and updated existing test cases to reflect these changes (src/handler/v2/game_video_test.go). [1] [2] [3] [4] [5] [6]

Generated Code Updates:

  • Updated the generated OpenAPI code to include the new MIME types (src/handler/v2/openapi/openapi.gen.go).

fix: #668

m4vとmkvを許容した。

  • 🩹 domainにm4vとmkvを追加
  • ✨ serviceのSaveGameVideoでm4vとmkvに対応
  • ✅ m4vとmkvのテストを追加
  • 🩹 OpenAPIにm4vとmkvを追加
  • ✨ handlerでmp4以外に対応
  • ✨ migrationでgame_video_typesにmkvとm4vを追加
  • ✨ repositoryでm4vとmkvに対応
  • 🩹 repositoryのGetGameVideoとGetGameVideosで動画の種類を変換する処理を追加
  • 🩹 handlerで動画のタイプmkv,m4vに対応する
  • ✅ formatが不正な場合は400を返すテスト追加

@ikura-hamu ikura-hamu requested a review from Copilot December 28, 2024 09:47
@ikura-hamu ikura-hamu linked an issue Dec 28, 2024 that may be closed by this pull request

Choose a reason for hiding this comment

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

Copilot reviewed 7 out of 14 changed files in this pull request and generated no comments.

Files not reviewed (7)
  • docs/openapi/v2.yaml: Language not supported
  • src/repository/gorm2/v2_game_video_test.go: Evaluated as low risk
  • src/service/v2/game_video.go: Evaluated as low risk
  • src/service/v2/game_video_test.go: Evaluated as low risk
  • src/domain/values/game_video.go: Evaluated as low risk
  • src/repository/gorm2/migrate/current.go: Evaluated as low risk
  • src/repository/gorm2/migrate/migrate.go: Evaluated as low risk
Comments suppressed due to low confidence (2)

src/handler/v2/game_video.go:38

  • [nitpick] The error message 'invalid video type' could be more specific, e.g., 'unsupported video type: %s'.
return "", errors.New("invalid video type")

src/repository/gorm2/v2_game_video.go:29

  • [nitpick] The error message 'invalid video type: %s' could be more informative. Suggest changing it to 'convertGameVideoType: invalid video type encountered: %s'.
return 0, fmt.Errorf("invalid video type: %s", migrateGameVideoType)
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

668 - Partially compliant

Fully compliant requirements:

  • 動画の種類としてm4vとmkvを新たに許容する変更が含まれている。
  • 動画の種類に応じたエラーハンドリングが追加されている。
  • OpenAPI仕様にm4vとmkvのMIMEタイプが追加されている。
  • m4vとmkvに関するテストケースが追加されている。

Not compliant requirements:

  • フロントエンドとの仕様調整がコード上では確認できない。
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Error Handling

The error handling for invalid video types in the convertVideoType function should be reviewed to ensure it covers all edge cases and provides meaningful error messages.

func convertVideoType(value values.GameVideoType) (openapi.GameVideoMime, error) {
	switch value {
	case values.GameVideoTypeMp4:
		return openapi.Videomp4, nil
	case values.GameVideoTypeM4v:
		return openapi.Videom4v, nil
	case values.GameVideoTypeMkv:
		return openapi.Videomkv, nil
	default:
		return "", errors.New("invalid video type")
	}
}
Database Query Robustness

The database query for fetching video types should be reviewed to ensure it handles cases where the video type is inactive or missing.

var videoType migrate.GameVideoTypeTable
err = db.
	Where("name = ?", videoTypeName).
	Where("active = ?", true).
	Select("id").
	Take(&videoType).Error
if err != nil {
	return fmt.Errorf("failed to get video type: %w", err)
}

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
General
Improve error message in convertVideoType to include the invalid value for better debugging

convertVideoType関数でデフォルトケースのエラーメッセージをより具体的にして、無効な値が何であるかを明示することでデバッグを容易にしてください。

src/handler/v2/game_video.go [38]

-return "", errors.New("invalid video type")
+return "", fmt.Errorf("invalid video type: %v", value)
Suggestion importance[1-10]: 9

Why: Including the invalid value in the error message provides more context, making debugging easier and improving traceability. This is a highly relevant and accurate suggestion for the convertVideoType function.

9
Enhance error message in SaveGameVideo to include the invalid video type value for better traceability

SaveGameVideo関数のデフォルトケースでエラーを返す際に、無効な動画タイプの値を含めることで、エラーの原因を特定しやすくしてください。

src/repository/gorm2/v2_game_video.go [54]

-return fmt.Errorf("invalid video type: %d", video.GetType())
+return fmt.Errorf("invalid video type: %d (value: %v)", video.GetType(), video)
Suggestion importance[1-10]: 9

Why: Adding the invalid video type value to the error message improves traceability and helps identify the root cause of the error. This is a precise and impactful enhancement to the SaveGameVideo function.

9
Clarify error message in convertGameVideoType by including the invalid value

convertGameVideoType関数でデフォルトケースのエラーメッセージに無効な値を含め、エラーの詳細を明確にしてください。

src/repository/gorm2/v2_game_video.go [29]

-return 0, fmt.Errorf("invalid video type: %s", migrateGameVideoType)
+return 0, fmt.Errorf("invalid video type: %s (value: %v)", migrateGameVideoType, migrateGameVideoType)
Suggestion importance[1-10]: 9

Why: Including the invalid value in the error message makes it easier to debug issues related to unsupported video types. This suggestion is accurate and improves the clarity of the convertGameVideoType function.

9
Include invalid file extension in error message for better debugging in SaveGameVideo

SaveGameVideo関数でデフォルトケースのエラー処理において、無効なファイル拡張子をエラーメッセージに含めることで、問題の特定を容易にしてください。

src/service/v2/game_video.go [83]

-return service.ErrInvalidFormat
+return fmt.Errorf("invalid file format: %s", fType.Extension)
Suggestion importance[1-10]: 9

Why: Adding the invalid file extension to the error message provides more context, making it easier to debug issues related to unsupported file formats. This is a highly relevant and effective improvement for the SaveGameVideo function.

9

Copy link

codecov bot commented Dec 28, 2024

Codecov Report

Attention: Patch coverage is 86.11111% with 10 lines in your changes missing coverage. Please review.

Project coverage is 50.01%. Comparing base (8d846d7) to head (1211314).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
src/repository/gorm2/v2_game_video.go 72.00% 5 Missing and 2 partials ⚠️
src/repository/gorm2/migrate/v14.go 83.33% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1083      +/-   ##
==========================================
+ Coverage   49.85%   50.01%   +0.16%     
==========================================
  Files         121      122       +1     
  Lines       10986    11026      +40     
==========================================
+ Hits         5477     5515      +38     
- Misses       5171     5174       +3     
+ Partials      338      337       -1     

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

@ikura-hamu ikura-hamu merged commit a52a8bc into main Dec 30, 2024
10 checks passed
@ikura-hamu ikura-hamu deleted the fix/movie_other branch December 30, 2024 12:34
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.

mp4のふりをしたmp4じゃない動画を弾いている
1 participant