レスポンスにサムネイル情報追加

[azbcww]

サムネイル情報をGET /api/v3/stamps のレスポンスに載るよう変更しました．
"hasThumbnailを含める"っていうのをサムネイル情報が欲しいって解釈したんですけどあってますかね．．．？

返ってくる情報は以下のようになります．
{
"id": "68df025e-8260-4a11-bd50-f1bb32692c5a",
"name": "test",
"creatorId": "52687fda-bc08-470c-83fa-67b0fe5f644d",
"fileId": "3ec736ff-857f-420d-889e-911745c23c6b",
"isUnicode": false,
"createdAt": "2023-08-12T08:02:21.547014Z",
"updatedAt": "2023-08-12T08:02:21.547014Z",
"thumbnails": [
{
"FileID": "3ec736ff-857f-420d-889e-911745c23c6b",
"Type": 1,
"Mime": "image/png",
"Width": 128,
"Height": 128
}
]
},

[ikura-hamu]

ありがとうございます。

僕はhasThumbnailというbool値をレスポンスに含めるのだと思ってましたが、 @motoki317 さんはissueを立てた時、どういうのを意図していましたか？

[motoki317]

このissueの目的は、UIの省エネモードでアニメーションのスタンプを表示しないために、サムネイルを代わりに表示したいことです
そのために現状はファイルの /files/:fid/meta をいちいち叩かないと /files/:fid/thumbnail が存在するかわからないので、サムネイルが存在するかを一気に取りたいという意図です
https://q.trap.jp/messages/93892217-e704-4104-ad00-166851650d90

なので、bool値で十分だと思います

[azbcww]

スタンプに対応したサムネイルを取得できる既存のメソッドを探したのですが見つけることができなかったので助言をいただきたいです．

やろうとしたこと
スタンプのファイルIDからファイルメタを取得してサムネイルの有無を調べようとした．

現状
GetAllStamps()でmodel.Stamp構造体の配列を取得している．

問題
上記配列にあるすべてのファイルIDからファイルメタを，DBへの問い合わせ1往復のみで行いたいが方法が見つからない．

手詰まり感あるので想定している実装方法などあればヒント欲しいです．．．

[ikura-hamu]

遅くなってごめんなさい。

忙しくてちゃんとコードを読めてないのですが、データベースでサムネイルの存在を確かめるメソッドを書く必要があると思います。

docs/dbSchema にあるデータベーススキーマと、repositroryで定義されているinterfaceとそのgormの実装を参考にして、ループ回さず必要な情報を取得できるメソッドをお願いします。

[azbcww]

返信ありがとうございます！

教えていただいたことを参考にしながらやってみます！

[azbcww]

前の変更からだいぶ時間たってしまいましたがdbから一度のクエリでスタンプのサムネイル情報を取得できるようにしました．
しかし，CIのエラーが出ており解決したいのですが見方が分からず困っています．
どこを見ればいいかおしえていただけないでしょうか？

[motoki317]

LintとTestの2つFailしてますね。一つずつ見ましょうか

Lintの方は、file is not gofmt-ed というメッセージが出ていますが、文字通り、ファイルが「gofmt」でフォーマットされてないよということです。Goには言語標準のgofmtというフォーマッタがあるので、それでフォーマットしてねということです。
https://github.com/traPtitech/traQ/actions/runs/6402762003/job/17383151638

Testの方は、↓のチェックが失敗しているようですね。APIのレスポンスとしてJSONのArrayが返ってくることを期待したけど、実際には null という文字列が返ってきているようです。

traQ/router/v3/stamps_test.go

Line 115 in 469b8db

Array()

Goでは実はsliceのゼロ値(nil)と、make([]T, 0) などで作った長さ0のsliceは違って、JSONにエンコードするときも前者は nil ]に、後者は [] になります。これを考慮してみてください
https://github.com/traPtitech/traQ/actions/runs/6402762003/job/17379996280

[motoki317]

Testの方の補足です
https://go.dev/play/p/WTdS_ZpLYfp

[azbcww]

エラー内容を教えていただきありがとうございます！
参考にして修正したらエラー消すことができました

修正完了したので内容についても時間があるときに見てほしいです
処理を書く場所や処理方法の改善があれば教えていただければと思います
よろしくお願いします！

[ikura-hamu]

ありがとうございます！実装は大丈夫だと思います。1か所だけリファクタリングお願いします。

また、今回の変更部分のテストコードを書いてほしいです。

repositoryのStampThumbnailExists()のテストは repository/gorm/stamp_test.goに書いてください。
routerのGetStamps()のテストはrouter/v3/stamps_test.goで修正を加えてください。

他のテストの関数を参考にすると書きやすいと思います。
GetStampsの方は、レスポンスボディが変わっているので、その評価を行う部分に修正が必要です。

何かわからないことがあったら聞いてください。お願いします！

[azbcww]

実装確認していただきありがとうございました！
指示もどこを修正すればいいのかわかりやすく助かりました！

指摘してくださったことを踏まえ構造体の修正とテストコードの追加を行いました．

ただテストコード自体はこれでいいのかよくわからなかったので確認していただけると助かります．
また，api/v3/stampsのレスポンスを行うメソッドであるGetStampsのレスポンス内容を変更しているのでSwaggerのレスポンス内容も書き換えた方がよいでしょうか？

時間あるときに確認していただければと思います．
よろしくお願いします．

[ikura-hamu]

ありがとうございます！

repositoryの方のテストでいくつか修正をお願いします。
routerの方はいいと思います。

そうですね、swaggerの方もレスポンスボディのフィールドの修正をお願いします。

[logica0419]

レビューフローに割り込んでしまってすみません。

「StampThumbnailExists」の名前がちょっと気持ち悪いなぁと思って色々考えてたんですけど、その途中で「そもそもGetAllStampsWithThumbnailみたいにした方がいいのでは？」という考えが浮かびました。
根本の実装方針から覆してしまいそうで申し訳ない…
StampThumbnailExistsに渡すstampsって多分GetAllStampsで取るのしか無いから、それならその2つはまとめて1つのメソッドとして確立させた方がわかりやすいのでは？という考えですね。

すでにやっているかもしれませんが、こういう時に気にすべきは、次にこのメソッドを使って別機能を実装する人のことです。
StampRepositoryはinterface、すなわちリポジトリ(データ永続化機能)への入り口ですから、使う人にとって明快で使いやすい入り口を用意するという目線で検討してほしいと思います。

@azbcww @ikura-hamu の2人の意見も聞きたいですが、ちょっと実装方針を再検討してもらえると嬉しいです。
その結果今の形でいきたい、となったらそれはそれで構いません。よろしくお願いします。

[azbcww]

@ikura-hamu
テストコードの確認ありがとうございます！
いろいろ指摘していただいたことで何をテストすればいいのかわかりました，ありがとうございます<(_ _)>

@logica0419
実装方針の指摘ありがとうございます！
たしかに１つのメソッドにまとめた方が見栄えがいいですね．．．

@ikura-hamu @logica0419
僕としては実装時にtraQの思想を十分理解できていたわけではないので実装方針をこうやって提示していただけるのはありがたいです！
また，今回ここまで実装し皆さんのレビューを受けたことで，実装の流れや思想も何となく理解することができたので修正する手間は最初程かからないと考えています．
故に今後わかりやすくなるように実装しなおすのもありだと考えています．
そうなるとikura-hamuさんたちにレビューしていただいたところも最初からやり直しになってしまいますが．．．大事なところは変らないと思うので実装時にめちゃくちゃ参考にさせていただくつもりです！

[ikura-hamu]

@logica0419 ( @azbcww )
むしろコメントもらってありがたいです。確かにって感じなので、大変かもしれませんが再実装の方が良さそうです。よろしくお願いします。

[azbcww]

また時間が空いてしまったのですが修正をしたのでテストを書く前にこの実装方法で良いか一度確認してほしいです．
以下変更点と懸念点です．
お時間あるときに確認していただけると幸いです．
よろしくお願いします．

変更点:
GetAllStampsをGetAllStampsWithThumbnailに書き換えた．
それに伴い，stampRepository構造体要素のperTypeをStamp構造体のキャッシュから，Stamp構造体+HasThumbnailの構造体(以降StampWithThumbnail)のキャッシュに変更．

懸念点:
stampRepository構造体要素のstampsもStampWithThumbnailのキャッシュにすればコードは綺麗になりそうではあったが，GetAllStamps変更のためにそこまで変える必要があるのか不明であったためperTypeのみ変更した．

[logica0419]

ありがとう！
実装は問題無さそうでした！素晴らしいです

細かい書き方に関する指摘を数ヶ所しましたが、基本方針は完璧だと思います！

[logica0419]

stampsキャッシュの書き換えは、今回は必要なさそうなので無くていいと思います
使わない情報持ってても…という感じがするので

[azbcww]

コードの確認ありがとうございます！

修正を終えてテストを書いているのですが実装方法で質問があります．

やりたいテストは下記のものです．
#1929 (comment)

サムネイル画像が無い場合にちゃんとfalseが返って来るかの確認もやりたいです。t.Run()を使って、他のところと同じ感じに2つのテストケース(サムネイルがある、ない)を書いてください。

このテストでサムネイルのないスタンプを作る実装方法を迷っています．
まず，実装方法として次の2つを提案していただきました．

1. repository_test.goのmustMakeStampとその内部で呼ばれているmustMakeDummyFileにhasThumbnailのような新しい引数を作り、サムネイルを作るかどうかを決めれるようにする。
2. 新しくmustMakeStampWithoutThumbnail、mustMakeFileWithoutThumbnailを作る。

しかし，1. 2.どちらの実装方法でもmustMakeStamp内のrepo.CreateStampでサムネイルが存在している場合のみスタンプを作成しているため，サムネイルを作らずスタンプを作成する方法を書くのが大変だなと感じています．

そこで，mustMakeStampでサムネイルの存在するスタンプを作成後，repo.DeleteFileMeta()でサムネイルを削除すれば結果的にサムネイルの無いスタンプを得られると考えたました．こちらの実装方法では良くないところがありますでしょうか？

時間があるときに教えていただけると助かります．
よろしくお願いします．

[azbcww]

テスト追加しました．
サムネイルありスタンプ10個，サムネイルなしスタンプ10個作成しその個数をそれぞれテストしています．
確認よろしくお願いします．

[logica0419]

ありがとう！返信できてなくてごめんなさい…

修正・テスト共に良さそうです！DeleteFileMetaでの実装も良いと思います。
一応1箇所指摘をしておきました。

また、今回APIが返すデータ型に変更があるので、docs/v3-api.yamlをそれに合わせる必要があります(知ってたらすみません)。
OpenAPIという規格に則っているので、それに従って改変をお願いします。わかんないことあったら聞いてください。

[azbcww]

確認&指摘ありがとございます！
修正しました！
時間あるときで全然大丈夫なので確認よろしくお願いします．

[logica0419]

修正は完璧ですね！ありがとうございます
OpenAPIスキーマの方、修正して欲しいポイントがあるのでお願いします～

[azbcww]

確認ありがとうございます！
他の場所でも使われているの失念してました．．．
変更したので時間あるときに確認お願いします！

[logica0419]

LGTM！
一回ほぼ0からの実装し直しを挟んでしまって申し訳なかったです。もうちょっと早く言えれば良かったなと思っています…
結構重めの実装だったと思いますが、完走してくれてありがとう！
今後もよろしくお願いします〜

[azbcww]

何度も確認ありがとうございました！