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

feat: ogp cache を message ごとに分割してブラウザにキャッシュさせるように #4421

Closed
wants to merge 1 commit into from

Conversation

cp-20
Copy link
Contributor

@cp-20 cp-20 commented Dec 5, 2024

モチベーション

DELETE /ogp/cache したとしてもブラウザのネットワークキャッシュとして OGP のキャッシュが残っており、一度取得してしまうと通常の max-age (1週間?) が終わるまで更新されなくなる。これを直したい。

解決策

この問題にはいくつか解決策が考えられるが、比較的実装が容易な URL のキャッシュキーを変えるという方法を用いた。GET /ogp?url=... で OGP を取得しているところを GET /ogp?url=...&message=... とすることでメッセージごとにキャッシュキーを変えた。

この変更によってサーバー側のキャッシュが消えてから投稿されたメッセージに関しては OGP が再取得されるようになる。弊害として特にキャッシュの削除を行っていない場合でも異なるメッセージではリクエストが飛んでしまうが、サーバー側のキャッシュにはヒットする上、このようなケースは比較的稀なので問題ないと判断した。

Copy link

github-actions bot commented Dec 5, 2024

@cp-20 cp-20 requested a review from nokhnaton December 5, 2024 16:02
Copy link
Contributor

@nokhnaton nokhnaton left a comment

Choose a reason for hiding this comment

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

PRありがとうございます!

キャッシュをどうやるかというのは私も何が正解かよくわかっていないんですが、アプリケーション側でMapを使ってキャッシュしている以上、ブラウザ側でのキャッシュは使わないようにしちゃっていいんじゃないかと思ってます。
どうしてもやるならstale-while-revalidateとかの戦略かなって思いますが、アプリ側でのキャッシュの更新とか含めてめんどくさそうなのでno-cacheとかで問題ないのかなと思ってます。

実装に関しても今回の方針だとどうしてもopenapiから生成された関数を使えないのであまり良くないのかなと思ってます。(例えばoptionが指定できなくなっているなど)

@cp-20
Copy link
Contributor Author

cp-20 commented Dec 6, 2024

ブラウザのキャッシュを使わずにアプリケーションのMapだけを使ってモチベーションのことを達成するとしたら、そもそもアプリケーションのMapは何をキャッシュするんでしょう?(キャッシュヒットしなくないですか?)

実装の方針が良くないのは分かっているのですが、これ以上に筋の良い方法が思いつかず…という感じです

@nokhnaton
Copy link
Contributor

リロードしたりするまではURLごとにキャッシュするのを想定してます

リロードしたらogpが更新されるっていうのは割と直感的な仕様かなーと思うので悪くないのでは?と思ってます。

リロードするごとに通信が走るのは確かにそうなのでETagを返すようにする(もともとそんな大きくないテキストしか送ってないので効果は微妙かも?)とかswrのキャッシュ戦略をする(うまくやらないと2回リロードしないと更新されないとかになりそう)とかの対策を必要そうなら取る感じになるのかなぁと思います

@cp-20
Copy link
Contributor Author

cp-20 commented Dec 6, 2024

理解しました、悪くないと思います

リロード後のリクエストは特に対策しなくてもサーバー側のキャッシュにヒットすれば十分かなという気がします

@cp-20
Copy link
Contributor Author

cp-20 commented Dec 6, 2024

他に懸念点がなければその方針で新しくPR出します

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.

2 participants