Feature/sql auditing #1370

juandent · 2024-12-20T19:53:36Z

First implementation of sql_auditing -- testing use of it

fnc12 · 2024-12-22T13:08:34Z

architecture of current implementation of sql auditing can't be combined with the project's architecture which (projects) have logs system like boost::log or custom written ones. How this approach is going to wok in projects which use boost::log for example?

dev/sql_auditing.h

fnc12 · 2024-12-22T12:59:22Z

dev/sql_auditing.h

+    std::string destination_file = "sql_auditor.txt";
+    friend class sql_auditor;
+    sql_auditor_settings() {}
+    inline static sql_auditor_settings& settings() {


what if the app has >1 storages and author wants to write logs to different files for different storages?

the destination file could be part of the storage_base object but then we could not log the initial instructions (i.e. make_storage) -- what if we want to see the SQL emitted by make_storage()? What do you think? is storage_base the place to put the sql_auditor?

since the call to sql_auditor::log() occurs in util.h, how do we know which storage it belongs to? I am talking about e.g. void perform_exec()??? we only know the sqlite3* but that does not map to storage so its no use

we could intercept on_open to register every connection used by a storage_base?

and create a map of sqlite3* pointers and storage_base* pointers!!

What do you think? is storage_base the place to put the sql_auditor?

I don't think we need sql_auditor at all but it is up to you

fnc12 · 2024-12-22T13:01:17Z

dev/sql_auditing.h

+    static void log(const std::string& message);
+};
+
+class sql_auditor_settings {


different classes better be put in different files like so: one file for one class/enum/struct

ok - one question about namespace: what to use here? namespace sqlite_orm or global namespace?

don't use global namespace in the lib cause it may lead to collisions in the user code

fnc12 · 2024-12-22T13:01:37Z

dev/sql_auditing.h

@@ -0,0 +1,86 @@
+#pragma once
+#include <fstream>
+#include "error_code.h"


put "" include after <> include like it is done everywhere within this repo

dev/sql_auditing.h

fnc12 · 2024-12-22T13:02:29Z

dev/sql_auditing.h

+#include "error_code.h"
+#include <chrono>
+#include <ctime>  // For std::localtime and std::tm
+#include <iomanip>  // For std::put_time


please skip For, just write expressions for which this include is required how it is done everywhere within this repo

fnc12 · 2024-12-22T13:03:45Z

dev/sql_auditing.h

+    open();
+}
+
+inline void sql_auditor_settings::set_destination_file(const std::string& filename) {


one can refactor argument to accepts std::string by value not by reference and to move it. It can skip one extra copying of std::string

fnc12 · 2024-12-22T13:04:42Z

dev/sql_auditing.h

+    if(!log_file.is_open()) {
+        log_file.open(sql_auditor_settings::settings().destination_file, ios::trunc | ios::out);
+        if(!log_file.good()) {
+            throw std::system_error{sqlite_orm::orm_error_code::failure_to_init_logfile};


what happens if this exception is fired, caught, the app is executing further and calls sql_auditor::log?

fnc12 · 2024-12-22T13:05:31Z

dev/sql_auditing.h

+    std::tm local_time = *std::localtime(&now_time);
+
+    // Print the local time in a human-readable format
+    auditor().log_file << "@: " << std::put_time(&local_time, sql_auditor_settings::settings().format_str.c_str())


what if the user of sqlite_orm wants to use sql auditor but doesn't want @ symbol to be put in the logs?

fnc12 · 2024-12-22T13:06:53Z

tests/prepared_statement_tests/select.cpp

@@ -14,6 +14,14 @@ TEST_CASE("Prepared select") {

    auto filename = "prepared.sqlite";
    remove(filename);
+
+#define JD_AUDITING_SETTINGS


what is the purpose of lines 18 and 19? you define a value and next you check whether it is defined (and it is definitely defined)

its the way I mark tentative code...
experimental code...

it is useless cause it never goes in branch, it goes the same way in all the conditions so if you drop it nothing will change

fnc12

architecture of current implementation of sql auditing can't be combined with the project's architecture which (projects) have logs system like boost::log or custom written ones. How this approach is going to wok in projects which use boost::log for example?

fnc12 · 2025-01-03T06:42:56Z

tests/prepared_statement_tests/select.cpp

@@ -14,6 +14,14 @@ TEST_CASE("Prepared select") {

    auto filename = "prepared.sqlite";
    remove(filename);
+
+#define JD_AUDITING_SETTINGS


it is useless cause it never goes in branch, it goes the same way in all the conditions so if you drop it nothing will change

fnc12 · 2025-01-03T08:13:07Z

dev/sql_auditing.h

+        class storage_base;
+    }
+}
+using sqlite_orm::internal::storage_base;


why this using is here?

using is used here because log_settings and log_admin are declared in the glocal namespace

don't use global namespace in the lib code

fnc12 · 2025-01-03T08:14:30Z

dev/sql_auditing.h

+        status = stats;
+        global_settings = glob_settings;
+    }
+    log_settings() {}


why do we need default constructor here?

we need a default constructor for log_settings because we provide a default constructor for log_admin!!

fnc12 · 2025-01-03T08:15:32Z

dev/sql_auditing.h

+    std::shared_ptr<std::ofstream> log_file;
+    std::string log_file_name;
+    std::string time_format;
+    enum status_code { FAIL, GOOD } status = status_code::FAIL;


avoid using non-class enums cause old enums add their constants into global scope

fnc12 · 2025-01-03T08:17:00Z

dev/sql_auditing.h

+};
+
+inline std::shared_ptr<std::ofstream> get_file(std::string log_file_name) {
+    static std::map<std::string, std::shared_ptr<std::ofstream>> files;


why do we need to keep file streams in a static map? better avoid static vars at all cause it is not thread safe

__the other alternative would be a global object;....

don't use global/static objects cause it it not thread safe and break the architecture. Global object is anti-pattern

fnc12 · 2025-01-03T08:17:24Z

dev/sql_auditing.h

+    static std::map<std::string, std::shared_ptr<std::ofstream>> files;
+    auto it = files.find(log_file_name);
+    if(it == files.end()) {
+        std::shared_ptr<std::ofstream> pointer_to_shared{new std::ofstream()};


use std::make_shared instead of new. Avoid using new and delete if you can

fnc12 · 2025-01-03T08:17:59Z

dev/storage_base.h

@@ -766,6 +766,9 @@ namespace sqlite_orm {
                if(this->on_open) {
                    this->on_open(db);
                }
+                // JD added


this comment is useless cause git blame has info about authors of every line

fnc12 · 2025-01-03T08:18:06Z

dev/storage_base.h

@@ -766,6 +766,9 @@ namespace sqlite_orm {
                if(this->on_open) {
                    this->on_open(db);
                }
+                // JD added
+                log_admin::admin().register_db(this, db);
+                // JD end


same here: this comment is useless cause git blame has info about authors of every line

juandent · 2025-01-05T16:28:21Z

Why do they break the architecture?Sent from my iPhoneOn 5 Jan 2025, at 4:07 AM, Yevgeniy Zakharov ***@***.***> wrote: @fnc12 commented on this pull request. In dev/sql_auditing.h:

+

+ std::time_t now_time = std::chrono::system_clock::to_time_t(now); + + // Convert to local time (std::tm structure) + // WARNING: localtime is not thread safe! + std::tm local_time = *std::localtime(&now_time); + + // Print the local time in a human-readable format + *log_file << "@: " << std::put_time(&local_time, time_format.c_str()) << " = "; + *log_file << message << std::endl; + } + } +}; + +inline std::shared_ptr<std::ofstream> get_file(std::string log_file_name) { + static std::map<std::string, std::shared_ptr<std::ofstream>> files; don't use global/static objects cause it it not thread safe and break the architecture. Global object is anti-pattern —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

fnc12 · 2025-01-05T16:30:10Z

Why do they break the architecture?

Cause global object (singleton) is anti-pattern. If we can avoid it we have to avoid it

juandent · 2025-01-05T16:30:37Z

Which is the global/static object you refer to?Sent from my iPhoneOn 5 Jan 2025, at 10:28 AM, Juan Dent ***@***.***> wrote:Why do they break the architecture?Sent from my iPhoneOn 5 Jan 2025, at 4:07 AM, Yevgeniy Zakharov ***@***.***> wrote: @fnc12 commented on this pull request. In dev/sql_auditing.h:

+

+ std::time_t now_time = std::chrono::system_clock::to_time_t(now); + + // Convert to local time (std::tm structure) + // WARNING: localtime is not thread safe! + std::tm local_time = *std::localtime(&now_time); + + // Print the local time in a human-readable format + *log_file << "@: " << std::put_time(&local_time, time_format.c_str()) << " = "; + *log_file << message << std::endl; + } + } +}; + +inline std::shared_ptr<std::ofstream> get_file(std::string log_file_name) { + static std::map<std::string, std::shared_ptr<std::ofstream>> files; don't use global/static objects cause it it not thread safe and break the architecture. Global object is anti-pattern —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

juandent · 2025-01-05T16:33:56Z

But I am using singleton patternSent from my iPhoneOn 5 Jan 2025, at 10:30 AM, Yevgeniy Zakharov ***@***.***> wrote: Why do they break the architecture? Cause global object (singleton) is anti-pattern. If we can avoid it we have to avoid it —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

fnc12 · 2025-01-05T16:48:38Z

Which is the global/static object you refer to?

The one defined as static in a function and returns as a ref

But I am using singleton pattern

This comment doesn't make any sense

fnc12 · 2025-01-05T16:49:41Z

@juandent you are still ignoring this comment

architecture of current implementation of sql auditing can't be combined with the project's architecture which (projects) have logs system like boost::log or custom written ones. How this approach is going to wok in projects which use boost::log for example?

It is the main issue with this PR. No reason to spend time on talking about singleton and other stuff until this one is solved

juandent · 2025-01-05T17:06:25Z

“But I am using singleton pattern” I am following Dimitri Nesteruk’ “Design patterns in Modern C++” and Fedor Pikus’ “Hands-on design patterns with C++” both of which implement a safe way for Singletons. So why so negative about the correct use of singletons? Regards, Juan From: Yevgeniy Zakharov ***@***.***> Sent: Sunday, January 5, 2025 10:49 AM To: fnc12/sqlite_orm ***@***.***> Cc: designPattern_OptimisticOfflineLock_sqlite_orm ***@***.***>; Author ***@***.***> Subject: Re: [fnc12/sqlite_orm] Feature/sql auditing (PR #1370) Which is the global/static object you refer to? The one defined as static in a function and returns as a ref But I am using singleton pattern This comment doesn't make any sense — Reply to this email directly, view it on GitHub <#1370 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABI6OFJQDXRWCTVVNJJ56YD2JFO7ZAVCNFSM6AAAAABT7X2B5SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNZRGY4DMMRQGA> . You are receiving this because you authored the thread. <https://github.com/notifications/beacon/ABI6OFPARYGTX6KSTDSVMT32JFO7ZA5CNFSM6AAAAABT7X2B5SWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTUZJDITQ.gif> Message ID: ***@***.*** ***@***.***> >

fnc12 · 2025-01-05T17:25:02Z

I am following Dimitri Nesteruk’ “Design patterns in Modern C++” and Fedor Pikus’ “Hands-on design patterns with C++” both of which implement a safe way for singletons.

Good for you. Still it is not acceptable for sqlite_orm cause singleton is anti-pattern

juandent · 2025-01-05T17:28:11Z

This is no way to react! “Good for you” you say! Can you be more sarcastic? From: Yevgeniy Zakharov ***@***.***> Sent: Sunday, January 5, 2025 11:25 AM To: fnc12/sqlite_orm ***@***.***> Cc: designPattern_OptimisticOfflineLock_sqlite_orm ***@***.***>; Mention ***@***.***> Subject: Re: [fnc12/sqlite_orm] Feature/sql auditing (PR #1370) I am following Dimitri Nesteruk’ “Design patterns in Modern C++” and Fedor Pikus’ “Hands-on design patterns with C++” both of which implement a safe way for singletons. Good for you. Still it is not acceptable for sqlite_orm cause singleton is anti-pattern — Reply to this email directly, view it on GitHub <#1370 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABI6OFLB7TE35GRTYLF27VD2JFTIHAVCNFSM6AAAAABT7X2B5SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNZRGY4TKNZYHA> . You are receiving this because you were mentioned. <https://github.com/notifications/beacon/ABI6OFOGQVFBM776UHJM6V32JFTIHA5CNFSM6AAAAABT7X2B5SWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTUZJD3KY.gif> Message ID: ***@***.*** ***@***.***> >

fnc12 · 2025-01-05T17:33:24Z

This is no way to react!
“Good for you” you say! Can you be more sarcastic?

I am not sarcastic at all. Please don't ignore my other comments

fnc12 · 2025-01-08T17:06:07Z

Discussed in DM. Closing

juandent added 2 commits December 20, 2024 12:50

Testing clang-format 17.0.1

4d4a76c

first implementation of sql_auditing

f32b3e0

fnc12 self-requested a review December 22, 2024 12:57

fnc12 requested changes Dec 22, 2024

View reviewed changes

Working fine for global and storage-based auditors

a1d9665

fnc12 requested changes Jan 3, 2025

View reviewed changes

fnc12 closed this Jan 8, 2025

Feature/sql auditing #1370

Feature/sql auditing #1370

Conversation

juandent commented Dec 20, 2024

fnc12 commented Dec 22, 2024

Choose a reason for hiding this comment

juandent Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juandent Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnc12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juandent Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

juandent commented Jan 5, 2025 via email

fnc12 commented Jan 5, 2025

juandent commented Jan 5, 2025 via email

juandent commented Jan 5, 2025 via email

fnc12 commented Jan 5, 2025

fnc12 commented Jan 5, 2025

juandent commented Jan 5, 2025 via email

fnc12 commented Jan 5, 2025

juandent commented Jan 5, 2025 via email

fnc12 commented Jan 5, 2025

fnc12 commented Jan 8, 2025

juandent Dec 22, 2024 •

edited

Loading

juandent Dec 22, 2024 •

edited

Loading

juandent Jan 3, 2025 •

edited

Loading