チャットアップロードバグ

こんにちは、チャットのダイレクトメッセージでファイルや画像をアップロードする際に発見した問題についてご報告します。

  • ユーザーAが投稿またはチャットでファイルAをアップロードした場合、問題なく動作し、ファイルは正しく登録されます。

  • しかし、ユーザーBファイルAをダウンロードし、別のチャットで再度アップロードしようとすると:

    • ユーザーBがテキストなしでファイルを送信した場合、エラーが発生します:

      Message is too short, must have a minimum of 1 character.
      
    • ユーザーBがテキストありでファイルを送信した場合、テキストのみが送信され、添付ファイルは失われます。

しかし、ユーザーAが同じファイルを再度アップロードすると、問題なく動作し、ファイルはメッセージに含まれます。

私の質問:
これは意図された動作であり、元のアップローダーのみがアップロードされたファイルを再利用できるということでしょうか?それとも、他のユーザーも同じファイルをチャットにアップロードして送信できるべきでしょうか?

「いいね!」 1

これは少しグリッチのようですね。数週間かけて誰かが確認します。それまでの間、PR歓迎とさせていただきます。

「いいね!」 1

この原因を突き止めるのに時間がかかりましたが、原因がわかったと思います。それは、このコードに関連していると思います。

def fetch_uploads(params:, guardian:)
  return [] if !SiteSetting.chat_allow_uploads
  guardian.user.uploads.where(id: params.upload_ids) # 具体的にはここ
end

新しいチャットメッセージが作成されると、メッセージに添付するアップロードを取得するためにこれが呼び出されます。おそらく所有権を確認するために、メソッドは Guardian のユーザーを通じてアップロードを取得し、そのユーザーに属するアップロードのみを許可するようにします。

問題は、こちらで見られるように、アップロードが重複排除されていることです。

# そのアップロードはすでにありますか?
@upload = Upload.find_by(sha1: sha1)

# ...

# 存在する場合は前のアップロードを返します
if @upload
  add_metadata!
  UserUpload.find_or_create_by!(user_id: user_id, upload_id: @upload.id) if user_id
  return @upload
end

可能な修正策は、ユーザーに属するアップロードのみを考慮するのではなく、UserUpload を介して取得することだと思います。UserUpload はアップロードを複数のユーザーにリンクしており、それが私たちが必要としているもののようです。まだ完全に確信はありませんが、もう遅いので寝ます。もし他の人が修正しなければ、後で戻って PR を作成しようと思います。:slightly_smiling_face:

「いいね!」 3

これは素晴らしい指摘で、的確です。 :hugs:

アップロードには user_id がありますが、それはアップロードを作成した元のユーザーだけです。

このコードを次のように変換すると:

Upload.where(id: params.upload_ids).joins(:user_uploads).where(user_uploads: { user_id: guardian.user.id })

うまくいくはずです。

PR を送っていただけますか!

ありがとうございます。このクエリをどのように構築するのが最善か確信が持てませんでした(私はかなり新参者です :slightly_smiling_face:)。PRを作成します。

「いいね!」 3

PRが完了しました。遅れて申し訳ありません。私用で手間取っていました。

FIX: チャットメッセージで、Upload.userではなくUserUploadでアップロードをフィルタリングする by clechasseur · Pull Request #34596 · discourse/discourse

一部のテストが失敗しているようですが、私が触ったテストではありません。ローカルでもこれらのファイルでテストを実行するのに問題があり、ここに見られるのと同じ「重複キー」エラーが発生しました。私が何かを壊したのか、それともテストが時々不安定なのか分かりません…

失敗したテストを少なくとも3回再実行し、3回目に失敗した場合にのみパニックを開始するのが良い経験則です😜

とはいえ、@j.jaffeux が手伝えるかもしれませんか?

「いいね!」 1

はい、これらは単なる不安定なエラーやその他のランダムなエラーです。あなたの変更は問題ありません。PRテストを再実行し、パスするか、同じランダムなシーケンスエラーで失敗した場合は、承認してマージします。

「いいね!」 1

ホラーファンとしては、3回何かをすると何が起こるかを知らないわけにはいきません :sweat_smile:

「いいね!」 2

このPRはマージされました。@clechasseur さん、貢献ありがとうございました :rocket:

「いいね!」 6