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

Sql #250

Closed
wants to merge 2 commits into from
Closed

Sql #250

wants to merge 2 commits into from

Conversation

ynsta
Copy link

@ynsta ynsta commented May 19, 2020

Hello,

Two commit to solve issues encountered using shiori with postgresql.

The serial auto increment was not working, and made a constraint violation for all bookmarks insertions after a delete. I also improved auto tag bookmark relation deletion with an on cascade delete.

The second commit is the removal of all invalid runes for utf8 before inserting in the db as it caused an error with pg.

Note that it might mask a bug in another part of the programs on the validity of the fetched bookmark content.

ynsta added 2 commits May 19, 2020 09:31
Bookmark id auto increment did not permit to remove a bookmak
and then insert a new one with postgresql.

Signed-off-by: Stany MARCEL <[email protected]>
@deanishe
Copy link
Contributor

deanishe commented Aug 6, 2020

Hi @ynsta, thanks for the PR.

I'm thinking that the UTF-8 fix should probably be applied globally, i.e. we shouldn't be putting screwed-up data in any database, even if MySQL and SQLite don't complain about it. What do you think?

Regarding the ID fix, I think we should also be using autoincrement (and cascading deletes) across all 3 databases, too.

Rather than adding a fix for Postgres and turning CreateNewID() into a noop, I'm thinking that CreateNewID() should be scrapped completely. The way I'd normally do it is to pass in pointers to Bookmark structs, and SaveBookmarks() would populate the ID field of any new ones.

In addition, I'd like to add migration support before making any further changes to DB schemas. As best as I can tell from reading the source code, your changes won't work well on existing installations because they won't have the updated DB schema.

What do you think?

@deanishe deanishe added type:bug Something isn't working type:enhancement New feature or request labels Aug 6, 2020
@ynsta
Copy link
Author

ynsta commented Sep 22, 2020

I not yet looked at your changes is this PR useful anymore ?

@deanishe
Copy link
Contributor

I not yet looked at your changes is this PR useful anymore ?

I haven't changed the database code yet. Please see my comments above.

@imajes
Copy link

imajes commented Oct 21, 2020

@deanishe @ynsta sounds like a smart move. The unicode fix is definitely cross db sensible, and the serial/auto-increment makes sense for all systems at this point. what will it take to make that happen?

@ynsta
Copy link
Author

ynsta commented Mar 3, 2021

If needed I can look at it again but don't know yet if @deanishe still want to rework the sql.

@imajes
Copy link

imajes commented Mar 7, 2022

Hey- just working on an import with pg and finding that the constraint and tag aliggnment is messed up. this definitely needs to get fixed. can we do what we can to move it forward?

@fmartingr
Copy link
Member

So. I would like to move this forward. The CreateNewID will be tackled once #271 gets picked, but I don't know what's hapepening with the runes or utf-8. Can someone guide me through the problem so I can reproduce it?

@imajes
Copy link

imajes commented Oct 7, 2022

@fmartingr i'm pretty slammed with work work right now, but once it lets up i'll try and jump back to this, as i've got way way too many documents full of links i want to centralize :P

@fmartingr
Copy link
Member

@imajes Of course! No problem, I just want to understand what the problem is because I will be changuing things related to the database engines in the following week (hopefully) so I would want to know what I have to take into consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag:stalebot type:bug Something isn't working type:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants