Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(SQLSRV): Added more detailed MSSQL error message to CI log #9338

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

eSkiSo
Copy link
Contributor

@eSkiSo eSkiSo commented Dec 26, 2024

Description
Adds a more detailed error message to log, includes stack trace and sql executed.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Adds a more details error message to log, includes stack trace and sql executed
@michalsn
Copy link
Member

I'm not sure if I like this. Why do you need this?

Too much detail can make logs harder to navigate, especially in production environments where brevity is preferred.

And most importantly, it would have to be applied to all database handlers.

@eSkiSo
Copy link
Contributor Author

eSkiSo commented Dec 26, 2024

Hello Michal,
"Too much detail can make logs harder to navigate, especially in production environments where brevity is preferred."

  • This actually makes the log easier to read as it is not just a generic MSSQL error but a more detailed structed error which make it easy to review and therefor quicker to fix.
    I've been manually patching this after each CI upgrade as it does really help alot quickly pin-pointing issues and MSSQL general error messages are sometimes too generic.
    This will structure it a bit like this (im typing this by memory as im not at work at the moment and cant be more precise but for example):

Currenctly

ERROR - 2024-12-23 11:32:08 --> Unknown column 't1.order_id' in 'on clause'

With this patch

ERROR - 2024-12-23 11:32:08 --> Unknown column 't1.order_id' in 'on clause'
[SQL]
SELECT t2.* 
FROM orders as t1 
LEFT JOIN items as t2 ON (t1.id = t2.order_id)
WHERE t1.order_id = 1
[END OF SQL]
in SYSTEMPATH/Database/BaseConnection.php on line 645.
--SYSTEMPATH/Database/BaseBuilder.php(1700): CodeIgniter\Database\BaseConnection->query()
----APPPATH/Models/Test_model.php(70): CodeIgniter\Database\BaseBuilder->countAllResults()
------APPPATH/Controllers/Test.php(875): App\Models\Test_model->getData()
--------APPPATH/Controllers/Test.php(554): App\Controllers\Test->_getData()
----------SYSTEMPATH/CodeIgniter.php(934): App\Controllers\Test->getData()
------------SYSTEMPATH/CodeIgniter.php(499): CodeIgniter\CodeIgniter->runController()
--------------SYSTEMPATH/CodeIgniter.php(368): CodeIgniter\CodeIgniter->handleRequest()
----------------FCPATH/index.php(67): CodeIgniter\CodeIgniter->run()

"And most importantly, it would have to be applied to all database handlers."

  • Not sure about the other drivers (as i dont use them) but MySQL already does something like this:
CRITICAL - 2024-12-23 11:32:08 --> Unknown column 't1.order_id' in 'on clause'
in SYSTEMPATH/Database/BaseConnection.php on line 645.
1 SYSTEMPATH/Database/BaseBuilder.php(1700): CodeIgniter\Database\BaseConnection->query()
2 APPPATH/Models/Test_model.php(70): CodeIgniter\Database\BaseBuilder->countAllResults()
3 APPPATH/Controllers/Test.php(875): App\Models\Test_model->getData()
4 APPPATH/Controllers/Test.php(554): App\Controllers\Test->_getData()
5 SYSTEMPATH/CodeIgniter.php(934): App\Controllers\Test->getData()
6 SYSTEMPATH/CodeIgniter.php(499): CodeIgniter\CodeIgniter->runController()
7 SYSTEMPATH/CodeIgniter.php(368): CodeIgniter\CodeIgniter->handleRequest()
8 FCPATH/index.php(67): CodeIgniter\CodeIgniter->run()

@michalsn
Copy link
Member

michalsn commented Dec 27, 2024

Okay, I just checked, and a detailed stack trace is already available for all the drivers. Just make sure to enable DBDebug option in the connection config group and the Config\Exceptions::log is set to true.

Displaying Logging the full SQL query is another case. This should be configurable for all the handlers as it might bring a risk of logging sensitive data as email addresses (or worse).

@paulbalandan
Copy link
Member

If we will accept this, the format should be consistent with other drivers.

@paulbalandan paulbalandan added the wrong branch PRs sent to wrong branch label Dec 27, 2024
@kenjis kenjis added tests needed Pull requests that need tests docs needed Pull requests needing documentation write-ups and/or revisions. labels Dec 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs needed Pull requests needing documentation write-ups and/or revisions. tests needed Pull requests that need tests wrong branch PRs sent to wrong branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants