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

[mkcal] Add DB support for thisAndFuture incidence attribute. #43

Merged
merged 1 commit into from Jan 24, 2023

Conversation

dcaliste
Copy link
Contributor

@pvuorela: this may become handy if we plan to support changes from a point in time for recurring events at UI level.

There is no test for the migration code path. I run it by hand though on an old database and it seems that the new column is correctly added. And rerunning it on the upgraded database didn't trigger the migration path anymore.

Copy link
Contributor

@pvuorela pvuorela left a comment

Choose a reason for hiding this comment

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

Seems good. For one detail got pondering if there should be some DEFAULT 0 to convert old values but looks like the sqlite3_column_int() converts null to 0 anyway.

@dcaliste
Copy link
Contributor Author

Exactly, I wanted to put the DEFAULT statement for the old values, but the documentation explaining that not set being interpreted as 0 for integer, there is no need.

@pvuorela pvuorela merged commit 91e28e8 into sailfishos:master Jan 24, 2023
@pvuorela
Copy link
Contributor

Had something funny on my devel device, user_version was 1 but the thisAndFuture was already migrated.
Result: calendar database failed to open at all due to add column attempting to create a duplicate and SL3_exec() then bailing out with goto error.

Suppose this could happen there in the wild if for whatever reason the migration stops in the middle. Perhaps should try to cope better.

@dcaliste
Copy link
Contributor Author

Oh yes, this is bad indeed. Maybe one should put a SL3_exec("PRAGMA user_version = xx") in each if statement of migration in addition to the list of startup statements ? Maybe also encapsulated around with a query = BEGIN_TRANSACTION; SL3_exec(d->mDatabase); and a query = COMMIT_TRANSACTION; SL3_exec(d->mDatabase);` ? I still don't see how the startup statements can go wrong, but eh, that's usually the case with bugs...

@pvuorela
Copy link
Contributor

On this particular case I might have tried something here myself to cause the problem.

Could have been nice if there was error code to detect the already exists thing. Without it, not sure, could make the column addition just executed and ignore the result value.

Then again not sure how big of a problem this really is. The migration should happen only once and what are the chances that it exits in the middle.

@dcaliste
Copy link
Contributor Author

Could have been nice if there was error code to detect the already exists thing. Without it, not sure, could make the column addition just executed and ignore the result value.

I'll see if SQLite is returning plain enough errors to detect this. I'll propose something to try to migitate such cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants