#1392 : Add support for SQLite VFS and open mode flags

[ancientjpeg]

Addresses #1392

Changelog

• Add vfs_mode enum class to simplify the sqlite_orm API for using different VFS systems, as compared to the raw string passed in sqlite3.h
• Add open_mode enum class to support the open flags passed by sqlite3_open_v2. Only two values have been added so far, but this addition makes it very easy to add more in the future.
• Add platform definitions to detect operating system
  • Necessary for exposing platform-specific VFS types
• Add tests

More Info

https://www.sqlite.org/vfs.html - VFS options and description
https://www.sqlite.org/c3ref/open.html - open flag options

[fnc12]

hey @ancientjpeg . Thank for this PR. Please work in dev branch not master

[ancientjpeg]

Just rebased. Thanks and apologies again.

[ancientjpeg]

@fnc12 I've added enums to allow for full control over what open mode/VFS options users actually have access to. I'm happy with the state this PR is in, but I have some preliminary evidence that using some VFS modes like unix-dotfile may interfere with the ability to set certain journal_modes. This is detectable by just checking journal_mode after you've set it, but if you'd like me to brainstorm a layer of protection for that I'd be happy to.

[trueqbit]

Thanks @ancientjpeg for your contribution! Here come my 2 cents...

[ancientjpeg]

@trueqbit Still working through your changes, but something occurred to me while refactoring. I had some of those unnecessary sqlite_orm:: qualifiers on the mode types because I actually got a couple compile errors complaining about ambiguous symbols. Would it be alright if I appended _t to these flag types? I.e.,

enum class vfs_mode_t;

Then it'd be a lot clearer to re-use the same verbiage for defining variables, e.g.

vfs_mode_t vfs_mode;
open_mode_t open_mode;

struct storage_options {
  vfs_mode_t vfs_mode;
  open_mode_t open_mode;
};

Let me know what you think of that.

Edit: I ended up implementing this because GCC complains of ambiguity in cases such as const open_mode open_mode. I prefer the look of const open_mode_t open_mode to const open_mode open_flags as I feel the latter might sow confusion, but I'm happy to change it if it feels more in line with the rest of the lib.

[trueqbit]

@trueqbit Still working through your changes, but something occurred to me while refactoring. I had some of those unnecessary sqlite_orm:: qualifiers on the mode types because I actually got a couple compile errors complaining about ambiguous symbols. Would it be alright if I appended _t to these flag types? I.e.,

enum class vfs_mode_t;

Then it'd be a lot clearer to re-use the same verbiage for defining variables, e.g.

vfs_mode_t vfs_mode;
open_mode_t open_mode;

struct storage_options {
  vfs_mode_t vfs_mode;
  open_mode_t open_mode;
};

Let me know what you think of that.

Hmm, that's a funny problem as both are public :)
However, I do not recommend changing the enum type's name. Firstly, it is not recommended in the coding guidelines, and we do not use it in sqlite_orm with one exception.

I personally like camel case for (member) variables, but I agree that's debatable.
A good choice in any case for the variable name is vfsOption, modeOption or vfs_option, mode_option as "option" conveys that fact that a specific thing is selected .

While we are at it, I also suggest renaming vfs_mode -> vfs_object, as SQLite calls the feature Virtual File System Objects.

[ancientjpeg]

@trueqbit Great point about using option in the variable name, I think I'll go with that. I'll also change the members and parameters to camelCase if that's the preferred styling for this library—I had just assumed snake_case was the go-to given the external API is style that way, but now that I look closer I see a lot of parameter names are camelCase.

I'll be away from my development system until next week, at which time I'll wrap up these changes. Thanks for the feedback!

[ancientjpeg]

Re-drafting to address merge conflicts.

[trueqbit]

Re-drafting to address merge conflicts.

Hi @ancientjpeg,

I must apologize for the inconvenience. Independently of you, I have developed a way to pass various storage options to make_storage(), in particular connection control options. The important change for you is that there is now a connection_control aggregate that you extend with your new options.

Also, I would like to point out to you in advance that an enum to represent a VFS is most likely too restrictive as it is not a fixed set of values. But we can discuss that later.

[ancientjpeg]

@trueqbit I've refactored to best match my understanding of your new connection_control object. Let me know if this is how you'd expect it to be used.

Also, what exactly were you thinking when you said that an enum is too restrictive for the VFS object? If you meant that you'd like to open up support to the sqlite3_vfs_register api, I can adjust the code to account for that. In my mind the best way would be to still use the enum, but add a vfs_object::custom enum value, and connection_control could be extended to include a std::string custom_vfs_object that's only used if the aforementioned enum is passed. I believe this would be a good balance of type safety and extensibility.

[trueqbit]

Only a few things are left to change, great work otherwise!

[trueqbit]

@ancientjpeg Why do you always force-push changes onto your feature branch?

[ancientjpeg]

@ancientjpeg Why do you always force-push changes onto your feature branch?

@trueqbit I force-push because I generally rebase onto dev any time there's updates just in case there's conflicts to resolve. I generally prefer a rebase workflow but if you'd prefer a merge workflow for PRs against this repo I'll gladly do that instead!

[ancientjpeg]

Corrected conflict over extended response code usage. I opted to keep the flag addition in connection_holder.h instead of inserting it into db_open_mode_to_int_flags(), as I felt it was clearer for the latter to return exactly the flags its input argument corresponds to and nothing more. The flag manipulation feels like the responsibility of the connection holder, not the flag translator IMO.

[fnc12]

the code is good, the tests are good. The feature is very good. Thank you for submitting this. I got several nits which are not so urgent. You can fix it now within this PR or we can change it later in dev branch in further PRs - up to you

[fnc12]

how to fix this?

@trueqbit @ancientjpeg

[trueqbit]

#1393 (comment)
how to fix this?

@trueqbit @ancientjpeg

Hmm, which SQLite library version are you using? It should be there since SQLite 3.3.8, and is defined in sqlite3.h:

BTW, your screenshot seems not to be from the Minimal-Audio:feature/1392-support-sqlite-vfs branch...
BTW @trueqbit The version number should be >= 3003008 instead of >= 3008008.

If everything fails we could of course check directly with SQLITE_OPEN_EXRESCODE...

[fnc12]

Hmm, which SQLite library version are you using? It should be there since SQLite 3.3.8, and is defined in sqlite3.h:

I use 3.26.0 and my sqlite3.h doesn't have SQLITE_OPEN_EXRESCODE

BTW, your screenshot seems not to be from the Minimal-Audio:feature/1392-support-sqlite-vfs branch...

My screenshot os from dev merged into feature/logger

BTW @trueqbit The version number should be >= 3003008 instead of >= 3008008.

I recon you put a wrong at here =)

[fnc12]

[trueqbit]

Hmm, which SQLite library version are you using? It should be there since SQLite 3.3.8, and is defined in sqlite3.h:

I use 3.26.0 and my sqlite3.h doesn't have SQLITE_OPEN_EXRESCODE

Alright, I assumed this flag was introduced with the "extended result code" feature....
@ancientjpeg Maybe you want to replace the check #if SQLITE_VERSION_NUMBER >= 3008008 with #if SQLITE_VERSION_NUMBER >= 3037002, then it is already done?

BTW @trueqbit The version number should be >= 3003008 instead of >= 3008008.

I recon you put a wrong at here =)

No, that's right to blame myself, because I was the one who introduced the handling of extended result codes with PR #1412.

[fnc12]

@trueqbit oh so I posted my issue at wrong place. Sorry @ancientjpeg

[ancientjpeg]

@fnc12 @trueqbit I believe I've addressed all comments! As I said before, I'm happy to expand the platform definition macros but I believe it would be wise to do that in a separate PR to keep the scope limited here. Let me know if there's any other changes you'd like made before merging.

[trueqbit]

@ancientjpeg
fix visual studio error

IMHO we should prefer good commit messages, or at least correct ones - it's not a Visual Studio bug, it's the wrong order of initialization of members.
We just recently changed the compiler warning to an error so that we would notice it in the future.

Otherwise thank you for your good worK and outstanding patience! 🥇

[ancientjpeg]

Apologies about the commit message—I had interpreted this error as being MSVC-specific (I see those a lot), as I didn't realize that Werror=reorder had been added in the recent merge. Thank you for all your time spent reviewing and refining this code!