Fix parameter order bug in WHERE clause with :in operator

.github/workflows/ci.yml

@@ -31,6 +31,7 @@ jobs:
         CLAILS_MIGRATION_DIR_0010: ${{ github.workspace }}/test/data/0010-bulk-test
         CLAILS_MIGRATION_DIR_0011: ${{ github.workspace }}/test/data/0011-bulk-insert-test
         CLAILS_MIGRATION_DIR_0012: ${{ github.workspace }}/test/data/0012-type-conversion-test
+        CLAILS_MIGRATION_DIR_0013: ${{ github.workspace }}/test/data/0013-todo-tag-junction-test
         CLAILS_SQLITE3_DATABASE: ${{ github.workspace }}
         CLAILS_MYSQL_DATABASE: clails_test
         CLAILS_MYSQL_USERNAME: root

clails-test.asd

@@ -69,6 +69,7 @@
                #:clails-test/model/type-conversion/mysql
                #:clails-test/model/type-conversion/postgresql
                #:clails-test/model/type-conversion/sqlite3
+               #:clails-test/model/todo-tag-junction-query
                #:clails-test/task/registry
                #:clails-test/task/runner
                #:clails-test/task/core)

src/model/query.lisp

@@ -848,52 +848,63 @@
 
     (let ((final-sql sql-template)
           (final-params '())
-          (regular-named-params '())
-          (regular-column-types '())
+          (param-specs '())
           (alias->model (slot-value query 'alias->model)))
 
-      ;; Process WHERE clause parameters (including dynamic IN clause expansion)
+      ;; Process WHERE clause parameters - collect all parameter specs in order
       (loop for param in where-params
             for param-index from 0
             do (if (and (listp param) (eq (car param) :in-expansion))
-                   ;; Dynamic IN clause expansion
+                   ;; Dynamic IN clause expansion - store spec for later processing
                    (destructuring-bind (op column-sql keyword) (cdr param)
-                     (let* ((values (getf named-values keyword))
-                            (placeholder (format nil "__IN_CLAUSE_~A_~A__"
-                                                 (cl-ppcre:regex-replace-all "[.:]" column-sql "_")
-                                                 keyword)))
-                       (if (null values)
-                           (let ((replacement (if (string= op "IN") "1=0" "1=1")))
-                             (setf final-sql (cl-ppcre:regex-replace-all 
-                                              (cl-ppcre:quote-meta-chars placeholder) 
-                                              final-sql 
-                                              replacement)))
-                           (let* ((question-marks (format nil "(~{?~*~^, ~})" values))
-                                  (replacement (format nil "~A ~A ~A" column-sql op question-marks)))
-                             (setf final-sql (cl-ppcre:regex-replace-all 
-                                              (cl-ppcre:quote-meta-chars placeholder) 
-                                              final-sql 
-                                              replacement))
-                             (appendf final-params values)))))
-                   ;; Regular parameter
-                   (progn
-                     (push param regular-named-params)
-                     (push (nth param-index column-types) regular-column-types))))
+                     (let ((placeholder (format nil "__IN_CLAUSE_~A_~A__"
+                                                (cl-ppcre:regex-replace-all "[.:]" column-sql "_")
+                                                keyword)))
+                       (push (list :in-expansion 
+                                   :op op 
+                                   :column-sql column-sql 
+                                   :keyword keyword 
+                                   :placeholder placeholder)
+                             param-specs)))
+                   ;; Regular parameter - store spec for later processing
+                   (push (list :regular 
+                               :keyword param 
+                               :column-type (nth param-index column-types))
+                         param-specs)))
 
-      ;; Convert parameter values based on column types
-      (let* ((reversed-params (nreverse regular-named-params))
-             (reversed-types (nreverse regular-column-types))
-             (converted-values
-              (if convert-types
-                  (loop for param-key in reversed-params
-                        for column-type in reversed-types
-                        as value = (getf named-values param-key)
-                        collect (if column-type
-                                    (convert-value-by-type value column-type)
-                                    value))
-                  (loop for param-key in reversed-params
-                        collect (getf named-values param-key)))))
-        (appendf final-params converted-values))
+      ;; Reverse to maintain original order
+      (setf param-specs (nreverse param-specs))
+
+      ;; Process all parameters in order
+      (loop for spec in param-specs
+            do (case (first spec)
+                 (:in-expansion
+                  (let* ((op (getf (rest spec) :op))
+                         (column-sql (getf (rest spec) :column-sql))
+                         (keyword (getf (rest spec) :keyword))
+                         (placeholder (getf (rest spec) :placeholder))
+                         (values (getf named-values keyword)))
+                    (if (null values)
+                        (let ((replacement (if (string= op "IN") "1=0" "1=1")))
+                          (setf final-sql (cl-ppcre:regex-replace-all 
+                                           (cl-ppcre:quote-meta-chars placeholder) 
+                                           final-sql 
+                                           replacement)))
+                        (let* ((question-marks (format nil "(~{?~*~^, ~})" values))
+                               (replacement (format nil "~A ~A ~A" column-sql op question-marks)))
+                          (setf final-sql (cl-ppcre:regex-replace-all 
+                                           (cl-ppcre:quote-meta-chars placeholder) 
+                                           final-sql 
+                                           replacement))
+                          (appendf final-params values)))))
+                 (:regular
+                  (let* ((keyword (getf (rest spec) :keyword))
+                         (column-type (getf (rest spec) :column-type))
+                         (value (getf named-values keyword))
+                         (converted-value (if (and convert-types column-type)
+                                              (convert-value-by-type value column-type)
+                                              value)))
+                    (appendf final-params (list converted-value))))))
 
       ;; Add LIMIT/OFFSET parameters
       (when limit-param

test/data/0013-todo-tag-junction-test/db/migrate/20260204-000000-create-todo-tag-tables.lisp

@@ -0,0 +1,25 @@
+(in-package #:clails-test/model/db/todo-tag-junction-query)
+
+(defmigration "20260204-000000-create-todo-tag-tables"
+ (:up #'(lambda (conn)
+          (create-table conn :table "todo"
+                             :columns '(("title" :type :string
+                                                :not-null t)
+                                       ("owner-id" :type :string
+                                                  :not-null t)))
+          (create-table conn :table "tag"
+                             :columns '(("name" :type :string
+                                                :not-null t)
+                                       ("owner-id" :type :string
+                                                  :not-null t)))
+          (create-table conn :table "todo-tags"
+                             :columns '(("todo-id" :type :integer
+                                                   :not-null t)
+                                       ("tag-id" :type :integer
+                                                 :not-null t)
+                                       ("owner-id" :type :string
+                                                   :not-null t))))
+  :down #'(lambda (conn)
+            (drop-table conn :table "todo-tags")
+            (drop-table conn :table "tag")
+            (drop-table conn :table "todo"))))

test/model/todo-tag-junction-query.lisp

@@ -0,0 +1,144 @@
+(in-package #:cl-user)
+(defpackage #:clails-test/model/todo-tag-junction-query
+  (:use #:cl
+        #:rove
+        #:clails/model/query)
+  (:import-from #:clails/util
+                #:env-or-default)
+  (:import-from #:clails/model/base-model
+                #:<base-model>
+                #:defmodel
+                #:ref
+                #:ref-in))
+
+(defpackage #:clails-test/model/db/todo-tag-junction-query
+  (:use #:cl)
+  (:import-from #:clails/model/migration
+                #:defmigration
+                #:create-table
+                #:drop-table))
+
+(in-package #:clails-test/model/todo-tag-junction-query)
+
+
+(setup
+  (clrhash clails/model/base-model::*table-information*)
+
+  (defmodel <todo> (<base-model>)
+    (:table "todo"
+     :relations ((:has-many "clails-test/model/todo-tag-junction-query::<todo-tag>"
+                   :as :todo-tags
+                   :foreign-key :todo-id))))
+
+  (defmodel <tag> (<base-model>)
+    (:table "tag"
+     :relations ((:has-many "clails-test/model/todo-tag-junction-query::<todo-tag>"
+                   :as :todo-tags
+                   :foreign-key :tag-id))))
+
+  (defmodel <todo-tag> (<base-model>)
+    (:table "todo_tags"
+     :relations ((:belongs-to "clails-test/model/todo-tag-junction-query::<todo>"
+                   :column :todo
+                   :key :todo-id)
+                 (:belongs-to "clails-test/model/todo-tag-junction-query::<tag>"
+                   :column :tag
+                   :key :tag-id))))
+
+  (setf clails/environment:*database-type* (make-instance 'clails/environment::<database-type-mysql>))
+  (setf clails/environment:*project-environment* :test)
+  (setf clails/environment:*database-config* `(:test (:database-name ,(env-or-default "CLAILS_MYSQL_DATABASE" "clails_test")
+                                                      :username ,(env-or-default "CLAILS_MYSQL_USERNAME" "root")
+                                                      :password ,(env-or-default "CLAILS_MYSQL_PASSWORD" "password")
+                                                      :host ,(env-or-default "CLAILS_MYSQL_HOST" "mysql-test")
+                                                      :port ,(env-or-default "CLAILS_MYSQL_PORT" "3306"))))
+  (setf clails/environment:*migration-base-dir* (env-or-default "CLAILS_MIGRATION_DIR_0013" "/app/test/data/0013-todo-tag-junction-test"))
+  (uiop:setup-temporary-directory)
+  (ensure-directories-exist (merge-pathnames "db/" uiop:*temporary-directory*))
+  (setf clails/environment::*project-dir* uiop:*temporary-directory*)
+  (clails/model/migration::db-create)
+  (clails/model/migration::db-migrate)
+  (clails/model/connection::with-db-connection-direct (connection)
+    ;; todo: A
+    (dbi-cp:do-sql connection "insert into todo (created_at, updated_at, title, owner_id) values ('2024-01-01 00:00:00', '2024-01-01 00:00:00', 'Todo A', 'owner1')")
+    ;; todo: B
+    (dbi-cp:do-sql connection "insert into todo (created_at, updated_at, title, owner_id) values ('2024-01-02 00:00:00', '2024-01-02 00:00:00', 'Todo B', 'owner1')")
+    ;; tag: X
+    (dbi-cp:do-sql connection "insert into tag (created_at, updated_at, name, owner_id) values ('2024-01-01 00:00:00', '2024-01-01 00:00:00', 'Tag X', 'owner1')")
+    ;; tag: Y
+    (dbi-cp:do-sql connection "insert into tag (created_at, updated_at, name, owner_id) values ('2024-01-02 00:00:00', '2024-01-02 00:00:00', 'Tag Y', 'owner1')")
+    ;; todo-tags: A(id=1), X(id=1)
+    (dbi-cp:do-sql connection "insert into todo_tags (created_at, updated_at, todo_id, tag_id, owner_id) values ('2024-01-01 00:00:00', '2024-01-01 00:00:00', 1, 1, 'owner1')")
+    ;; todo-tags: B(id=2), Y(id=2)
+    (dbi-cp:do-sql connection "insert into todo_tags (created_at, updated_at, todo_id, tag_id, owner_id) values ('2024-01-02 00:00:00', '2024-01-02 00:00:00', 2, 2, 'owner1')"))
+  (clails/model/connection:startup-connection-pool)
+  (clails/model/base-model:initialize-table-information))
+
+(teardown
+  (uiop:delete-directory-tree uiop:*temporary-directory* :if-does-not-exist :ignore :validate t)
+  (clails/model/connection:shutdown-connection-pool))
+
+(deftest test-simple-parameter-order-bug
+  (testing "Minimal reproduction: single table with :in clause"
+    (let ((simple-query (query <todo>
+                              :as :todos
+                              :where (:and (:= (:todos :owner-id) :owner-id)
+                                           (:= (:todos :title) :title)
+                                           (:in (:todos :id) :ids)))))
+      (multiple-value-bind (sql params)
+          (generate-query simple-query
+                         '(:owner-id "owner1" :title "Todo A" :ids (1 2 3)))
+        (format t "~%=== Simple Query Test ===~%")
+        (format t "Generated SQL: ~A~%" sql)
+        (format t "Parameters: ~A~%" params)
+        (format t "Expected params order: (~S ~S ~A ~A ~A)~%" "owner1" "Todo A" 1 2 3)
+        (format t "Actual params order:   ~A~%" params)
+        (ok (equal params '("owner1" "Todo A" 1 2 3)) 
+            "Parameters should be in WHERE clause order")))))
+
+(deftest test-find-todos-by-tag-ids
+  (let ((find-query (query <todo-tag>
+                          :as :todo-tags
+                          :joins ((:inner-join :todo))
+                          :where (:and (:= (:todo :owner-id) :owner-id)
+                                       (:= (:todo-tags :owner-id) :owner-id)
+                                       (:in (:todo-tags :tag-id) :tag-ids))
+                          :order-by ((:todo :created-at :desc)))))
+    (testing "Find todo A by tag X (tag-id=1)"
+      (let ((results (execute-query find-query
+                                    '(:owner-id "owner1" :tag-ids (1)))))
+        (format t "~%Results: ~S~%" results)
+        (ok (= 1 (length results)))
+        (ok (string= "Todo A" (ref (ref (first results) :todo) :title)))))
+
+    (testing "Verify parameter order bug with JOIN query"
+      (multiple-value-bind (sql params)
+          (generate-query find-query
+                         '(:owner-id "owner1" :tag-ids (1 2)))
+        (format t "~%=== JOIN Query Test ===~%")
+        (format t "Generated SQL: ~A~%" sql)
+        (format t "Parameters: ~A~%" params)
+        (format t "Expected params order: (~S ~S ~A ~A)~%" "owner1" "owner1" 1 2)
+        (format t "Actual params order:   ~A~%" params)
+        (ok (equal params '("owner1" "owner1" 1 2))
+            "Parameters should be in WHERE clause order")))))
+
+(deftest test-multiple-in-clauses
+  (testing "Multiple IN clauses with mixed parameters"
+    (let ((query-with-multiple-ins (query <todo-tag>
+                                         :as :todo-tags
+                                         :where (:and (:= (:todo-tags :owner-id) :owner-id)
+                                                      (:in (:todo-tags :todo-id) :todo-ids)
+                                                      (:= (:todo-tags :tag-id) :tag-id)
+                                                      (:in (:todo-tags :id) :ids)))))
+      (multiple-value-bind (sql params)
+          (generate-query query-with-multiple-ins
+                         '(:owner-id "owner1" :todo-ids (100 200) :tag-id 5 :ids (10 20 30)))
+        (format t "~%=== Multiple IN Clauses Test ===~%")
+        (format t "Generated SQL: ~A~%" sql)
+        (format t "Parameters: ~A~%" params)
+        (format t "Expected params order: (~S ~A ~A ~A ~A ~A ~A)~%" "owner1" 100 200 5 10 20 30)
+        (format t "Actual params order:   ~A~%" params)
+        (ok (equal params '("owner1" 100 200 5 10 20 30))
+            "Parameters should be in WHERE clause order with multiple IN clauses")))))
+