[PATCH v2 0/1] MR2498: msado15: Implement _Recordset Find

[Alistair Leslie-Hughes (＠alesliehughes)]

Find only supports one Column to filter on.

The parser for this will be the same for both Find/Filter with the only different
is the Filter supports multiple columns.

In trying to keep this patch small, only WSTR/BSTR/Integer have been supported. (The Common cases).

--
  v2: msado15: Implement _Recordset Find

https://gitlab.winehq.org/wine/wine/-/merge_requests/2498

[PATCH v2 1/1] msado15: Implement _Recordset Find

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes

Find only supports one Column to filter on.

The parser for this will be the same for both Find/Filter with the only different
is the Filter supports multiple columns.

In trying to keep this patch small, only WSTR/BSTR/Integer have been supported. (The Common cases).
---
 dlls/msado15/Makefile.in       |   6 +
 dlls/msado15/filter.l          |  99 ++++++++++++++
 dlls/msado15/filter.y          | 130 ++++++++++++++++++
 dlls/msado15/msado15_private.h |  58 +++++++++
 dlls/msado15/recordset.c       | 232 ++++++++++++++++++++++++++++-----
 dlls/msado15/tests/Makefile.in |   2 +-
 dlls/msado15/tests/msado15.c   | 184 +++++++++++++++++++++++++-
 7 files changed, 677 insertions(+), 34 deletions(-)
 create mode 100644 dlls/msado15/filter.l
 create mode 100644 dlls/msado15/filter.y

diff --git a/dlls/msado15/Makefile.in b/dlls/msado15/Makefile.in
index 9c1fba502c5..0c10402269e 100644
--- a/dlls/msado15/Makefile.in
+++ b/dlls/msado15/Makefile.in
@@ -8,6 +8,12 @@ C_SRCS = \
    recordset.c \
    stream.c

+LEX_SRCS = \
+	filter.l
+
+BISON_SRCS = \
+	filter.y
+
 IDL_SRCS = \
    msado15_classes.idl \
    msado15_tlb.idl
diff --git a/dlls/msado15/filter.l b/dlls/msado15/filter.l
new file mode 100644
index 00000000000..156df1524ee
--- /dev/null
+++ b/dlls/msado15/filter.l
@@ -0,0 +1,99 @@
+/*
+ * Copyright 2021 Alistair Leslie-Hughes
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+ */
+%option noyywrap
+%option caseless
+%option noinput nounput never-interactive
+%option reentrant bison-bridge
+
+%{
+#include <stdio.h>
+
+#include "msado15_private.h"
+#include "filter.tab.h"
+
+#include "wine/debug.h"
+
+WINE_DEFAULT_DEBUG_CHANNEL(msado15);
+
+%}
+
+%%
+
+[ \r\t]         { /* ignore all whitespace */ } ;
+[0-9]+.[0-9]+ 	{ yylval->fval = atof(yytext); return T_FLOAT; }
+[-]?[0-9]+      { yylval->ival = atoi(yytext); return T_INT;   }
+
+'[^'].*'        {
+                    yylval->sval = malloc(strlen(yytext)-1);
+                    memcpy(yylval->sval, yytext+1, strlen(yytext)-2);
+                    yylval->sval[strlen(yytext)-2] = '\0';
+                    return T_STRING;
+                }
+#.*#            {
+                    yylval->sval = malloc(strlen(yytext)-1);
+                    memcpy(yylval->sval, yytext+1, strlen(yytext)-2);
+                    yylval->sval[strlen(yytext)-2] = '\0';
+                    return T_STRING;
+                }
+
+\n              { return T_NEWLINE;}
+"("             { /* Ignored for now */ }
+")"             { /* Ignored for now */ }
+
+"="             { return (yylval->operation = TOKEN_EQ); }
+">"             { return (yylval->operation = TOKEN_GREATER); }
+"<"             { return (yylval->operation = TOKEN_LESS); }
+"<>"            { return (yylval->operation = TOKEN_NOT_EQ); }
+">="            { return (yylval->operation = TOKEN_GREATER_EQ); }
+"<="            { return (yylval->operation = TOKEN_LESS_EQ); }
+like            { return TOKEN_LIKE; }
+
+[A-Za-z_0-9]*    { yylval->sval = strdup(yytext); return TOKEN_COLUMN; }
+
+%%
+
+int recordset_parse_filter(WCHAR *filter, struct recordset *recordset)
+{
+    char *str;
+    parser_param p;
+    YY_BUFFER_STATE buffer;
+    int len;
+    int ret;
+
+    TRACE("(%s)\n", debugstr_w(filter));
+    memset(&p, 0, sizeof(parser_param));
+    p.recordset = recordset;
+
+    len = WideCharToMultiByte(CP_ACP, 0, filter, wcslen(filter)+1, 0, 0, NULL, NULL);
+    str = malloc(len);
+    if (!str)
+        return 2; /* Out of Memory */
+    len = WideCharToMultiByte(CP_ACP, 0, filter, wcslen(filter)+1, str, len, NULL, NULL);
+
+    yylex_init(&p.yyscanner);
+    yyset_extra(&p, p.yyscanner);
+
+    buffer = yy_scan_string(str, p.yyscanner);
+    ret = yyparse(&p, p.yyscanner);
+    yy_delete_buffer(buffer, p.yyscanner);
+    yylex_destroy(p.yyscanner);
+
+    free(str);
+
+    return ret;
+}
\ No newline at end of file
diff --git a/dlls/msado15/filter.y b/dlls/msado15/filter.y
new file mode 100644
index 00000000000..d02bd76b2a7
--- /dev/null
+++ b/dlls/msado15/filter.y
@@ -0,0 +1,130 @@
+/*
+ * Copyright 2021 Alistair Leslie-Hughes
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+ */
+%{
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#define COBJMACROS
+#include "objbase.h"
+#include "msado15_backcompat.h"
+
+#include "oledb.h"
+#include "sqlucode.h"
+
+#include "msado15_private.h"
+
+#include "wine/debug.h"
+
+WINE_DEFAULT_DEBUG_CHANNEL(msado15);
+
+void yyerror(parser_param* param, void const* scanner, char const* msg);
+parser_param* yyget_extra ( void* yyscanner );
+%}
+
+%define api.pure
+%parse-param {parser_param* p}
+%parse-param {void* scanner}
+%lex-param {yyscan_t scanner}
+
+%union {
+	int operation;
+	int join;
+
+	/* Possible values types */
+	int ival;
+	float fval;
+	char *sval;
+}
+
+%code provides
+{
+int yylex(YYSTYPE *yylval, void *scanner);
+}
+
+
+%token<ival> T_INT
+%token<fval> T_FLOAT
+%token<sval> T_STRING
+
+%token T_NEWLINE
+
+%type<operation> operations
+
+%token<operation> TOKEN_GREATER
+%token<operation> TOKEN_GREATER_EQ
+%token<operation> TOKEN_LESS
+%token<operation> TOKEN_LESS_EQ
+%token<operation> TOKEN_NOT_EQ
+%token<operation> TOKEN_EQ
+%token<operation> TOKEN_LIKE
+
+%token<sval> TOKEN_COLUMN
+
+
+%start filter
+
+%%
+
+filter: %empty
+	| filter line
+;
+
+line: T_NEWLINE
+	| TOKEN_COLUMN operations T_INT {
+		filter_add_integer_column(p->recordset, $1, $2, $3);
+	}
+	| TOKEN_COLUMN operations T_FLOAT {
+		filter_add_float_column(p->recordset, $1, $2, $3);
+	}
+	| TOKEN_COLUMN operations T_STRING {
+		filter_add_string_column(p->recordset, $1, $2, $3);
+	}
+
+	/* Like is always followed by a string */
+	| TOKEN_COLUMN TOKEN_LIKE T_STRING {
+		int i = 0;
+		char *stripped = malloc(strlen($3) + 1);
+		while(*$3 != '\0')
+		{
+			if(*$3 != '*')
+			{
+				stripped[i++] = *$3;
+			}
+			$3++;
+		}
+		TRACE("Stripped: %s, %s\n", debugstr_a($3), debugstr_a(stripped));
+		filter_add_string_column(p->recordset, $1, $2, stripped);
+		free(stripped);
+	}
+;
+
+operations: TOKEN_GREATER
+	| TOKEN_GREATER_EQ
+	| TOKEN_LESS
+	| TOKEN_LESS_EQ
+	| TOKEN_NOT_EQ
+	| TOKEN_EQ
+;
+
+%%
+
+void yyerror(parser_param* param, void const* scanner, char const* msg)
+{
+    FIXME("%p, %p, error: %s\n", param, scanner, msg);
+}
\ No newline at end of file
diff --git a/dlls/msado15/msado15_private.h b/dlls/msado15/msado15_private.h
index f7062c94b27..f14494e1c37 100644
--- a/dlls/msado15/msado15_private.h
+++ b/dlls/msado15/msado15_private.h
@@ -19,6 +19,64 @@
 #ifndef _WINE_MSADO15_PRIVATE_H_
 #define _WINE_MSADO15_PRIVATE_H_

+#define COBJMACROS
+#include "objbase.h"
+#include "msado15_backcompat.h"
+#include "oledb.h"
+#include "sqlucode.h"
+
+#include "wine/list.h"
+#include "wine/debug.h"
+
+struct column_filter
+{
+    struct list entry;
+
+    LONG column;
+    LONG condition;
+    VARIANT value;
+};
+
+struct fields;
+struct recordset
+{
+    _Recordset         Recordset_iface;
+    ADORecordsetConstruction ADORecordsetConstruction_iface;
+    ISupportErrorInfo  ISupportErrorInfo_iface;
+    LONG               refs;
+    LONG               state;
+    struct fields     *fields;
+    LONG               count;
+    LONG               allocated;
+    LONG               index;
+    VARIANT           *data;
+    CursorLocationEnum cursor_location;
+    CursorTypeEnum     cursor_type;
+    IRowset           *row_set;
+    EditModeEnum      editmode;
+    VARIANT            filter;
+    BSTR              criteria;
+
+    DBTYPE            *columntypes;
+    HACCESSOR         *haccessors;
+
+    struct list       filters;
+};
+
+typedef struct _parser_param
+{
+    void* yyscanner;
+
+    struct recordset *recordset;
+} parser_param;
+
+#define YY_EXTRA_TYPE parser_param*
+
+int recordset_parse_filter(WCHAR *filter, struct recordset *recordset) DECLSPEC_HIDDEN;
+void filter_add_integer_column(struct recordset *recordset, char *name, LONG cond, LONG value) DECLSPEC_HIDDEN;
+void filter_add_float_column(struct recordset *recordset, char *name, LONG cond, float value) DECLSPEC_HIDDEN;
+void filter_add_string_column(struct recordset *recordset, char *name, LONG cond, char *value) DECLSPEC_HIDDEN;
+
 #define MAKE_ADO_HRESULT( err ) MAKE_HRESULT( SEVERITY_ERROR, FACILITY_CONTROL, err )

HRESULT Command_create( void ** ) DECLSPEC_HIDDEN;
diff --git a/dlls/msado15/recordset.c b/dlls/msado15/recordset.c
index 5522a3c9e61..236f2a391b9 100644
--- a/dlls/msado15/recordset.c
+++ b/dlls/msado15/recordset.c
@@ -20,40 +20,13 @@
 #include <assert.h>
 #include "windef.h"
 #include "winbase.h"
-#define COBJMACROS
-#include "objbase.h"
-#include "msado15_backcompat.h"
-#include "oledb.h"
-#include "sqlucode.h"
-
+#include "msado15_private.h"
 #include "wine/debug.h"

-#include "msado15_private.h"
+#include "filter.tab.h"

-WINE_DEFAULT_DEBUG_CHANNEL(msado15);

-struct fields;
-struct recordset
-{
-    _Recordset         Recordset_iface;
-    ADORecordsetConstruction ADORecordsetConstruction_iface;
-    ISupportErrorInfo  ISupportErrorInfo_iface;
-    LONG               refs;
-    LONG               state;
-    struct fields     *fields;
-    LONG               count;
-    LONG               allocated;
-    LONG               index;
-    VARIANT           *data;
-    CursorLocationEnum cursor_location;
-    CursorTypeEnum     cursor_type;
-    IRowset           *row_set;
-    EditModeEnum      editmode;
-    VARIANT            filter;
-
-    DBTYPE            *columntypes;
-    HACCESSOR         *haccessors;
-};
+WINE_DEFAULT_DEBUG_CHANNEL(msado15);

struct fields
 {
@@ -986,6 +959,66 @@ static inline WCHAR *heap_strdupAtoW(const char *str)
     return ret;
 }

+static LONG find_column_index(struct fields *fields, char *name)
+{
+    VARIANT column;
+    LONG ret = -1;
+    ULONG index;
+    WCHAR *str = heap_strdupAtoW(name);
+
+    V_VT(&column) = VT_BSTR;
+    V_BSTR(&column) = SysAllocString(str);
+
+    free(str);
+
+    if (map_index(fields, &column, &index) == S_OK)
+        ret = index;
+    VariantClear(&column);
+
+    return ret;
+}
+
+void filter_add_integer_column(struct recordset *recordset, char *name, LONG cond, LONG value)
+{
+    struct column_filter *filter;
+
+    filter = calloc(1, sizeof(*filter));
+    filter->column = find_column_index(recordset->fields, name);
+    filter->condition = cond;
+    V_VT(&filter->value) = VT_I4;
+    V_I4(&filter->value) = value;
+
+    list_add_head(&recordset->filters, &filter->entry);
+}
+
+void filter_add_float_column(struct recordset *recordset, char *name, LONG cond, float value)
+{
+    struct column_filter *filter;
+
+    filter = calloc(1, sizeof(*filter));
+    filter->column = find_column_index(recordset->fields, name);
+    filter->condition = cond;
+    V_VT(&filter->value) = VT_I8;
+    V_I8(&filter->value) = value;
+
+    list_add_head(&recordset->filters, &filter->entry);
+}
+
+void filter_add_string_column(struct recordset *recordset, char *name, LONG cond, char *value)
+{
+    struct column_filter *filter;
+    WCHAR *str = heap_strdupAtoW(value);
+
+    filter = calloc(1, sizeof(*filter));
+    filter->column = find_column_index(recordset->fields, name);
+    filter->condition = cond;
+    V_VT(&filter->value) = VT_BSTR;
+    V_BSTR(&filter->value) = SysAllocString(str);
+    free(str);
+
+    list_add_head(&recordset->filters, &filter->entry);
+}
+
 static HRESULT WINAPI fields_get_Item( Fields *iface, VARIANT index, Field **obj )
 {
     struct fields *fields = impl_from_Fields( iface );
@@ -1199,6 +1232,20 @@ static HRESULT fields_create( struct recordset *recordset, struct fields **ret )
     return S_OK;
 }

+static void clear_recordset_filter(struct recordset *recordset)
+{
+    struct column_filter *filters, *filters2;
+
+    LIST_FOR_EACH_ENTRY_SAFE(filters, filters2, &recordset->filters, struct column_filter, entry)
+    {
+        list_remove(&filters->entry);
+        VariantClear(&filters->value);
+        free(filters);
+    }
+    SysFreeString(recordset->criteria);
+    recordset->criteria = NULL;
+}
+
 static inline struct recordset *impl_from_Recordset( _Recordset *iface )
 {
     return CONTAINING_RECORD( iface, struct recordset, Recordset_iface );
@@ -1223,6 +1270,8 @@ static void close_recordset( struct recordset *recordset )
     ULONG i;
     IAccessor *accessor;

+    clear_recordset_filter(recordset);
+
     if (recordset->haccessors)
         IRowset_QueryInterface(recordset->row_set, &IID_IAccessor, (void**)&accessor);

@@ -2327,12 +2376,129 @@ static HRESULT WINAPI recordset_put_MarshalOptions( _Recordset *iface, MarshalOp
     return E_NOTIMPL;
 }

+static BOOL column_match_string(int condition, BSTR col_data, BSTR filter_data)
+{
+    BOOL match = FALSE;
+
+    switch (condition)
+    {
+        case TOKEN_EQ:
+            match = lstrcmpW(col_data, filter_data) == 0;
+            break;
+        case TOKEN_NOT_EQ:
+            match = lstrcmpW(col_data, filter_data) != 0;
+            break;
+        case TOKEN_LIKE:
+            FIXME("LIKE currently not supported\n");
+            break;
+        default:
+            FIXME("Unsupported condition %d\n", condition);
+    }
+
+    return match;
+}
+
+static BOOL column_match_integer(int condition, INT col_data, INT filter_data)
+{
+    BOOL match = FALSE;
+
+    switch (condition)
+    {
+        case TOKEN_EQ:
+            match = col_data == filter_data;
+            break;
+        case TOKEN_NOT_EQ:
+            match = col_data != filter_data;
+            break;
+        case TOKEN_LESS:
+            match = col_data < filter_data;
+            break;
+        case TOKEN_LESS_EQ:
+            match = col_data <= filter_data;
+            break;
+        case TOKEN_GREATER:
+            match = col_data > filter_data;
+            break;
+        case TOKEN_GREATER_EQ:
+            match = col_data >= filter_data;
+            break;
+        default:
+            FIXME("Unsupported condition %d\n", condition);
+    }
+
+    return match;
+}
+
 static HRESULT WINAPI recordset_Find( _Recordset *iface, BSTR criteria, LONG skip_records,
                                       SearchDirectionEnum search_direction, VARIANT start )
 {
-    FIXME( "%p, %s, %ld, %d, %s\n", iface, debugstr_w(criteria), skip_records, search_direction,
+    struct recordset *recordset = impl_from_Recordset( iface );
+    struct column_filter *filters;
+    DataTypeEnum datatype;
+    BOOL found = FALSE;
+    ULONG colcnt;
+
+    TRACE( "%p, %s, %ld, %d, %s\n", recordset, debugstr_w(criteria), skip_records, search_direction,
            debugstr_variant(&start) );
-    return E_NOTIMPL;
+
+    if (recordset->state != adStateOpen) return MAKE_ADO_HRESULT( adErrObjectClosed );
+
+    if (!recordset->criteria || wcscmp(recordset->criteria, criteria) != 0)
+    {
+        clear_recordset_filter(recordset);
+
+        recordset->criteria = SysAllocString(criteria);
+
+        /* Parse criteria */
+        recordset_parse_filter(criteria, recordset);
+    }
+
+    if (list_count(&recordset->filters) != 1)
+    {
+        WARN("Invalid number of filters.\n");
+        clear_recordset_filter(recordset);
+        return MAKE_ADO_HRESULT( adErrDataConversion );
+    }
+
+    colcnt = get_column_count(recordset);
+
+    filters = LIST_ENTRY(list_head(&recordset->filters), struct column_filter, entry);
+    field_get_Type(recordset->fields->field[filters->column], &datatype);
+
+    recordset->index += skip_records;
+    for(; recordset->index < recordset->count; recordset->index++)
+    {
+        found = FALSE;
+
+        switch(datatype)
+        {
+            case adBSTR:
+            case adWChar:
+            {
+                found = column_match_string(filters->condition,
+                                            V_BSTR(&recordset->data[recordset->index * colcnt + filters->column]),
+                                            V_BSTR(&filters->value));
+                break;
+            }
+            case adInteger:
+            {
+                found = column_match_integer(filters->condition,
+                                         V_I4(&recordset->data[recordset->index * colcnt + filters->column]),
+                                         V_I4(&filters->value));
+                break;
+            }
+            default:
+                FIXME("Unsupported datatype %d\n", datatype);
+        }
+
+        if (found)
+        {
+            TRACE("Return record %ld\n", recordset->index);
+            break;
+        }
+    }
+
+    return S_OK;
 }

static HRESULT WINAPI recordset_Cancel( _Recordset *iface )
@@ -2739,6 +2905,8 @@ HRESULT Recordset_create( void **obj )
     VariantInit( &recordset->filter );
     recordset->columntypes = NULL;
     recordset->haccessors = NULL;
+    recordset->criteria = NULL;
+    list_init(&recordset->filters);

*obj = &recordset->Recordset_iface;
     TRACE( "returning iface %p\n", *obj );
diff --git a/dlls/msado15/tests/Makefile.in b/dlls/msado15/tests/Makefile.in
index a97f3dd4339..e7ade35fa38 100644
--- a/dlls/msado15/tests/Makefile.in
+++ b/dlls/msado15/tests/Makefile.in
@@ -1,5 +1,5 @@
 TESTDLL  = msado15.dll
-IMPORTS  = oleaut32 ole32
+IMPORTS  = oleaut32 ole32 odbccp32

C_SRCS = \
    msado15.c
diff --git a/dlls/msado15/tests/msado15.c b/dlls/msado15/tests/msado15.c
index 724787870e0..075f87bdb3b 100644
--- a/dlls/msado15/tests/msado15.c
+++ b/dlls/msado15/tests/msado15.c
@@ -19,8 +19,10 @@
 #include <stdio.h>
 #define COBJMACROS
 #include <initguid.h>
+#include "msdasc.h"
 #include <oledb.h>
 #include <olectl.h>
+#include "odbcinst.h"
 #include <msado15_backcompat.h>
 #include "wine/test.h"
 #include "msdasql.h"
@@ -57,7 +59,7 @@ static void test_Recordset(void)
     VARIANT missing, val, index;
     CursorLocationEnum location;
     CursorTypeEnum cursor;
-    BSTR name;
+    BSTR name, crit;
     HRESULT hr;
     VARIANT bookmark, filter;
     EditModeEnum editmode;
@@ -143,6 +145,12 @@ static void test_Recordset(void)
     hr = _Recordset_put_Filter( recordset, filter );
     ok( hr == S_OK, "got %08lx\n", hr );

+    crit = SysAllocString(L"colu1=1");
+    V_VT( &index ) = VT_EMPTY;
+    hr = _Recordset_Find( recordset, crit, 0, adSearchForward, index );
+    ok( hr == MAKE_ADO_HRESULT( adErrObjectClosed ), "got %08lx\n", hr );
+    SysFreeString(crit);
+
     VariantInit( &missing );
     hr = _Recordset_AddNew( recordset, missing, missing );
     ok( hr == MAKE_ADO_HRESULT( adErrObjectClosed ), "got %08lx\n", hr );
@@ -1335,6 +1343,179 @@ static void test_Command(void)
     _Command_Release( command );
 }

+static BOOL db_created = TRUE;
+static char mdbpath[MAX_PATH];
+
+static void setup_database(void)
+{
+    char *driver;
+    DWORD code;
+    char buffer[1024];
+    WORD size;
+
+    if (winetest_interactive)
+    {
+        trace("assuming odbc 'wine_msado15' is available\n");
+        db_created = TRUE;
+        return;
+    }
+
+    /*
+     * 32 bit Windows has a default driver for "Microsoft Access Driver" (Windows 7+)
+     *  and has the ability to create files on the fly.
+     *
+     * 64 bit Windows ONLY has a driver for "SQL Server", which we cannot use since we don't have a
+     *   server to connect to.
+     *
+     * The filename passed to CREATE_DB must end in mdb.
+     */
+    GetTempPathA(sizeof(mdbpath), mdbpath);
+    strcat(mdbpath, "wine_msado15.mdb");
+
+    driver = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof("DSN=wine_msado15\0CREATE_DB=") + strlen(mdbpath) + 2);
+    memcpy(driver, "DSN=wine_msado15\0CREATE_DB=", sizeof("DSN=wine_msado15\0CREATE_DB="));
+    strcat(driver+sizeof("DSN=wine_msado15\0CREATE_DB=")-1, mdbpath);
+
+    db_created = SQLConfigDataSource(NULL, ODBC_ADD_DSN, "Microsoft Access Driver (*.mdb)", driver);
+    if (!db_created)
+    {
+        SQLInstallerError(1, &code, buffer, sizeof(buffer), &size);
+        trace("code  %ld, buffer %s, size %d\n", code, debugstr_a(buffer), size);
+
+        HeapFree(GetProcessHeap(), 0, driver);
+
+        return;
+    }
+
+    memcpy(driver, "DSN=wine_msado15\0DBQ=", sizeof("DSN=wine_msado15\0DBQ="));
+    strcat(driver+sizeof("DSN=wine_msado15\0DBQ=")-1, mdbpath);
+    db_created = SQLConfigDataSource(NULL, ODBC_ADD_DSN, "Microsoft Access Driver (*.mdb)", driver);
+
+    HeapFree(GetProcessHeap(), 0, driver);
+}
+
+static void cleanup_database(void)
+{
+    BOOL ret;
+
+    if (winetest_interactive)
+        return;
+
+    ret = SQLConfigDataSource(NULL, ODBC_REMOVE_DSN, "Microsoft Access Driver (*.mdb)", "DSN=wine_msado15\0\0");
+    if (!ret)
+    {
+        DWORD code;
+        char buffer[1024];
+        WORD size;
+
+        SQLInstallerError(1, &code, buffer, sizeof(buffer), &size);
+        trace("code  %ld, buffer %s, size %d\n", code, debugstr_a(buffer), size);
+    }
+
+    DeleteFileA(mdbpath);
+}
+
+struct recfilter
+{
+    const WCHAR *filter;
+    HRESULT expected;
+};
+
+static void test_recordset_Find(void)
+{
+    HRESULT hr;
+    _Connection *connection;
+    _Recordset *recordset;
+    VARIANT index;
+    BSTR str;
+    int i;
+    struct recfilter filters[] =
+    {
+        {L"field1 = 1", MAKE_ADO_HRESULT( adErrItemNotFound ) },
+        {L"col1 = 1 OR col3 = 2", MAKE_ADO_HRESULT( adErrInvalidArgument ) },
+        {L"col1 = 1", S_OK },
+        {L"'testing.col1' = 1", MAKE_ADO_HRESULT( adErrInvalidArgument ) },
+        {L"col1 <> 1", S_OK },
+        {L"col1 = 0", S_OK },
+        {L"", MAKE_ADO_HRESULT( adErrInvalidArgument ) },
+    };
+
+    setup_database();
+
+    if (!db_created)
+    {
+        skip("ODBC source wine_msado15 not available.\n");
+        return;
+    }
+
+    str = SysAllocString(L"Provider=MSDASQL.1;Persist Security Info=False;Data Source=wine_msado15");
+
+    hr = CoCreateInstance( &CLSID_Connection, NULL, CLSCTX_INPROC_SERVER, &IID__Connection, (void**)&connection );
+    ok( hr == S_OK, "got %08lx\n", hr );
+
+    hr = _Connection_put_CursorLocation( connection, adUseClient );
+    ok( hr == S_OK, "got %08lx\n", hr );
+
+    hr = _Connection_Open( connection, str, NULL, NULL, adConnectUnspecified );
+    ok( hr == S_OK, "got %08lx\n", hr );
+    SysFreeString( str );
+
+    str = SysAllocString(L"CREATE TABLE testing (col1 INT, col2 VARCHAR(20) NOT NULL, col3 FLOAT)");
+    hr = _Connection_Execute(connection, str, NULL, 0, &recordset);
+    ok( hr == S_OK, "got %08lx\n", hr );
+    _Recordset_Release(recordset);
+    SysFreeString( str );
+
+    str = SysAllocString(L"insert into testing values(1, 'red', 1.0)");
+    hr = _Connection_Execute(connection, str, NULL, 0, &recordset);
+    ok( hr == S_OK, "got %08lx\n", hr );
+    _Recordset_Release(recordset);
+    SysFreeString( str );
+
+    str = SysAllocString(L"insert into testing values(2, 'red', 1.0)");
+    hr = _Connection_Execute(connection, str, NULL, 0, &recordset);
+    ok( hr == S_OK, "got %08lx\n", hr );
+    _Recordset_Release(recordset);
+    SysFreeString( str );
+
+    str = SysAllocString(L"SELECT * from testing");
+    hr = _Connection_Execute(connection, str, NULL, 0, &recordset);
+    ok( hr == S_OK, "got %08lx\n", hr );
+    SysFreeString( str );
+
+    for (i=0; i < ARRAY_SIZE(filters); i++)
+    {
+        BSTR crit = SysAllocString( filters[i].filter );
+
+        hr = _Recordset_MoveFirst( recordset );
+        ok( hr == S_OK, "got %08lx\n", hr );
+
+        V_VT( &index ) = VT_I4;
+        V_I4( &index) = 0;
+
+        hr = _Recordset_Find( recordset, crit, 0, adSearchForward, index );
+        ok( hr == filters[i].expected, "%d got %08lx expected %08lx\n", i, hr, filters[i].expected );
+        SysFreeString( crit );
+    }
+
+    str = SysAllocStringLen( NULL, 0 );
+    hr = _Recordset_Find( recordset, str, 0, adSearchForward, index );
+    ok( hr == MAKE_ADO_HRESULT( adErrInvalidArgument ), "got %08lx\n", hr );
+    SysFreeString( str );
+
+    hr = _Recordset_Find( recordset, NULL, 0, adSearchForward, index );
+    ok( hr == MAKE_ADO_HRESULT( adErrInvalidArgument ), "got %08lx\n", hr );
+
+    hr = _Recordset_Close( recordset );
+    ok( hr == S_OK, "got %08lx\n", hr );
+    _Recordset_Release( recordset );
+
+    hr = _Connection_Close( connection );
+    ok( hr == S_OK, "got %08lx\n", hr );
+
+    cleanup_database();
+}
+
 struct conn_event {
     ConnectionEventsVt conn_event_sink;
     LONG refs;
@@ -1554,6 +1735,7 @@ START_TEST(msado15)
     test_ConnectionPoint();
     test_Fields();
     test_Recordset();
+    test_recordset_Find();
     test_Stream();
     test_Command();
     CoUninitialize();

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/tests/msado15.c:
> -
>
> +struct recfilter
> +{
> - const WCHAR *filter;
> - HRESULT expected;
>
> +};
> -
>
> +static void test_recordset_Find(void)
> +{
> - HRESULT hr;
> - _Connection *connection;
> - _Recordset *recordset;
> - VARIANT index;
> - BSTR str;
> - int i;
> - struct recfilter filters[] =

"static const", probably.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/tests/msado15.c:
> +static BOOL db_created = TRUE;
> +static char mdbpath[MAX_PATH];
> -
>
> +static void setup_database(void)
> +{
> - char *driver;
> - DWORD code;
> - char buffer[1024];
> - WORD size;
> -
> - if (winetest_interactive)
> - {
> -
>    trace("assuming odbc 'wine_msado15' is available\n");
> -
>    db_created = TRUE;
> -
>    return;
> - }

I don't quite understand the point of this part. If we expect the driver to always work on Wine, why guard it with winetest_interactive?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/tests/msado15.c:
>  _Command_Release( command );
>
> }
>
> +static BOOL db_created = TRUE;

Any reason this is a global variable instead of a value returned from setup_database()?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/tests/msado15.c:
> -
> *  and has the ability to create files on the fly.
> -
> *
> -
> * 64 bit Windows ONLY has a driver for "SQL Server", which we cannot use since we don't have a
> -
> *   server to connect to.
> -
> *
> -
> * The filename passed to CREATE_DB must end in mdb.
> -
> */
> - GetTempPathA(sizeof(mdbpath), mdbpath);
> - strcat(mdbpath, "wine_msado15.mdb");
> -
> - driver = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof("DSN=wine_msado15\0CREATE_DB=") + strlen(mdbpath) + 2);
> - memcpy(driver, "DSN=wine_msado15\0CREATE_DB=", sizeof("DSN=wine_msado15\0CREATE_DB="));
> - strcat(driver+sizeof("DSN=wine_msado15\0CREATE_DB=")-1, mdbpath);
> -
> - db_created = SQLConfigDataSource(NULL, ODBC_ADD_DSN, "Microsoft Access Driver (*.mdb)", driver);
> - if (!db_created)

If we expect the above call to always work on Wine, should this be "if (broken(!db_created))"?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/tests/msado15.c:
> -
> - /*
> -
> * 32 bit Windows has a default driver for "Microsoft Access Driver" (Windows 7+)
> -
> *  and has the ability to create files on the fly.
> -
> *
> -
> * 64 bit Windows ONLY has a driver for "SQL Server", which we cannot use since we don't have a
> -
> *   server to connect to.
> -
> *
> -
> * The filename passed to CREATE_DB must end in mdb.
> -
> */
> - GetTempPathA(sizeof(mdbpath), mdbpath);
> - strcat(mdbpath, "wine_msado15.mdb");
> -
> - driver = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof("DSN=wine_msado15\0CREATE_DB=") + strlen(mdbpath) + 2);
> - memcpy(driver, "DSN=wine_msado15\0CREATE_DB=", sizeof("DSN=wine_msado15\0CREATE_DB="));
> - strcat(driver+sizeof("DSN=wine_msado15\0CREATE_DB=")-1, mdbpath);

You're using "strcat" but manually calculating the length; probably this should be strcpy() in that case?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/tests/msado15.c:
> -
>    return;
> - }
> -
> - memcpy(driver, "DSN=wine_msado15\0DBQ=", sizeof("DSN=wine_msado15\0DBQ="));
> - strcat(driver+sizeof("DSN=wine_msado15\0DBQ=")-1, mdbpath);
> - db_created = SQLConfigDataSource(NULL, ODBC_ADD_DSN, "Microsoft Access Driver (*.mdb)", driver);
> -
> - HeapFree(GetProcessHeap(), 0, driver);
>
> +}
> -
>
> +static void cleanup_database(void)
> +{
> - BOOL ret;
> -
> - if (winetest_interactive)
> -
>    return;

What's this for?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/tests/msado15.c:
> +{
> - BOOL ret;
> -
> - if (winetest_interactive)
> -
>    return;
> -
> - ret = SQLConfigDataSource(NULL, ODBC_REMOVE_DSN, "Microsoft Access Driver (*.mdb)", "DSN=wine_msado15\0\0");
> - if (!ret)
> - {
> -
>    DWORD code;
> -
>    char buffer[1024];
> -
>    WORD size;
> -
> -
>    SQLInstallerError(1, &code, buffer, sizeof(buffer), &size);
> -
>    trace("code  %ld, buffer %s, size %d\n", code, debugstr_a(buffer), size);
> - }

Do we ever expect this call to fail?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/tests/msado15.c:
> - ok( hr == S_OK, "got %08lx\n", hr );
> - SysFreeString( str );
> -
> - for (i=0; i < ARRAY_SIZE(filters); i++)
> - {
> -
>    BSTR crit = SysAllocString( filters[i].filter );
> -
> -
>    hr = _Recordset_MoveFirst( recordset );
> -
>    ok( hr == S_OK, "got %08lx\n", hr );
> -
> -
>    V_VT( &index ) = VT_I4;
> -
>    V_I4( &index) = 0;
> -
> -
>    hr = _Recordset_Find( recordset, crit, 0, adSearchForward, index );
> -
>    ok( hr == filters[i].expected, "%d got %08lx expected %08lx\n", i, hr, filters[i].expected );
> -
>    SysFreeString( crit );

I originally wrote "should we check the returned index here too?" and then realized that "index" isn't the returned index, it's the index at which to start searching. Perhaps it could be renamed to something like "start_index" to communicate that more clearly...

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/filter.l:
> +#include <stdio.h>
> -
>
> +#include "msado15_private.h"
> +#include "filter.tab.h"
> -
>
> +#include "wine/debug.h"
> -
>
> +WINE_DEFAULT_DEBUG_CHANNEL(msado15);
> -
>
> +%}
> -
>
> +%%
> -
>
> +[ \r\t]         { /* ignore all whitespace */ } ;
> +[0-9]+.[0-9]+ 	{ yylval->fval = atof(yytext); return T_FLOAT; }
> +[-]?[0-9]+      { yylval->ival = atoi(yytext); return T_INT;   }

That minus sign doesn't need to be in brackets.

Should this check if the integer is out of range?

Should hexadecimal or octal numbers be handled? The latter is particularly important.

[Zebediah Figura (＠zfigura)]

On Mon Mar 27 21:09:17 2023 +0000, Zebediah Figura wrote:
> Is the dot in this rule an error?
> Assuming it is, note that this will match things like:
> 'abc#
> #abc'
> ##abc#
>
> Again, I don't know SQL, so this might be correct, but it seems surprising.
> Also, is there an escape character?

I see that the rule has been separated into two, but the quotes version still looks wrong (it looks like you meant to write the same thing without the dot), and the lack of a similar [^#] in the hash rule also looks wrong or at least surprising.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/filter.l:
> +"<="            { return (yylval->operation = TOKEN_LESS_EQ); }
> +like            { return TOKEN_LIKE; }
> -
>
> +[A-Za-z_0-9]*    { yylval->sval = strdup(yytext); return TOKEN_COLUMN; }
> -
>
> +%%
> -
>
> +int recordset_parse_filter(WCHAR *filter, struct recordset *recordset)
> +{
> - char *str;
> - parser_param p;
> - YY_BUFFER_STATE buffer;
> - int len;
> - int ret;
> -
> - TRACE("(%s)\n", debugstr_w(filter));

Isn't this already traced earlier?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/filter.l:
> +{
> - char *str;
> - parser_param p;
> - YY_BUFFER_STATE buffer;
> - int len;
> - int ret;
> -
> - TRACE("(%s)\n", debugstr_w(filter));
> - memset(&p, 0, sizeof(parser_param));
> - p.recordset = recordset;
> -
> - len = WideCharToMultiByte(CP_ACP, 0, filter, wcslen(filter)+1, 0, 0, NULL, NULL);
> - str = malloc(len);
> - if (!str)
> -
>    return 2; /* Out of Memory */
> - len = WideCharToMultiByte(CP_ACP, 0, filter, wcslen(filter)+1, str, len, NULL, NULL);

You can use -1 instead of the explicit wcslen().

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/filter.l:
> - if (!str)
> -
>    return 2; /* Out of Memory */
> - len = WideCharToMultiByte(CP_ACP, 0, filter, wcslen(filter)+1, str, len, NULL, NULL);
> -
> - yylex_init(&p.yyscanner);
> - yyset_extra(&p, p.yyscanner);
> -
> - buffer = yy_scan_string(str, p.yyscanner);
> - ret = yyparse(&p, p.yyscanner);
> - yy_delete_buffer(buffer, p.yyscanner);
> - yylex_destroy(p.yyscanner);
> -
> - free(str);
> -
> - return ret;
>
> +}

Whitespace error here, and in other files.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/filter.y:
> +filter: %empty
> - | filter line
>
> +;
> -
>
> +line: T_NEWLINE
> - | TOKEN_COLUMN operations T_INT {
> -
> filter_add_integer_column(p->recordset, $1, $2, $3);
> - }
> - | TOKEN_COLUMN operations T_FLOAT {
> -
> filter_add_float_column(p->recordset, $1, $2, $3);
> - }
> - | TOKEN_COLUMN operations T_STRING {
> -
> filter_add_string_column(p->recordset, $1, $2, $3);
> - }
> -
> - /* Like is always followed by a string */

That may be, but currently the way conditions are handled seems very inconsistent. I'd argue it'd be better to move *all* the "like" logic to column_match_string(), and then report an error in e.g. column_match_integer() when LIKE is encountered.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/filter.l:
> -
>                memcpy(yylval->sval, yytext+1, strlen(yytext)-2);
> -
>                yylval->sval[strlen(yytext)-2] = '\0';
> -
>                return T_STRING;
> -
>            }
> -
>
> +\n              { return T_NEWLINE;}
> +"("             { /* Ignored for now */ }
> +")"             { /* Ignored for now */ }
> -
>
> +"="             { return (yylval->operation = TOKEN_EQ); }
> +">"             { return (yylval->operation = TOKEN_GREATER); }
> +"<"             { return (yylval->operation = TOKEN_LESS); }
> +"<>"            { return (yylval->operation = TOKEN_NOT_EQ); }
> +">="            { return (yylval->operation = TOKEN_GREATER_EQ); }
> +"<="            { return (yylval->operation = TOKEN_LESS_EQ); }
> +like            { return TOKEN_LIKE; }

This bothers me, firstly because it's redundant [we're both returning a TOKEN_* vaue and setting its interior value to the same thing] and secondly because it's inconsistent (LIKE is an operation but we don't handle it the same way). I'd argue we should either:

(1) just return token values from the lexer, and then set the operation value accordingly from the parser, or

(2) return a single token value like TOKEN_OPERATION from the lexer, and set its operation enum like now.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/filter.y:
> -
> -
> -
> - This library is free software; you can redistribute it and/or
> -
> - modify it under the terms of the GNU Lesser General Public
> -
> - License as published by the Free Software Foundation; either
> -
> - version 2.1 of the License, or (at your option) any later version.
> -
> -
> -
> - This library is distributed in the hope that it will be useful,
> -
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> -
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -
> - Lesser General Public License for more details.
> -
> -
> -
> - You should have received a copy of the GNU Lesser General Public
> -
> - License along with this library; if not, write to the Free Software
> -
> - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> - */
>
> +%{

Mixing unannotated %{ and annotated %code probably isn't great. The annotated version you want here is probably "%code requires".

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/filter.y:
> -
>
> +#define COBJMACROS
> +#include "objbase.h"
> +#include "msado15_backcompat.h"
> -
>
> +#include "oledb.h"
> +#include "sqlucode.h"
> -
>
> +#include "msado15_private.h"
> -
>
> +#include "wine/debug.h"
> -
>
> +WINE_DEFAULT_DEBUG_CHANNEL(msado15);
> -
>
> +void yyerror(parser_param* param, void const* scanner, char const* msg);
> +parser_param* yyget_extra ( void* yyscanner );

Do you need either of these declarations?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/filter.y:
> - }
> -
> - /* Like is always followed by a string */
> - | TOKEN_COLUMN TOKEN_LIKE T_STRING {
> -
> int i = 0;
> -
> char *stripped = malloc(strlen($3) + 1);
> -
> while(*$3 != '\0')
> -
> {
> -
> 	if(*$3 != '*')
> -
> 	{
> -
> 		stripped[i++] = *$3;
> -
> 	}
> -
> 	$3++;
> -
> }
> -
> TRACE("Stripped: %s, %s\n", debugstr_a($3), debugstr_a(stripped));
> -
> filter_add_string_column(p->recordset, $1, $2, stripped);

This doesn't null-terminate "stripped", does it?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/msado15_private.h:
> - LONG               count;
> - LONG               allocated;
> - LONG               index;
> - VARIANT           *data;
> - CursorLocationEnum cursor_location;
> - CursorTypeEnum     cursor_type;
> - IRowset           *row_set;
> - EditModeEnum      editmode;
> - VARIANT            filter;
> - BSTR              criteria;
> -
> - DBTYPE            *columntypes;
> - HACCESSOR         *haccessors;
> -
> - struct list       filters;
>
> +};

This moves the type and changes it in the same commit, which is unfortunate, since it makes it hard to see what changed.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/recordset.c:
>  return S_OK;
>
> }
>
> +static void clear_recordset_filter(struct recordset *recordset)

"clear" sounds like an odd name for this function; I'd expect something like "destroy", "cleanup", "free" (and "filters" in the plural?)

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/msado15_private.h:
> - BSTR              criteria;
> -
> - DBTYPE            *columntypes;
> - HACCESSOR         *haccessors;
> -
> - struct list       filters;
>
> +};
> -
>
> +typedef struct _parser_param
> +{
> - void* yyscanner;
> -
> - struct recordset *recordset;
>
> +} parser_param;
> -
>
> +#define YY_EXTRA_TYPE parser_param*

This probably works, but the way flex documentation recommends to do it is to set "%option extra-type="parser_param *" in the lexer.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/recordset.c:
> - filters = LIST_ENTRY(list_head(&recordset->filters), struct column_filter, entry);
> - field_get_Type(recordset->fields->field[filters->column], &datatype);
> -
> - recordset->index += skip_records;
> - for(; recordset->index < recordset->count; recordset->index++)
> - {
> -
>    found = FALSE;
> -
> -
>    switch(datatype)
> -
>    {
> -
>        case adBSTR:
> -
>        case adWChar:
> -
>        {
> -
>            found = column_match_string(filters->condition,
> -
>                                        V_BSTR(&recordset->data[recordset->index * colcnt + filters->column]),
> -
>                                        V_BSTR(&filters->value));

This wraps awkwardly, and makes me think perhaps that VARIANT could be a local variable.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/recordset.c:
>  return S_OK;
>
> }
>
> +static void clear_recordset_filter(struct recordset *recordset)
> +{
> - struct column_filter *filters, *filters2;
> -
> - LIST_FOR_EACH_ENTRY_SAFE(filters, filters2, &recordset->filters, struct column_filter, entry)
> - {
> -
>    list_remove(&filters->entry);
> -
>    VariantClear(&filters->value);
> -
>    free(filters);
> - }

Shouldn't this be "filter" in the singular? It's confusing and makes the function look like it's doing the wrong thing otherwise...

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/recordset.c:
> - ULONG colcnt;
> -
> - TRACE( "%p, %s, %ld, %d, %s\n", recordset, debugstr_w(criteria), skip_records, search_direction,
>        debugstr_variant(&start) );
> - return E_NOTIMPL;
> -
> - if (recordset->state != adStateOpen) return MAKE_ADO_HRESULT( adErrObjectClosed );
> -
> - if (!recordset->criteria || wcscmp(recordset->criteria, criteria) != 0)
> - {
> -
>    clear_recordset_filter(recordset);
> -
> -
>    recordset->criteria = SysAllocString(criteria);
> -
> -
>    /* Parse criteria */
> -
>    recordset_parse_filter(criteria, recordset);

Should we handle the return value here?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/filter.y:
> +%token<operation> TOKEN_GREATER_EQ
> +%token<operation> TOKEN_LESS
> +%token<operation> TOKEN_LESS_EQ
> +%token<operation> TOKEN_NOT_EQ
> +%token<operation> TOKEN_EQ
> +%token<operation> TOKEN_LIKE
> -
>
> +%token<sval> TOKEN_COLUMN
> -
> -
>
> +%start filter
> -
>
> +%%
> -
>
> +filter: %empty
> - | filter line

The way this is currently written, this will match things like

"col1 == 1 col2 == 2"

which looks surprising and, given tests, probably wasn't intended?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/msado15/recordset.c:
> - if (!recordset->criteria || wcscmp(recordset->criteria, criteria) != 0)
> - {
> -
>    clear_recordset_filter(recordset);
> -
> -
>    recordset->criteria = SysAllocString(criteria);
> -
> -
>    /* Parse criteria */
> -
>    recordset_parse_filter(criteria, recordset);
> - }
> -
> - if (list_count(&recordset->filters) != 1)
> - {
> -
>    WARN("Invalid number of filters.\n");
> -
>    clear_recordset_filter(recordset);
> -
>    return MAKE_ADO_HRESULT( adErrDataConversion );
> - }

This bothers me. As noted above, currently the code can return multiple filters, but it doesn't look right. I imagine we may want to remove that code (in the initial version), in which case the current code will never return more than one filter, and this should really just be an assert.

More than that, though: the fact that conditions are chained together in arbitrary trees of AND and OR makes me think that a list is not expressive enough to handle this, and hence we should find some other way to structure multiple filters.

[Zebediah Figura (＠zfigura)]

I see some of my earlier comments have been addressed, although others haven't. I've added some new comments anyway.

[Alistair Leslie-Hughes (＠alesliehughes)]

On Mon May 15 18:11:37 2023 +0000, Zebediah Figura wrote:
> I don't quite understand the point of this part. If we expect the driver
> to always work on Wine, why guard it with winetest_interactive?

This guard is so we can setup at connection to a different database type.  Though we are using a "standard" interface,  it can depend on the driver in the backend (ado vs dao) for example.

[Alistair Leslie-Hughes (＠alesliehughes)]

On Mon May 15 18:11:37 2023 +0000, Zebediah Figura wrote:
> Any reason this is a global variable instead of a value returned from setup_database()?

If we want to extend the tests to use real data, we only want to create the database one.  I would expect the next patch _Filter to want to use the same database.

[Alistair Leslie-Hughes (＠alesliehughes)]

On Mon May 15 18:11:37 2023 +0000, Zebediah Figura wrote:
> Do we ever expect this call to fail?

This might fail if we don't have permission.

[Zebediah Figura (＠zfigura)]

On Fri May 19 08:22:27 2023 +0000, Alistair Leslie-Hughes wrote:
> This guard is so we can setup at connection to a different database
> type.  Though we are using a "standard" interface,  it can depend on the
> driver in the backend (ado vs dao) for example.

How would that look like? It currently looks like we're trying to connect to any database we can, and there's no accounting in the tests for differences between drivers (are there any that matter?)

[Zebediah Figura (＠zfigura)]

On Fri May 19 08:22:28 2023 +0000, Alistair Leslie-Hughes wrote:
> If we want to extend the tests to use real data, we only want to create
> the database one.  I would expect the next patch _Filter to want to use
> the same database.

Why only once? Why not create it separately for each test, to avoid the possibility of one test affecting another?

And if we do want to create it only once, this doesn't seem like an idiomatic way to do it. I'd probably still return this value from setup_database(), and then in the main function do something like

if (setup_database())
{
    test_recordset_find();
    test_something_else();
    cleanup_database();
}

[Zebediah Figura (＠zfigura)]

On Fri May 19 08:22:28 2023 +0000, Alistair Leslie-Hughes wrote:
> This might fail if we don't have permission.

Wouldn't it have already failed to create the database in that case?