salesagility / SuiteCRM

SuiteCRM - Open source CRM for the world
https://www.suitecrm.com
GNU Affero General Public License v3.0
4.27k stars 2.03k forks source link

`LIMIT` is not compatible with Microsoft SQL Server #10350

Open chris001 opened 5 months ago

chris001 commented 5 months ago

Issue

LIMIT is not compatible with Microsoft SQL Server, so all hard coded SQL query strings containing LIMIT should be changed to be compatible. For example: SuiteCRM/include/database/MssqlManager.php

Expected Behavior

Code such as in issue 9990 should work on all SQL servers, not just MariaDB/MySQL.

Actual Behavior

Error, the SQL query fails, the feature malfunctions.

Possible Fix

Replace LIMIT with standard SQL. Use the DbManager class, which makes SQL compatible with the database in use.

Steps to Reproduce

  1. Search the code for all instances of LIMIT in SQL queries, e.g. Indexing in Elasticsearch.
  2. Try to run the features which have LIMIT queries, on MS SQL Server.
  3. Error. The features don't work. Yet it should work trouble free on MS SQL Server same as MariaDB/MySQL.

Context

Medium priority. Many companies infrastructure use Microsoft SQL Server and have established backup and redundancy.

Your Environment

JanSiero commented 5 months ago

Hi @chris001 is this really an issue for MS SQL? In the file

https://github.com/salesagility/SuiteCRM/blob/hotfix/include/database/MssqlManager.php

I don't see any usage of the limit keyword. Moreover, limitting seems to have been implemented using the top and row_number keywords for MS SQL here.

chris001 commented 5 months ago

Hi @JanSiero In MssqlManager, it's got a method limitQuery that does the "limit" the right way, it generates proper SQL compatible with SQL Server. Example: https://github.com/salesagility/SuiteCRM/blob/b39d9610f705807ce4b595c04031b001c2f0c819/modules/Schedulers/_AddJobsHere.php#L453

In many places in the code base, the SQL is hard coded to use LIMIT which will fail on SQL Server. Example 1: https://github.com/salesagility/SuiteCRM/blob/b39d9610f705807ce4b595c04031b001c2f0c819/modules/Emails/PessimisticLock.php#L75 Example 2: https://github.com/salesagility/SuiteCRM/blob/b39d9610f705807ce4b595c04031b001c2f0c819/modules/Emails/PessimisticLock.php#L80 Example 3: https://github.com/salesagility/SuiteCRM/blob/b39d9610f705807ce4b595c04031b001c2f0c819/include/SubPanel/SubPanelRowCounter.php#L215 And more...

JanSiero commented 5 months ago

In many places in the code base, the SQL is hard coded to use LIMIT

Hi Chris, I agree that hard coded SQL in SuiteCRM is a problem and should be avoided, so this is a valid issue.

However, I am not sure if issue #9990 (elasticsearch) is a good example for hard coded SQL, as all SQLs in the Elasticsearch library are produced by SugarBean::get_list which in turn uses the database manager. In fact, the user who posted #9990 is using MySQL and not MS SQL Server, which is the reason why we see the limit keyword in the SQL in his stacktrace.