diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 77e6390..7a23509 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/clails-test.asd b/clails-test.asd index 13cb990..0e18e92 100644 --- a/clails-test.asd +++ b/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) diff --git a/src/model/query.lisp b/src/model/query.lisp index f1c776d..782fb72 100644 --- a/src/model/query.lisp +++ b/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 diff --git a/test/data/0013-todo-tag-junction-test/db/migrate/20260204-000000-create-todo-tag-tables.lisp b/test/data/0013-todo-tag-junction-test/db/migrate/20260204-000000-create-todo-tag-tables.lisp new file mode 100644 index 0000000..1556732 --- /dev/null +++ b/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")))) diff --git a/test/model/todo-tag-junction-query.lisp b/test/model/todo-tag-junction-query.lisp new file mode 100644 index 0000000..6f49e9c --- /dev/null +++ b/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 + #: + #: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 () + (:table "todo" + :relations ((:has-many "clails-test/model/todo-tag-junction-query::" + :as :todo-tags + :foreign-key :todo-id)))) + + (defmodel () + (:table "tag" + :relations ((:has-many "clails-test/model/todo-tag-junction-query::" + :as :todo-tags + :foreign-key :tag-id)))) + + (defmodel () + (:table "todo_tags" + :relations ((:belongs-to "clails-test/model/todo-tag-junction-query::" + :column :todo + :key :todo-id) + (:belongs-to "clails-test/model/todo-tag-junction-query::" + :column :tag + :key :tag-id)))) + + (setf clails/environment:*database-type* (make-instance 'clails/environment::)) + (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 + :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 + :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 + :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"))))) +