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

Fix bug when add new bookmark using PostgreSQL #480

Closed
wants to merge 2 commits into from

Conversation

fukajun
Copy link

@fukajun fukajun commented Sep 28, 2022

Fixed a bug pages not archived although createArchive checkbox checked. (fix: #385)

Maybe it is only occurred using postgres.
To be precise, the archive is saved, but the ID to refer to different.

This ploblem reason is diffrent create file path id(archive and thumb) and create bookmark record id.

Function of CreatNewID return recently numbered bookmark id that is not same to be insert bookmark record id.
Returned bookmark id is used to path of save thumb and archive.
But, record inserted by next numbered id.
As a result, New bookmark record try to refered not exists directory pathes.

This fix will be changes CreateNewID returned to be next numbered bookmark record id.

Excuse, This implements is not think about concurrency bookmarking.
But, Current implement is same about this point.

@fukajun fukajun marked this pull request as draft September 28, 2022 09:22
@fukajun fukajun marked this pull request as ready for review September 29, 2022 04:43
@fmartingr
Copy link
Member

Hey @fukajun, thank you for finding this bug! While the solution is not optimal, the actual implementation is not very good as well, so I'm good with merging this into Shiori, but could you add some tests so we have some workflows covered? I have plans to refactor this in the future so we don't need to get a new ID from the database (we will either generate ir ourselves or insert the bookmark before creating archives).

@fmartingr
Copy link
Member

fmartingr commented Oct 7, 2022

Hey @fukajun I have taken the liberty of solving this on #484 along with other related issue. Thank you for reporting this and taking the time to provide a fix, really appreciated!

@fmartingr fmartingr closed this Oct 7, 2022
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.

Adding a URL doesn't automatically create an archive
2 participants