Incompatibilities with SQL Server

[jeckyhl]

This bug report follows bug #36 ; I found some others incomptabilities with SQL Server

config : MantisBT 1.2.12 with SQL Server 2008 plugin-version : master (changeset 2a0e94beba)

1° Error when searching issues (Repositories > Search ; search for all changesets) saying that c.timestamp field is not in a aggregate function query :

SELECT COUNT(c.id) 
FROM mantis_plugin_Source_changeset_table AS c 
LEFT JOIN mantis_plugin_Source_repository_table AS r ON c.repo_id=r.id
ORDER BY c.timestamp DESC

This is caused by ORDER BY clause at the end of query

2° Again when searching issues --> error cannot compare TEXT fields query :

SELECT DISTINCT( c.id ), c.* 
FROM mantis_plugin_Source_changeset_table
 AS c LEFT JOIN mantis_plugin_Source_repository_table AS r 
 ON c.repo_id=r.id ORDER BY c.timestamp DESC

This is caused by DISTINCT clause.

My workarounds / solutions : 1° move $t_order variable from Source_Process_Filters(..) to find(..) function. The find(..) function becomes :

list( $t_filters, $t_filter_params ) = Source_Twomap( 'Source_Process_FilterOption', $this->filters );
list ( $t_query_tail, $t_params ) = Source_Process_Filters( $t_filters, $t_filter_params );
$t_order = 'ORDER BY c.timestamp DESC';

$t_count_query = "SELECT COUNT(c.id) $t_query_tail";
$t_full_query = "SELECT DISTINCT( c.id ), c.* $t_query_tail $t_order";
# ...

2° Simply delete DISTINCT keyword (seem not useful here) ini Source.FilterAPI.php, function find

[dregad]

1° your proposition makes sense to me, however I believe it would be better to leave the definition of the order clause in the Source_Process_Filters function, and change the return value as appropriate instead -- something like (not tested)

diff --git a/Source/Source.FilterAPI.php b/Source/Source.FilterAPI.php
index aceef05..de9f75b 100644
--- a/Source/Source.FilterAPI.php
+++ b/Source/Source.FilterAPI.php
@@ -65,10 +65,10 @@ class SourceFilter {

    function find( $p_page=1, $p_limit=25 ) {
        list( $t_filters, $t_filter_params ) = Source_Twomap( 'Source_Process_FilterOption', $this->
-       list ( $t_query_tail, $t_params ) = Source_Process_Filters( $t_filters, $t_filter_params );
+       list( $t_query_tail, $t_order, $t_params ) = Source_Process_Filters( $t_filters, $t_filter_p

        $t_count_query = "SELECT COUNT(c.id) $t_query_tail";
-       $t_full_query = "SELECT DISTINCT( c.id ), c.* $t_query_tail";
+       $t_full_query = "SELECT DISTINCT( c.id ), c.* $t_query_tail $t_order";

        $t_count = db_result( db_query_bound( $t_count_query, $t_params ) );

@@ -139,7 +139,7 @@ function Source_Process_Filters( $p_filters, $p_filter_params ) {

    $t_order = 'ORDER BY c.timestamp DESC';

-   return array( "$t_join $t_where $t_order", $t_params );
+   return array( "$t_join $t_where", $t_order, $t_params );
 }

 function Source_Process_FilterOption( $key, $option ) {

Regarding 2°, considering that @jreese specifically added the DISTINCT clause in commit faf9ca0196eee7dbe8757041ceeb633b43d35cb4, I would assume he had a valid reason for doing so, therefore we should be careful for any potential regression before removing it. John, any comment ?

[jeckyhl]

1°) I think your approach is indeed cleaner. Works fine for me (tested with MantisBT 1.2.12/SQL Server 2008)

[dregad]

Reopened as point 2° has not yet been addressed

[amyreese]

Assuming that you are adding some form of correction in the code for duplicate result rows, then I would imagine the DISTINCT is not necessary. But I don't even remember the context of when/why I added that, so your guess is as good as mine.