go-shiori / shiori

Simple bookmark manager built with Go
MIT License
9.28k stars 551 forks source link

fix: thumbnail should show last thumbnail in server in any condition #858

Closed Monirzadeh closed 2 weeks ago

Monirzadeh commented 6 months ago

thumbnail not update in browser sometimes.

bun.lockb update with make style so i just push that too.

Closes #857 Depends on #896

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 34.08%. Comparing base (e34cd36) to head (a7ceb09).

:exclamation: Current head a7ceb09 differs from pull request most recent head 78bffff

Please upload reports for the commit 78bffff to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #858 +/- ## ========================================== - Coverage 35.17% 34.08% -1.10% ========================================== Files 61 60 -1 Lines 4065 4008 -57 ========================================== - Hits 1430 1366 -64 - Misses 2413 2420 +7 Partials 222 222 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fmartingr commented 3 months ago

I'm thinking that instead of the query parameter, what do you think if we send the Last-Modified and ETag headers in the response?

Last-Modified: {formatted modified_at}
ETag: W/{filepath}-{modified_at}

What do you think?

Monirzadeh commented 3 months ago

@fmartingr thanks for idea. i prefer this way but after review some question stil exist for me:

  1. we send thumbmail with SendFile with this function https://github.com/go-shiori/shiori/blob/e34cd36e3f54e1d2ac00b93080824e3a94e7f765/internal/http/response/file.go#L13-L42 as you see it set ETag based of info.ModTime().Unix() that #857 should not happen at first place but it is happen. first we should fix the problem here that i am not sure why still exist

  2. beside that why ETag not work right now i can done same thing we done here for archive files (not use SendFile) https://github.com/go-shiori/shiori/blob/e34cd36e3f54e1d2ac00b93080824e3a94e7f765/internal/http/routes/bookmark.go#L110-L153 but this make duplicate code that i don't like

  3. if i want use SendFile i will change that to get one more input as Etag that if it be empty it generate current Etag logic

do you want approach 2 or 3?

fmartingr commented 3 months ago

Can you refactor SendFile adding a nilable argument with options coming from a struct?

Something along the lines:

type SendFileOptions struct {
  Headers []http.Header
}

func SendFile(c *gin.Context, storageDomain model.StorageDomain, path string, options *SendFileOptions) {
...
}

And if we don't provide Cache-Control/ETag headers in the options, defaults are set in the function.

What do you think?

Monirzadeh commented 3 months ago

@fmartingr i work on that to impeliment ETag there too. but use of etag not fix #857 it just make less prosses in server side. the problem is browser can send request for url and if it is not change with cacheBust or anything elese than not send request to update this even if you update Etag. so still we could follow this approach

but combine Etag (improve performance no need each time open that file https://github.com/go-shiori/shiori/pull/858/commits/7fb272183d81670c2e1757258d5060e004381703) with previous approach (add modifiedAt to that url https://github.com/go-shiori/shiori/pull/858/commits/c91ce2c8853f1a58d31e3e0584774af753c9410f) can be somehow useless in this case because if we used modifiedAt than unnecessary request will not send by default

do you have any idea?

fmartingr commented 1 month ago

If the best course of action here is to provide a queryparameter with the modified time, let's go for that so we fix the bug. Any improvements can be handled later.

Monirzadeh commented 1 month ago

@fmartingr i use queryparamet ?modfiedAt + keep option to define custom etag (now for thumbnail return 304 if no need to update thumbnail and use cache thumbnail instead). can you please review this on your free time?