SNOW-1739603: Implement toml config file parsing #836
Conversation
|
Hi, |
| * Load toml configuration file. | ||
| * @param connectionParams The connection parameters parsed from the toml configuration file in k=v; format | ||
| */ | ||
| void load_toml_config(char** connectionParams); |
There was a problem hiding this comment.
I think we should expose the parameters via C++ API, for example sth like this?
std::map<std::string, std::string> loadTomlConfig()
….com/snowflakedb/libsnowflakeclient into SNOW-1739603-toml-file-configuration
cpp/lib/TomlConfigParser.cpp
Outdated
| @@ -0,0 +1,109 @@ | |||
| /** | |||
There was a problem hiding this comment.
No need for copyright
| #include "snowflake/TomlConfigParser.hpp" | ||
| #include "../logger/SFLogger.hpp" | ||
| #include "memory.h" | ||
| #include <toml++/toml.hpp> |
There was a problem hiding this comment.
Please use #define TOML_EXCEPTIONS 0
https://marzer.github.io/tomlplusplus/#mainpage-example-parsing-without-exceptions
We want to move towards exception free code
tests/test_toml_config.cpp
Outdated
| char tomlConfig[] = "[default]\nkey1 = \"value1\"\nkey2 = \"value2\""; | ||
| char tomlFilePath[] = "./connections.toml"; | ||
| FILE* file; | ||
| file = fopen(tomlFilePath, "w"); |
There was a problem hiding this comment.
Use c++ fstream API instead. Same for other tests
tests/test_toml_config.cpp
Outdated
|
|
||
| char envbuf[MAX_PATH + 1]; | ||
| char* snowflakeHome = sf_getenv_s("SNOWFLAKE_HOME", envbuf, sizeof(envbuf)); | ||
| sf_setenv("SNOWFLAKE_HOME", "./"); |
There was a problem hiding this comment.
Please extract EnvOverride class from test_unit_secure_storage.cpp to a new file tests/utils/EnvOverride.hpp. It will make sure we cleanup env after test finishes without being too verbose. Same for other tests
| cmocka_unit_test(test_missing_toml_file), | ||
| cmocka_unit_test(test_invalid_toml_file), | ||
| cmocka_unit_test(test_client_config_log_invalid_config_name), | ||
| #if (!defined(_WIN32) && !defined(_DEBUG)) || defined(_WIN64) |
There was a problem hiding this comment.
Why do we need this ifdef?
There was a problem hiding this comment.
on 32-bit windows debug build, the toml::parse_file call causes a "(_osfile(fh) & FOPEN)" debug assertion causing test cases to hang indefinitely. It gets bypassed with the ifdef in load_toml_config, but the rest of the test case is irrelevant anyways because of that. So I can either ifdef each test case separately to check for nothing or just not run it entirely since it doesn't actually test for anything
tests/test_toml_config.cpp
Outdated
| void test_valid_toml_file(void** unused) { | ||
| SF_UNUSED(unused); | ||
| // Create toml file | ||
| char tomlConfig[] = "[default]\nkey1 = \"value1\"\nkey2 = \"value2\""; |
There was a problem hiding this comment.
Prefer std::string over char[], same in other tests
cpp/util/SnowflakeCommon.cpp
Outdated
| @@ -12,6 +12,7 @@ | |||
| #include "../logger/SFLogger.hpp" | |||
| #include <chrono> | |||
| #include "SnowflakeCommon.hpp" | |||
| #include <chrono> | |||
There was a problem hiding this comment.
Remove the include, it's not needed. The same file is included in 13th line of this file
SNOW-1739603: Implement toml config file parsing in libsnowflakeclient to be used in other connectors.
Added toml_config_parser file with load_toml_config, returns a
key=value;style string with parsed parameters.Behaviour: If
SNOWFLAKE_HOMEexists, searches for toml config file there, otherwise defaults to~/.snowflake.Within toml config file, if
SNOWFLAKE_DEFAULT_CONNECTION_NAMEdefined, use that as configuration name, otherwise defaults todefault.