wallet: refactor: dedup sqlite PRAGMA access #21962

pull theStack wants to merge 2 commits into bitcoin:master from theStack:202105-wallet-dedup_sqlite_pragma_stmts changing 1 files +46 −54

theStack commented at 2:57 PM on May 16, 2021: member

This refactoring PR deduplicates repeated SQLite access to PRAGMA settings. Two functions ReadPragmaInteger(...) (reads a single integer value via statement PRAGMA key) and SetPragma(...) (sets a key to specified value via statement PRAGMA key = value) are introduced for this purpose. This should be more readable and less error-prone, e.g. in case other PRAGMA settings need to be read/set in the future or the error handling has to be adapted.
DrahtBot added the label Wallet on May 16, 2021
in src/wallet/sqlite.cpp:45 in 3129f74ff3 outdated
```
  40 | +{
  41 | +    std::string stmt_text = strprintf("PRAGMA %s", key);
  42 | +    sqlite3_stmt* pragma_read_stmt{nullptr};
  43 | +    int ret = sqlite3_prepare_v2(db, stmt_text.c_str(), -1, &pragma_read_stmt, nullptr);
  44 | +    if (ret != SQLITE_OK) {
  45 | +        sqlite3_finalize(pragma_read_stmt);
```
laanwj commented at 12:17 PM on May 17, 2021:

Looks like there is still some duplication in the error handling. I wonder if it'd make sense to make RAII wrappers around these things at some point.

theStack commented at 3:29 PM on May 17, 2021:

I agree that RAII wrappers would make sense here to deduplicate the sqlite3_prepare_v2 / sqlite3_finalize pairs. My own C++ skills are probably still too lousy to implement this properly, but I'm happy to help reviewing :)

laanwj commented at 6:07 PM on May 17, 2021:

Yes, it was just a random idea, not a blocker for this PR.
wallet: refactor: dedup sqlite PRAGMA integer reads dca8ef586c
wallet: refactor: dedup sqlite PRAGMA assignments 9938d610b0
in src/wallet/sqlite.cpp:46 in 3129f74ff3 outdated
```
  41 | +    std::string stmt_text = strprintf("PRAGMA %s", key);
  42 | +    sqlite3_stmt* pragma_read_stmt{nullptr};
  43 | +    int ret = sqlite3_prepare_v2(db, stmt_text.c_str(), -1, &pragma_read_stmt, nullptr);
  44 | +    if (ret != SQLITE_OK) {
  45 | +        sqlite3_finalize(pragma_read_stmt);
  46 | +        error = strprintf(_("SQLiteDatabase: Failed to prepare the statement to fetch %s: %s"), description, sqlite3_errstr(ret));
```
laanwj commented at 12:18 PM on May 17, 2021:

I don't think we should be translating these SQLite internal error messages. It should be assumed that translators have some knowledge about bitcoin concept, but translators are not going to know what 'preparing a statement' is.

theStack commented at 3:27 PM on May 17, 2021:

Agreed, changed the code to return the error string with Untranslated(...) rather than _(...) -- the calling method SQLiteDatabase::Verify expects the error as bilingual_str, hence I think it's the easiest to also pass out this type in the helper ReadPragmaInteger.

laanwj commented at 2:47 PM on May 18, 2021:

Having the error message type be different between SetPragma and ReadPragmaInteger is slightly inconsistent, though yes I understand why.
theStack force-pushed on May 17, 2021
achow101 commented at 7:26 PM on May 17, 2021: member

Code Review ACK 9938d610b043bf018e0b34d92e3daaffe1f17fcc
laanwj commented at 12:01 PM on May 19, 2021: member

Looks good to me now, code review ACK 9938d610b043bf018e0b34d92e3daaffe1f17fcc
laanwj merged this on May 19, 2021
laanwj closed this on May 19, 2021
sidhujag referenced this in commit f2a8dbc1d0 on May 19, 2021
theStack deleted the branch on Jul 31, 2021
gwillen referenced this in commit ad78587f9d on Jun 1, 2022
DrahtBot locked this on Aug 16, 2022

Contributors

Labels