[native/sqlite] introduce function to set database version
ClosedPublic
Actions

Authored by kamil on Oct 17 2022, 5:56 AM.

Details

Reviewers

marcin
• jon
atul
tomek

Commits

rCOMMc2969dbdabab: [native/sqlite] introduce function to set database version

Summary

In the future there will be a need to update database version in different places, that being said extracting logic to separate function.
Additionally, added error handling which will detect if SQL was executed without failure.

Test Plan

Check if logged database version is propper (could be compared with user_version visible after connecting to the database).
Check if the version is set correctly after migrations (to test eg. logout user, after this version should be set to the highest currently possible).

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Oct 17 2022, 5:56 AM

kamil held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptOct 17 2022, 5:56 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

Harbormaster completed remote builds in B12821: Diff 17606.Oct 17 2022, 6:06 AM

kamil edited the summary of this revision. (Show Details)Oct 17 2022, 6:11 AM

kamil edited the test plan for this revision. (Show Details)

kamil added a reviewer: atul.

kamil edited the summary of this revision. (Show Details)

kamil added a parent revision: D5375: [native/sqlite] introduce function to get database version.Oct 17 2022, 6:13 AM

kamil added a child revision: D5377: [native/sqlite] refactor code to perform migration inside one transaction.Oct 17 2022, 6:28 AM

kamil published this revision for review.Oct 17 2022, 6:40 AM

atul added 1 blocking reviewer(s): atul.Oct 19 2022, 11:16 AM

Looks good, thanks for adding that error handling/logging

This revision is now accepted and ready to land.Oct 19 2022, 1:06 PM

Same concerns and questions as in D5375.

This revision now requires changes to proceed.Oct 20 2022, 12:27 PM

In D5376#160623, @marcin wrote:

Same concerns and questions as in D5375.

Same response as in D5375

marcin requested changes to this revision.Oct 21 2022, 1:31 AM

marcin added inline comments.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
575 ↗	(On Diff #17606)	Is it OK to ignore failure to set database version? This function returns false if SQL query fails but its return value is not checked against. I am not saying we need to check for every SQL query failure. I just want to be sure that this is conscious decision to ignore it here.

This revision now requires changes to proceed.Oct 21 2022, 1:31 AM

kamil requested review of this revision.Oct 21 2022, 1:48 AM

kamil added inline comments.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
575 ↗	(On Diff #17606)	Is it OK to ignore failure to set database version? It's not but this diff only extracts logic to function (and adds checks for errors) for now, and keeps the previous behavior - where code ignore errors. Handling these errors would be introduced in another diff - it's not part of this stack, we need a proper architecture for handling this, I am currently implementing this (tasks).