JTable Not handling default values correctly in MSSQL 2008+ #11056

williamsandy · 2016-07-08T17:24:45Z

Pull Request for Issue # .

Summary of Changes

Testing Instructions

Default values are not being assigned correctly for data types such as
Integer, Date and Datetime. So as a result it is causing the insert
query to fail. I have tested this on SQL SERVER 2008-2016.

The root cause of this issue lies with the getTableColumns in the SQL
server driver class, which relies on using the information_schema views
to get the information about the columns. The default values it
gets for the table columns are from the underlying system comments table
and as a result it is not in a format that can be injected as is into a
INSERT Query.

Below is a partial list of how the syscomments table stores default
values for the following data types:

Datatype : int
Actual default Value : 0
Value in syscomments table: ((0))

Datatype : int
Actual default Value : 10
Value in syscomments table: ((10))

Datatype : Datetime
Actual default Value : ‘1900-01-01T00:00:00.000’
Value in syscomments table: (‘1900-01-01T00:00:00.000’)

Datatype : Datetime
Actual default Value : getdate()
Value in syscomments table: (getdate())

Datatype : Date
Actual default Value : ‘1900-01-01’
Value in syscomments table: (‘1900-01-01’)

Datatype : Varchar
Actual default Value : ''
Value in syscomments table: ('')

Datatype : Varchar
Actual default Value : '1234'
Value in syscomments table: ('1234')

Datatype : NVarchar
Actual default Value : N''
Value in syscomments table: (N'')

Datatype : NVarchar
Actual default Value : N'1234'
Value in syscomments table: (N'1234')

Datatype : Varchar
Actual default Value : NULL
Value in syscomments table: (NULL)

You see the general pattern that the default value stored in the
syscomments table is wrapped around in curly brackets, whether that be
single or double depends on the data type.

Now the current implementation of the getTableColumns function only
deals with nvarchar data types by assuming every default string value
will be an empty string so defaults to that value ignoring the actual database value. This works but IMHO it is a bit too simplistic in that it does not allow for default string
values that is not an empty string value. Also it does not deal with
other data types such as datetime, date etc... So as a result the INSERT
query will still blow up when you try for example to pass the value ‘((0))’
for an integer data type.

My change just parses the value by removing the superfluous brackets and
returns the actual value for the data type in the correct format.

So for example the value ‘((0))’ will be returned as 0, (N‘1’) will
be returned as ‘1’ and value (getdate()) will be returned as getdate() and so on ....

Default values are not being assigned correctly for data types such as Integer, Date and Datetime. So as a result it is causing the insert query to fail. I have tested this on SQL SERVER 2008-2016. The root cause of this issue lies with the getTableColumns in the SQL server driver class, which relies on using the information_schema views to get the information about the columns. However the default values it gets for the table columns are from the underlying system comments table and as a result it is not in a format that can be injected as is into a INSERT Query. Below is a partial list of how the syscomments table stores default values for the following data types: Datatype Actual default Value Value in syscomments table Int 0 ((0)) Int 10 ((1)) DataTime ‘1900-01-01 00:00:00’ (‘1900-01-01 00:00:00’) DataTime ‘1900-01-01T00:00:00.000’ (‘1900-01-01T00:00:00.000’) DataTime getdate() (getdate()) Date ‘1900-01-01’ (‘1900-01-01’) Varchar ‘’ (’’) Varchar ‘1234’ (‘1234’) Nvarchar N‘’ (N’’) Nvarchar N‘1234’ (N‘1234’) Nvarchar NULL (NULL) You see the general idea that the default value stored in the syscomments table is wrapped around in curly brackets, whether that be single or double depends on the data type. Now the current implementation of the getTableColumns function only deals with nvarchar data types by assuming every default string value will be an empty string so defaults to that value. This works but IMHO is a bit too simplistic in that it does not allow for default string values that is not an empty string value. Also it does not deal with other data types such as datetime, date etc... So as a result the INSERT query will blow up when you try for example to pass the value ‘((0))’ for an integer data type. My change just parses the value by removing the superfluous brackets and returns the actual value for the data type in the correct format. So for example the value ‘((0))’ will be returned as ‘0’ and (N‘1’) will be returned as ‘1’.

brianteeman · 2016-08-02T10:09:36Z

Hi

Does #11396 resolve this issue?

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11056.}

williamsandy · 2016-08-02T11:48:11Z

Unfortunately no, this is because the fault lies with the getTableColums method of the JDatabaseDriverSqlsrv that tries to get the column details for a given database table.

The issue here is it uses this query below to get the default values for the table rows say for example '#__user_notes'--which to be honest you don't really have another way of getting that information.

SELECT column_name as Field, data_type as Type, is_nullable as 'Null', column_default as 'Default'
FROM information_schema.columns WHERE table_name = 'nqu7n_user_notes'

[Note 'nqu7n' is the prefix used for my database please change this with the one you are using]

If you run this query in any version of MS SQL Server and look at what it is given you for the default values, you will see straight away that this is going to cause issues. Please see my description above which goes in more depth.

The SQL Issue arises (Cannot convert value as Int or Cannot convert blah as datetime2 etc..) when you are using JTable to create a new row in a database table, and you don't explicitly supply the default values for every column that has a default value.

If you apply my patch, it will filter the default values and return them in a correct format.

brianteeman · 2016-08-02T11:52:48Z

Sorry I don't have mssql to test and to be honest with you it looks like there are very few <100 users of joomla on mssql

williamsandy · 2016-08-02T12:40:58Z

Thank you for your reply. I do understand that this is difficult to test if you don’t have MSSQL but the Joomla team clearly have put a lot of effort in making Joomla work on non-MySQL servers, and it would be a shame to see that work go to waste with such a simple fix.

On our side, we have thoroughly tested it as we have been working on a live project
that uses SQL Server so it is important to us that this fix can be applied so we can continue to upgrade Joomla for that project for security patches. The reason I am pushing this change is due to this issue #11280
(#11280 (comment)) I have committed to fixing this issue involving MSSQL. However, I cannot apply the fix for that one because it is being held up by this PR.

I also believe that there is a duty of care issue as Joomla states it is MSSQL compliant. We would be more than happy to fix these issues; we just need someone to test our patches so we will be able to fix issues
like this and the other one mentioned in this PR

brianteeman · 2016-08-02T12:45:04Z

Unfortunately the people who did the work to support mssql disappeared as soon as they committed the code. Right now my understanding is that the PLT are re-evaluating if Joomla should continue to support mssql as we don't have the resources to test and maintain it and the number of users is so small. Maybe @wilsonge or @rdeutz caan comment

rdeutz · 2016-08-02T13:07:55Z

We would be more than happy to support more database types. As far as I know then besides PostgreSQL all others are broken. I don't think it is realistic and we don't have the people and knowledge to support more.

I would merge it when it has 2 successful tests as we usually do

williamsandy · 2016-08-02T13:27:39Z

@sovainfo and @roland-d would you be so kind to test this PR. Thanks.

roland-d · 2016-08-02T15:19:52Z

I agree with @rdeutz here. I don't have any MSSQL server available, hopefully others have and can check. Although community support for MSSQL is nearly non-existent as we found out.

sovainfo · 2016-08-02T19:18:07Z

Tried to submit a positive test with issues.joomla.org, but that failed.

Used this code to test:
<?php
$datatypes = array("((0))", "((10))", "('1900-01-01T00:00:00.000')", "(getdate())", "('1900-01-01')", "('')", "('1234')", "(N'1234')", "(NULL)");
foreach ($datatypes as $datatype) {
    echo $datatype;echo " : ";
    echo preg_replace("/(^($\(|\('|\(N'|\()|(('$|(?<!$)$\)|\))$))/i", '', $datatype);
    echo "\n";
}
?>
Output:
((0)) : 0
((10)) : 10
('1900-01-01T00:00:00.000') : 1900-01-01T00:00:00.000
(getdate()) : getdate()
('1900-01-01') : 1900-01-01
('') :
('1234') : 1234
(N'1234') : 1234
(NULL) : NULL

sovainfo · 2016-08-02T19:22:20Z

I have tested this item ✅ successfully on 0fba84c

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11056.}

williamsandy · 2016-08-03T10:03:33Z

@waader and @lmcculley Hi gents I am trying to get a fix in for those of us that have MSSQL and I am wondering if you could test this PR so that we can get this fix in

marksmeed · 2016-08-05T08:32:57Z

@Bakual would you be so kind to test the fix?

waader · 2016-08-06T16:28:22Z

I have tested this item ✅ successfully on 0fba84c

Thanks @williamsandy! Just ping me if you have other mssql patches to test. But it may take some days as I am busy right now.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11056.}

rdeutz · 2016-08-06T16:31:00Z

Thanks for testing this, RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11056.}

zero-24 · 2016-08-08T19:10:46Z

@joomla-cms-bot please 😄

brianteeman · 2016-08-08T19:13:01Z

@zero-24 I am changing ! my name

zero-24 · 2016-08-08T19:25:11Z

hehe 😄 Thanks.

…-cms#11056

…1056) * JTable Not handling default values correctly in MSSQL 2008+ Default values are not being assigned correctly for data types such as Integer, Date and Datetime. So as a result it is causing the insert query to fail. I have tested this on SQL SERVER 2008-2016. The root cause of this issue lies with the getTableColumns in the SQL server driver class, which relies on using the information_schema views to get the information about the columns. However the default values it gets for the table columns are from the underlying system comments table and as a result it is not in a format that can be injected as is into a INSERT Query. Below is a partial list of how the syscomments table stores default values for the following data types: Datatype Actual default Value Value in syscomments table Int 0 ((0)) Int 10 ((1)) DataTime ‘1900-01-01 00:00:00’ (‘1900-01-01 00:00:00’) DataTime ‘1900-01-01T00:00:00.000’ (‘1900-01-01T00:00:00.000’) DataTime getdate() (getdate()) Date ‘1900-01-01’ (‘1900-01-01’) Varchar ‘’ (’’) Varchar ‘1234’ (‘1234’) Nvarchar N‘’ (N’’) Nvarchar N‘1234’ (N‘1234’) Nvarchar NULL (NULL) You see the general idea that the default value stored in the syscomments table is wrapped around in curly brackets, whether that be single or double depends on the data type. Now the current implementation of the getTableColumns function only deals with nvarchar data types by assuming every default string value will be an empty string so defaults to that value. This works but IMHO is a bit too simplistic in that it does not allow for default string values that is not an empty string value. Also it does not deal with other data types such as datetime, date etc... So as a result the INSERT query will blow up when you try for example to pass the value ‘((0))’ for an integer data type. My change just parses the value by removing the superfluous brackets and returns the actual value for the data type in the correct format. So for example the value ‘((0))’ will be returned as ‘0’ and (N‘1’) will be returned as ‘1’. * Forgot to add rule for protecting default function computed values

williamsandy added 2 commits July 8, 2016 16:51

Forgot to add rule for protecting default function computed values

0fba84c

rdeutz added this to the Joomla 3.6.3 milestone Aug 6, 2016

rdeutz added RTC This Pull Request is Ready To Commit PR-staging labels Aug 6, 2016

wilsonge merged commit 6c5a25c into joomla:staging Aug 7, 2016

zero-24 mentioned this pull request Aug 8, 2016

The Bot don't run for the CMS Issue #11353 joomla/jissues#850

Closed

brianteeman removed the RTC This Pull Request is Ready To Commit label Aug 8, 2016

mbabker added a commit to joomla-framework/database that referenced this pull request Aug 14, 2016

SQL Server doesn't handle defaults correctly, backports joomla/joomla…

1234283

…-cms#11056

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JTable Not handling default values correctly in MSSQL 2008+ #11056

JTable Not handling default values correctly in MSSQL 2008+ #11056

williamsandy commented Jul 8, 2016 •

edited by joomla-cms-bot

Loading

brianteeman commented Aug 2, 2016

williamsandy commented Aug 2, 2016 •

edited

Loading

brianteeman commented Aug 2, 2016 via email

williamsandy commented Aug 2, 2016

brianteeman commented Aug 2, 2016 via email

rdeutz commented Aug 2, 2016

williamsandy commented Aug 2, 2016

roland-d commented Aug 2, 2016

sovainfo commented Aug 2, 2016

sovainfo commented Aug 2, 2016

williamsandy commented Aug 3, 2016

marksmeed commented Aug 5, 2016

waader commented Aug 6, 2016

rdeutz commented Aug 6, 2016

zero-24 commented Aug 8, 2016

brianteeman commented Aug 8, 2016

zero-24 commented Aug 8, 2016

JTable Not handling default values correctly in MSSQL 2008+ #11056

JTable Not handling default values correctly in MSSQL 2008+ #11056

Conversation

williamsandy commented Jul 8, 2016 • edited by joomla-cms-bot Loading

Summary of Changes

Testing Instructions

brianteeman commented Aug 2, 2016

williamsandy commented Aug 2, 2016 • edited Loading

brianteeman commented Aug 2, 2016 via email

williamsandy commented Aug 2, 2016

brianteeman commented Aug 2, 2016 via email

rdeutz commented Aug 2, 2016

williamsandy commented Aug 2, 2016

roland-d commented Aug 2, 2016

sovainfo commented Aug 2, 2016

sovainfo commented Aug 2, 2016

williamsandy commented Aug 3, 2016

marksmeed commented Aug 5, 2016

waader commented Aug 6, 2016

rdeutz commented Aug 6, 2016

zero-24 commented Aug 8, 2016

brianteeman commented Aug 8, 2016

zero-24 commented Aug 8, 2016

williamsandy commented Jul 8, 2016 •

edited by joomla-cms-bot

Loading

williamsandy commented Aug 2, 2016 •

edited

Loading