fix(model): keep comma-bearing SQL functions intact in select and group by

changelog.d/4634-comma-sql-functions.fixed.md

@@ -0,0 +1 @@
+- `select=` expressions that contain SQL functions with commas (`CONCAT`, `IF`, `COALESCE`, …) are no longer corrupted when used with `include=` or a distinct group-by query (#4634)

vendor/wheels/model/sql.cfm

@@ -399,7 +399,7 @@
 			local.rv = ReReplaceNoCase(local.rv, "\[\[duplicate\]\]\d+", "", "all");
 
 			local.groupByItems = [];
-			local.selectItems = ListToArray(local.rv);
+			local.selectItems = $splitOutsideFunctions(local.rv, ",");
 
 			for (local.item in local.selectItems) {
 				// Only skip subqueries (items with SELECT inside parentheses)
@@ -689,72 +689,6 @@
 				local.rv = $createSQLFieldListDuplicatePrefix(local.classes, local.rv);
 			}
 
-
-			if (Len(arguments.include) && arguments.clause == "select") {
-				local.newSelect = "";
-				local.addedProperties = "";
-				local.filteredArray = ListToArray(local.rv);
-				local.iEnd = ArrayLen(local.filteredArray);
-				for (local.i = 1; local.i <= local.iEnd; local.i++) {
-					local.iItem = local.filteredArray[local.i];
-
-					// get the property part, done by taking everything from the end of the string to a . or a space (which would be found when using " AS ")
-					local.property = Reverse(SpanExcluding(Reverse(local.iItem), ". "));
-
-					// Strip dialect quotes added above so alias matching and downstream concatenation work on bare identifiers.
-					local.property = variables.wheels.class.adapter.$stripIdentifierQuotes(local.property);
-
-					// check if this one has been flagged as a duplicate, we get the number of classes to skip and also remove the flagged info from the item
-					local.duplicateCount = 0;
-					local.matches = ReFind("^\[\[duplicate\]\](\d+)(.+)$", local.iItem, 1, true);
-					if (local.matches.pos[1] > 0) {
-						local.duplicateCount = Mid(local.iItem, local.matches.pos[2], local.matches.len[2]);
-						local.iItem = Mid(local.iItem, local.matches.pos[3], local.matches.len[3]);
-					}
-
-					if (!local.duplicateCount) {
-						// this is not a duplicate so we can just insert it as is
-						local.newItem = local.iItem;
-						local.newProperty = local.property;
-					} else {
-						// this is a duplicate so we prepend the class name and then insert it unless a property with the resulting name already exist
-						local.classData = local.classes[local.duplicateCount];
-
-						// Initialize aliasFound
-						local.aliasFound = false;
-						local.alias = "";
-
-						// Check for join and extract alias
-						if (StructKeyExists(local.classData, "join")) {
-							local.match = ReFindNoCase("\sAS\s+(\w+)", local.classData.join, 1, true);
-							if (ArrayLen(local.match.len) >= 2 && local.match.len[2] > 0) {
-								local.alias = Mid(local.classData.join, local.match.pos[2], local.match.len[2]);
-								local.aliasFound = CompareNoCase(local.alias, local.classData.pluralizedName) EQ 0;
-							}
-						}
-
-						// Construct newProperty using alias or modelName
-						local.newProperty = (local.aliasFound ? local.alias : local.classData.modelName) & local.property;
-
-						// Determine newItem based on presence of " AS " in iItem
-						if (Find(" AS ", local.iItem)) {
-							local.newItem = ReplaceNoCase(local.iItem, " AS " & local.property, " AS " & local.newProperty);
-						} else {
-							if (local.aliasFound) {
-								local.newItem = local.alias & "." & variables.wheels.class.adapter.$quoteIdentifier(local.property) & " AS " & local.newProperty;
-							} else {
-								local.newItem = local.iItem & " AS " & local.newProperty;
-							}
-						}
-					}
-					if (!ListFindNoCase(local.addedProperties, local.newProperty)) {
-						local.newSelect = ListAppend(local.newSelect, local.newItem);
-						local.addedProperties = ListAppend(local.addedProperties, local.newProperty);
-					}
-				}
-				local.rv = local.newSelect;
-			}
-
 			if (arguments.clause == "groupBy" && Find(" AS ", local.rv)) {
 				local.rv = ReReplace(local.rv, variables.wheels.class.RESQLAs, "", "all");
 			}
@@ -886,7 +820,7 @@
 	public string function $createSQLFieldListDuplicatePrefix(required array classes, required string rv) {
 		local.newSelect = "";
 		local.addedProperties = "";
-		local.filteredArray = ListToArray(arguments.rv);
+		local.filteredArray = $splitOutsideFunctions(arguments.rv, ",");
 		local.iEnd = ArrayLen(local.filteredArray);
 		for (local.i = 1; local.i <= local.iEnd; local.i++) {
 			local.iItem = local.filteredArray[local.i];

vendor/wheels/tests/specs/model/CommaFunctionSelectSpec.cfc

@@ -0,0 +1,56 @@
+/**
+ * Regression for #4634: a select= expression containing a SQL function with commas
+ * (CONCAT/IF/COALESCE/...) must survive intact when include= enables the duplicate
+ * prefix pass, and the distinct group-by pass must not split it into fragments.
+ *
+ * The expression's alias deliberately repeats the last column's name: the old
+ * ListToArray-based split treated every fragment as a select item and dropped the
+ * tail fragment as a "duplicate", corrupting the SQL.
+ */
+component extends="wheels.WheelsTest" {
+
+	function run() {
+
+		describe("comma-bearing SQL functions in select (##4634)", () => {
+
+			beforeEach(() => {
+				// Alias == last column name, as in the issue's report.
+				variables.expr = "CONCAT(IF(ISNULL(authors.firstname)=1, '', CONCAT(authors.firstname, ' ')), authors.firstname) AS firstname"
+				variables.model = application.wo.model("post")
+			})
+
+			it("keeps the function expression intact through the include duplicate-prefix pass", () => {
+				var sql = variables.model.$selectClause(select = "#variables.expr#", include = "author", returnAs = "query")
+				expect(sql).toInclude(variables.expr)
+			})
+
+			it("does not drop the expression tail when a plain item precedes it", () => {
+				var sql = variables.model.$selectClause(select = "id, #variables.expr#", include = "author", returnAs = "query")
+				expect(sql).toInclude(variables.expr)
+				// The old code truncated after the first fragment that collided by property name,
+				// so the expression (the last select item) has to still be the end of the clause.
+				expect(Right(sql, Len(variables.expr))).toBe(variables.expr)
+			})
+
+			it("keeps several comma-bearing functions intact side by side", () => {
+				var functions = "COALESCE(authors.firstname, 'x', 'y') AS fn, COALESCE(authors.lastname, 'x', 'y') AS ln"
+				var sql = variables.model.$selectClause(select = "id, #functions#", include = "author", returnAs = "query")
+				expect(sql).toInclude("COALESCE(authors.firstname, 'x', 'y') AS fn")
+				expect(sql).toInclude("COALESCE(authors.lastname, 'x', 'y') AS ln")
+			})
+
+			it("does not leak split subquery fragments into a distinct group by", () => {
+				// Subquery select items are deliberately skipped in the GROUP BY clause; the old
+				// ListToArray split kept the fragments that did not themselves contain SELECT.
+				var expr = "(SELECT CONCAT(authors.firstname, ' ') FROM c_o_r_e_authors) AS subname"
+				var groupSql = variables.model.$groupByClause(select = "id, #expr#", include = "author", group = "", distinct = true, returnAs = "query")
+				expect(groupSql).toInclude("id")
+				expect(groupSql).notToInclude("c_o_r_e_authors")
+				expect(groupSql).notToInclude("subname")
+			})
+
+		})
+
+	}
+
+}