[#1013] Implement the Database Adapter 2#1034
[#1013] Implement the Database Adapter 2#1034marcocapozzoli wants to merge 3 commits intomasc/1013-database-adapter-cppfrom
Conversation
andre-senna
changed the title
andre-senna
changed the title
src/db_adapter/ContextLoader.h
Outdated
| namespace fs = std::filesystem; | ||
|
|
||
| bool load_context_file(const string& file_path, vector<TableMapping>& out) { | ||
| out.clear(); |
There was a problem hiding this comment.
Why? I don't see the point of doing this inside this function. If the caller wants a cleanup made upfront, it would make it itself before calling this method. Enforcing a cleanup here prevent the caller from calling the function twice for different input files in order to add more mappings to a previously computed vector of mappings. You may not do this right now but I don't see any reason to restrict the function this way.
There was a problem hiding this comment.
Enforcing a cleanup here prevent the caller from calling the function twice for different input files
Yes, you're right, I hadn't thought of that.
src/db_adapter/ContextLoader.h
Outdated
|
|
||
| if (!fs::exists(file_path)) { | ||
| LOG_ERROR("Context file " + file_path + " does not exist"); | ||
| has_error = true; |
There was a problem hiding this comment.
Shouldn't the code return from here? What's the point of going on if the file don't exist?
src/db_adapter/ContextLoader.h
Outdated
| for (char c : tn) | ||
| if (c == '.') ++count_dot; | ||
| if (count_dot != 1) { | ||
| LOG_ERROR(msg_base + ".table_name must be in format 'schema.table' (single dot)"); |
There was a problem hiding this comment.
| LOG_ERROR(msg_base + ".table_name must be in format 'schema.table' (single dot)"); | |
| LOG_ERROR(msg_base + ".table_name must be in format 'schema.table'"); |
src/db_adapter/ContextLoader.h
Outdated
| } else { | ||
| size_t pos = tn.find('.'); | ||
| if (pos == 0 || pos + 1 >= tn.size()) { | ||
| LOG_ERROR(msg_base + ".table_name parts must not be empty in a table entry"); |
There was a problem hiding this comment.
| LOG_ERROR(msg_base + ".table_name parts must not be empty in a table entry"); | |
| LOG_ERROR(msg_base + ".table_name must be in format 'schema.table'"); |
src/db_adapter/ContextLoader.h
Outdated
| } | ||
|
|
||
| if (has_error) { | ||
| LOG_ERROR("Context file validation failed with errors. Please fix the issues and try again."); |
There was a problem hiding this comment.
I understand you allowed the JSON processing continue after an error to allow the caller to see all syntax error messages at once instead of going one by one. If this is the case you aren't supposed to give any outputs when errors occur. So you need to change the logic of the way you output the mappings.
I suggest creating a local vector and here in this if, if no errors occurred you feed the contents of this internal vector to the output (without cleaning it up before). If errors occur, nothing is added to the output.
src/db_adapter/ContextLoader.h
Outdated
|
|
||
| namespace fs = std::filesystem; | ||
|
|
||
| bool load_context_file(const string& file_path, vector<TableMapping>& out) { |
There was a problem hiding this comment.
You don't put actual code in .h files. Usually, the only exception is when you are declaring a templated class (which is not the case here).
To achieve what you intended here, I suggest creating a class ContextLoader (with a .h and a .cc files) and declaring load_context_file() as a static method (i.e. a class method).
2 participants
This pull request adds the ContextLoader to validate the context file for mapping.
Resolves #1013