feat: Add PostgreSQL support#8
feat: Add PostgreSQL support#8cB-Guru-Sharan-Kumar-Ram wants to merge 8 commits intoCodebucket-Solutions:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds dialect-aware identifier escaping and a public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
index.js (3)
234-234:⚠️ Potential issue | 🟠 MajorRemove debug
console.logstatement.
console.log(replacements)will leak query parameters (potentially sensitive user data) to stdout in production.Proposed fix
- console.log(replacements); return {
313-316:⚠️ Potential issue | 🟡 MinorPostgreSQL requires quoted aliases for case-sensitive or reserved-word aliases in subqueries.
The subquery alias
totalCountson line 316 andcountValueon line 203 are used unquoted. While these specific names happen to be safe, they are camelCase and PostgreSQL will fold them to lowercase. If any downstream code or the caller expectscountValue(camelCase), it will receivecountvalueinstead. Consider quoting withescapeIdentifieror using snake_case aliases for PostgreSQL compatibility.
345-349:⚠️ Potential issue | 🟠 MajorPostgreSQL queries with LIMIT will fail using the standard
pgdriver due to incompatible placeholder syntax.The
pgdriver requires$1, $2, ...style placeholders, not?. While the codebase has dialect detection for PostgreSQL (as shown byescapeIdentifier()and operator validation), the LIMIT clause at lines 345-349 hardcodes?placeholders without checking the dialect. This means whendialect === "postgresql"is set, pagination queries will fail.The fix requires conditional placeholder syntax based on the dialect (e.g.,
LIMIT $1,$2for PostgreSQL vs.LIMIT ?,?for MySQL). Without this, PostgreSQL support is incomplete and non-functional for pagination.
🤖 Fix all issues with AI agents
In `@index.js`:
- Around line 330-335: The orderBy assembly is applying escapeIdentifier before
columnNameConverter; change it to call columnNameConverter first and then
escapeIdentifier so the converter receives the raw column name. Locate the code
that builds the orderBy string (the expression using orderByQuery,
this.columnNameConverter(this.escapeIdentifier(...)), sort.attributes[i], and
sort.sorts[i]) and swap the calls to use
this.escapeIdentifier(this.columnNameConverter(sort.attributes[i])) (i.e., call
columnNameConverter on the raw attribute, then pass that result into
escapeIdentifier) so the escaping follows the converter consistently with the
rest of the codebase.
- Around line 23-28: escapeIdentifier currently wraps raw input without escaping
delimiter characters, enabling SQL injection; update escapeIdentifier to
sanitize the identifier by (1) handling dot-qualified names by splitting on '.',
(2) for each segment replace any occurrences of the delimiter with the delimiter
doubled (for PostgreSQL double-quote -> "" , for MySQL backtick -> ``), (3) then
rejoin and wrap each escaped segment with the proper quoting character based on
this.dialect; also ensure it safely handles null/undefined and non-string inputs
(coerce to string) before escaping.
🧹 Nitpick comments (5)
index.js (5)
8-8:SqlStringis imported but no longer used.All usages of
SqlString.escapeIdhave been replaced bythis.escapeIdentifier. This is now a dead dependency import.Proposed fix
-let SqlString = require('sqlstring');
158-175: Dead code:if (column)on line 161 is always true, andreturn []on line 175 is unreachable.Line 158-160 throws if
!column, so execution only reaches line 161 whencolumnis truthy. The guard on line 161 is redundant, and thereturn []on line 175 can never be reached.Proposed cleanup
if (!column) { throw `Invalid filter field: ${field}`; } - if (column) { - let fieldName; - - if (column.statement) { - fieldName = column.statement; - } else if (column.prefix) { - fieldName = `${this.escapeIdentifier(column.prefix)}.${this.escapeIdentifier(column.name)}`; - } else { - fieldName = this.escapeIdentifier(column.name); - } - - return [[fieldName, operator, value]]; + let fieldName; + + if (column.statement) { + fieldName = column.statement; + } else if (column.prefix) { + fieldName = `${this.escapeIdentifier(column.prefix)}.${this.escapeIdentifier(column.name)}`; + } else { + fieldName = this.escapeIdentifier(column.name); } - - return []; // Return an empty array if no column matches + + return [[fieldName, operator, value]];
33-37: Alias values are not escaped/quoted inASclauses.Column aliases (
a.alias) are interpolated directly into the SQL string without any quoting or escaping. If an alias contains special characters or is a reserved word, the query will break or become vulnerable. Consider applyingescapeIdentifierto aliases as well, for consistency with the rest of the identifier handling.Also applies to: 43-44
54-56: Throwing string literals instead ofErrorobjects.The dialect-specific checks throw raw strings (e.g.,
throw "FIND_IN_SET is not supported..."). This loses the stack trace and makes error handling harder for callers (e.g.,catch (e)won't have.stack). The same pattern exists elsewhere in the file (line 50, 159), but since these are new lines, consider usingnew Error(...).Also applies to: 66-68
10-17: No validation of thedialectoption.The constructor silently accepts any string as
dialect(e.g.,"postgres","pg","mssql"). Invalid values would silently fall through to MySQL quoting inescapeIdentifier. Consider validating against a known set and throwing on unrecognized dialects.Proposed validation
constructor(options) { + const SUPPORTED_DIALECTS = ["mysql", "postgresql"]; if(options) { let { columnNameConverter, dialect } = options; if (columnNameConverter) this.columnNameConverter = columnNameConverter; - this.dialect = dialect || "mysql"; + this.dialect = dialect || "mysql"; + if (!SUPPORTED_DIALECTS.includes(this.dialect)) { + throw new Error(`Unsupported dialect: "${this.dialect}". Supported: ${SUPPORTED_DIALECTS.join(", ")}`); + } } else { this.dialect = "mysql"; } }Also applies to: 20-21
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
index.js (3)
365-374:⚠️ Potential issue | 🔴 CriticalCritical:
LIMIT ?,?is MySQL-specific and will break on PostgreSQL.Two issues here:
- Syntax: MySQL interprets
LIMIT offset, countbut PostgreSQL requiresLIMIT <count> OFFSET <offset>— different keyword and reversed parameter order.- Placeholders: PostgreSQL's
pgdriver uses$1, $2, …positional placeholders, not?. If the consumer usespgdirectly, every?in the generated queries will fail.Since this PR's purpose is to add PostgreSQL support, the query builder needs dialect-aware LIMIT/OFFSET and placeholder generation.
🐛 Proposed fix for LIMIT/OFFSET (placeholder translation is a broader concern)
if (paginationObject.pageNo && paginationObject.itemsPerPage) { let offset = (paginationObject.pageNo - 1) * paginationObject.itemsPerPage; - query = query + " LIMIT ?,?"; - replacements.push(offset, paginationObject.itemsPerPage); + if (this.dialect === "postgresql") { + query = query + " LIMIT ? OFFSET ?"; + replacements.push(paginationObject.itemsPerPage, offset); + } else { + query = query + " LIMIT ?,?"; + replacements.push(offset, paginationObject.itemsPerPage); + } } else if(paginationObject.offset && paginationObject.limit ) { - query = query + " LIMIT ?,?"; - replacements.push(paginationObject.offset,paginationObject.limit) + if (this.dialect === "postgresql") { + query = query + " LIMIT ? OFFSET ?"; + replacements.push(paginationObject.limit, paginationObject.offset); + } else { + query = query + " LIMIT ?,?"; + replacements.push(paginationObject.offset, paginationObject.limit); + } }Note: The
?placeholder issue affects all generated queries, not just LIMIT. Consider either documenting that consumers must use a?-compatible driver (e.g.,mysql2,knex), or adding dialect-aware placeholder indexing.
161-173:⚠️ Potential issue | 🟠 MajorBug:
columnNameConverteris not applied to filter field names.In
columNames()(lines 37, 44), column names are passed throughthis.columnNameConverterbefore escaping. Here in the filter path,column.nameis escaped directly without conversion. If acolumnNameConverteris configured (e.g., camelCase → snake_case), the WHERE clause will reference unconverted column names that don't match the database schema.🐛 Proposed fix
if (column.statement) { fieldName = column.statement; } else if (column.prefix) { - fieldName = `${this.escapeIdentifier(column.prefix)}.${this.escapeIdentifier(column.name)}`; + fieldName = `${this.escapeIdentifier(column.prefix)}.${this.escapeIdentifier(this.columnNameConverter(column.name))}`; } else { - fieldName = this.escapeIdentifier(column.name); + fieldName = this.escapeIdentifier(this.columnNameConverter(column.name)); }
234-234:⚠️ Potential issue | 🟡 MinorRemove debug
console.log.This leaks query replacement values (potentially sensitive data) to stdout in production.
🧹 Nitpick comments (3)
index.js (3)
328-347:toCamelCaseis duplicated and redefined inside the loop.The same
toCamelCasehelper is defined in bothsingleTablePagination(line 130) and here inside theforloop body (line 332), meaning it's recreated on every iteration. Extract it once as a class method or module-level utility.
54-68: ThrowErrorobjects instead of string literals.
throw "FIND_IN_SET is not supported..."produces a plain string, which lacks a stack trace and won't be caught by error-handling middleware that checksinstanceof Error. This pattern is used throughout the file (lines 50, 55, 67, 159, 325, 350). Consider usingthrow new Error(...)consistently.
277-281: Hardcoded"id"fallback sort assumes every table has anidcolumn.Line 279 unconditionally appends
"id"to sort attributes, and line 349 special-cases it to bypass validation. PostgreSQL schemas commonly use different primary key names (e.g.,<table>_id, UUIDs). Consider making the default tiebreaker sort column configurable per option/table.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
index.js (5)
161-175:⚠️ Potential issue | 🔴 CriticalBug:
columnNameConverternot applied to filter field names.In the SELECT clause (
columNames), column names go throughthis.columnNameConverter(a.name)before escaping. But here inprocessCondition, the filter field is built fromcolumn.namewithout applyingcolumnNameConverter. When a converter is configured, the WHERE clause will reference different column names than the SELECT clause.🐛 Proposed fix
if (column.statement) { fieldName = column.statement; } else if (column.prefix) { - fieldName = `${this.escapeIdentifier(column.prefix)}.${this.escapeIdentifier(column.name)}`; + fieldName = `${this.escapeIdentifier(column.prefix)}.${this.escapeIdentifier(this.columnNameConverter(column.name))}`; } else { - fieldName = this.escapeIdentifier(column.name); + fieldName = this.escapeIdentifier(this.columnNameConverter(column.name)); }Also: the
if (column)on line 161 is always true since line 158–159 throws when!column, making line 175 dead code.
234-234:⚠️ Potential issue | 🟠 MajorRemove debug
console.log.This logs query replacement values (which may include user-supplied filter data or PII) to stdout. This should be removed before merging.
365-374:⚠️ Potential issue | 🔴 CriticalCritical:
LIMIT ?,?syntax is MySQL-only — PostgreSQL pagination is broken.PostgreSQL does not support
LIMIT offset, count. It requiresLIMIT count OFFSET offset(note the reversed parameter order). Since this PR's purpose is to add PostgreSQL support, this is a showstopper.🐛 Proposed fix
if (paginationObject.pageNo && paginationObject.itemsPerPage) { let offset = (paginationObject.pageNo - 1) * paginationObject.itemsPerPage; - query = query + " LIMIT ?,?"; - replacements.push(offset, paginationObject.itemsPerPage); + if (this.dialect === "postgresql") { + query = query + " LIMIT ? OFFSET ?"; + replacements.push(paginationObject.itemsPerPage, offset); + } else { + query = query + " LIMIT ?,?"; + replacements.push(offset, paginationObject.itemsPerPage); + } } else if(paginationObject.offset && paginationObject.limit ) { - query = query + " LIMIT ?,?"; - replacements.push(paginationObject.offset,paginationObject.limit) + if (this.dialect === "postgresql") { + query = query + " LIMIT ? OFFSET ?"; + replacements.push(paginationObject.limit, paginationObject.offset); + } else { + query = query + " LIMIT ?,?"; + replacements.push(paginationObject.offset, paginationObject.limit); + } }
6-6:⚠️ Potential issue | 🟠 MajorMySQL-specific operators
RLIKEandMEMBER OFlack PostgreSQL guards.
JSON_CONTAINS,JSON_OVERLAPS, andFIND_IN_SETcorrectly throw for PostgreSQL, butRLIKEandMEMBER OFare also MySQL-specific and will produce invalid SQL on PostgreSQL. They need similar dialect guards intupleCreator.Also applies to: 47-90
365-381:⚠️ Potential issue | 🟠 MajorPostgreSQL queries require
$1, $2, ...placeholders, not?.The standard Node.js PostgreSQL driver (
pg) uses numbered positional parameters like$1,$2, etc. The generated queries use?placeholders (MySQL syntax), which will not work with thepgdriver. If PostgreSQL support is a goal, either use a dialect-aware placeholder strategy or document this as a MySQL-only limitation.
🤖 Fix all issues with AI agents
In `@index.js`:
- Around line 202-206: The SQL alias for the COUNT column in totalCountQuery is
using mixed-case (countValue) which PostgreSQL will fold to lowercase; update
the query construction in totalCountQuery to quote the alias (e.g., AS
"countValue") so the returned column name preserves camelCase, and verify the
other COUNT alias usage (totalCounts) is consistent with consumer expectations
(either quote it or use a lowercase alias) to avoid undefined values when
reading the result.
- Around line 328-359: The validation currently uses toCamelCase to match
camelCase sort attributes against option.columnList entries but then builds
orderByQuery with the raw sortAttr; modify the loop that iterates
sort.attributes to store the matched column identifier (use col.name, col.alias,
or `${col.prefix}.${col.name}` as appropriate) into a variable like
resolvedSortAttr when isValidColumn becomes true, and after validation use this
resolvedSortAttr (passed through this.columnNameConverter and
this.escapeIdentifier) instead of the original sortAttr when appending to
orderByQuery; keep the existing check for "id" but ensure resolvedSortAttr falls
back to sortAttr only when no match was found.
- Around line 10-18: Validate the provided dialect in the constructor: check the
dialect option (used to set this.dialect) against an allowed set (e.g., "mysql",
"mariadb", "sqlite", "postgres"/"pg" etc. as your library supports) and if it is
not in that set throw a clear error instead of silently falling back to "mysql";
locate the constructor function and the references to options.dialect and
this.dialect and add a small validation branch that normalizes common aliases
(e.g., "pg" -> "postgres") or rejects unknown values with a descriptive
exception so users cannot unknowingly get MySQL quoting.
🧹 Nitpick comments (3)
index.js (3)
30-45: Aliases are not escaped withescapeIdentifier.Column names are properly escaped via
escapeIdentifier, but aliases (e.g., line 35, 40, 43) are interpolated raw into the query. While aliases are typically developer-controlled, applyingescapeIdentifierto aliases would be consistent and defensive.- this.escapeIdentifier(a.prefix) + "." + this.escapeIdentifier(this.columnNameConverter(a.name)) + " AS " + a.alias + this.escapeIdentifier(a.prefix) + "." + this.escapeIdentifier(this.columnNameConverter(a.name)) + " AS " + this.escapeIdentifier(a.alias)(Same pattern applies on lines 40, 43.)
54-68: Throwing string literals instead ofErrorobjects.Lines 55, 67 (and others like 50, 159, 325, 350) throw raw strings. This loses stack traces and breaks
instanceof Errorchecks. Consider throwing properErrorinstances.- throw `${operator} is not supported in PostgreSQL.`; + throw new Error(`${operator} is not supported in PostgreSQL.`);
130-136: DuplicatedtoCamelCasehelper.The same
toCamelCasefunction is defined inline in bothsingleTablePagination(line 130) andpaginate(line 332). Extract it as a class method or module-level utility.Also applies to: 332-336
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
index.js (4)
234-234:⚠️ Potential issue | 🟡 MinorRemove debug
console.log.This leaks query replacements (which may contain sensitive user data) to stdout in production.
Proposed fix
- console.log(replacements);
161-170:⚠️ Potential issue | 🟠 MajorBug:
columnNameConverteris not applied to filter field names.In
columNames(line 37), column names pass throughthis.columnNameConverter(a.name)before escaping. But here in filter resolution,column.nameis escaped directly without the converter. This inconsistency means filters reference the original DB column name while SELECT uses the converted name.🐛 Proposed fix
if (column.statement) { fieldName = column.statement; } else if (column.prefix) { - fieldName = `${this.escapeIdentifier(column.prefix)}.${this.escapeIdentifier(column.name)}`; + fieldName = `${this.escapeIdentifier(column.prefix)}.${this.escapeIdentifier(this.columnNameConverter(column.name))}`; } else { - fieldName = this.escapeIdentifier(column.name); + fieldName = this.escapeIdentifier(this.columnNameConverter(column.name)); }
368-377:⚠️ Potential issue | 🔴 CriticalCritical:
LIMIT ?,?is MySQL-only syntax — breaks PostgreSQL.MySQL's two-argument
LIMIT offset, countform is not valid PostgreSQL. PostgreSQL requiresLIMIT count OFFSET offset(note the reversed argument order too). This will cause a query syntax error for all PostgreSQL users attempting pagination.🐛 Proposed fix
if (paginationObject.pageNo && paginationObject.itemsPerPage) { let offset = (paginationObject.pageNo - 1) * paginationObject.itemsPerPage; - query = query + " LIMIT ?,?"; - replacements.push(offset, paginationObject.itemsPerPage); + if (this.dialect === "postgresql") { + query = query + " LIMIT ? OFFSET ?"; + replacements.push(paginationObject.itemsPerPage, offset); + } else { + query = query + " LIMIT ?,?"; + replacements.push(offset, paginationObject.itemsPerPage); + } } else if(paginationObject.offset && paginationObject.limit ) { - query = query + " LIMIT ?,?"; - replacements.push(paginationObject.offset,paginationObject.limit) + if (this.dialect === "postgresql") { + query = query + " LIMIT ? OFFSET ?"; + replacements.push(paginationObject.limit, paginationObject.offset); + } else { + query = query + " LIMIT ?,?"; + replacements.push(paginationObject.offset, paginationObject.limit); + } }
6-6:⚠️ Potential issue | 🟠 Major
allowedOperatorslist includes MySQL-only operators without complete dialect guards.
MEMBER OFandRLIKEare MySQL-specific operators that are included in the globalallowedOperatorslist but lack dialect guards intupleCreator. UnlikeJSON_CONTAINS,JSON_OVERLAPS, andFIND_IN_SET—which explicitly throw errors for PostgreSQL—MEMBER OFandRLIKEwill silently produce invalid SQL when used with PostgreSQL. Add dialect checks similar to the existing guards for these operators.
🤖 Fix all issues with AI agents
In `@index.js`:
- Around line 346-349: The current assignment resolvedAttr = matchedCol.alias ||
matchedCol.name drops the prefix for columns discovered via the prefix.name
path; update the assignment inside the if (matchedCol) branch so that when a
prefix object is present you preserve it (e.g., use
`${prefix.name}.${matchedCol.alias || matchedCol.name}`) while still preferring
alias when available; modify the resolvedAttr assignment in the block that
handles matchedCol (the code using prefix.name and matchedCol) to build a
prefixed identifier rather than a bare column name.
- Around line 30-45: The columNames arrow function is inserting aliases
unescaped into SQL; update every branch that uses a.alias (including the prefix
branch, the statement branch, and the no-prefix branch) to pass the alias
through this.escapeIdentifier before interpolating (e.g., replace "AS " +
a.alias with "AS " + this.escapeIdentifier(a.alias)); keep existing uses of
this.columnNameConverter and escapeIdentifier for names and prefixes unchanged.
🧹 Nitpick comments (1)
index.js (1)
328-336:toCamelCaseis duplicated — extract to a shared helper.This same function is defined at line 130 inside
singleTablePaginationand again at line 332 insidepaginate. Extract it once as a module-level utility or a class method.
470a433 to
ecc9c5d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
index.js (4)
234-234:⚠️ Potential issue | 🟠 MajorRemove debug
console.log— leaks query parameter values.
replacementscan contain user-supplied data (search terms, filter values). Logging them in production is a compliance/privacy risk and exposes potentially sensitive information.- console.log(replacements);
161-170:⚠️ Potential issue | 🟠 Major
columnNameConverteris not applied to filter field names, inconsistent withcolumNames.In
columNames(line 37), column names are passed throughthis.columnNameConverter(a.name)before escaping. Here in filter resolution (lines 167, 169),column.nameis escaped directly without the converter. IfcolumnNameConverterperforms any transformation (e.g., camelCase ↔ snake_case), the WHERE clause column references will mismatch the SELECT clause.Proposed fix
} else if (column.prefix) { - fieldName = `${this.escapeIdentifier(column.prefix)}.${this.escapeIdentifier(column.name)}`; + fieldName = `${this.escapeIdentifier(column.prefix)}.${this.escapeIdentifier(this.columnNameConverter(column.name))}`; } else { - fieldName = this.escapeIdentifier(column.name); + fieldName = this.escapeIdentifier(this.columnNameConverter(column.name)); }
373-382:⚠️ Potential issue | 🔴 CriticalCritical:
LIMIT ?,?syntax and parameter order are MySQL-specific — broken on PostgreSQL.MySQL uses
LIMIT offset, countwhile PostgreSQL usesLIMIT count OFFSET offset(different keyword and reversed parameter order). Since this PR adds PostgreSQL support, these queries will produce incorrect pagination results on PostgreSQL.Proposed fix
if (paginationObject.pageNo && paginationObject.itemsPerPage) { let offset = (paginationObject.pageNo - 1) * paginationObject.itemsPerPage; - query = query + " LIMIT ?,?"; - replacements.push(offset, paginationObject.itemsPerPage); + if (this.dialect === "postgresql") { + query = query + " LIMIT ? OFFSET ?"; + replacements.push(paginationObject.itemsPerPage, offset); + } else { + query = query + " LIMIT ?,?"; + replacements.push(offset, paginationObject.itemsPerPage); + } } else if(paginationObject.offset && paginationObject.limit ) { - query = query + " LIMIT ?,?"; - replacements.push(paginationObject.offset,paginationObject.limit) + if (this.dialect === "postgresql") { + query = query + " LIMIT ? OFFSET ?"; + replacements.push(paginationObject.limit, paginationObject.offset); + } else { + query = query + " LIMIT ?,?"; + replacements.push(paginationObject.offset, paginationObject.limit); + } }
6-8:⚠️ Potential issue | 🟠 MajorAdd guards for
RLIKEandMEMBER OFoperators; remove unusedSqlStringimport.
RLIKEandMEMBER OFare MySQL-specific operators but lack the PostgreSQL check present forJSON_CONTAINS,JSON_OVERLAPS, andFIND_IN_SET. Add similar guards intupleCreatorto prevent runtime errors on PostgreSQL.The
sqlstringimport on line 8 is unused and should be removed.
🧹 Nitpick comments (1)
index.js (1)
332-336:toCamelCaseis duplicated and needlessly recreated per loop iteration.This helper is identical to the one at line 130. Extract it as a module-level function or a class method to avoid duplication and per-iteration allocation.
+// At module level (near the top of the file) +const toCamelCase = (str) => { + return str.replace(/_([a-zA-Z0-9])/g, (_, char) => { + return /[a-zA-Z]/.test(char) ? char.toUpperCase() : char; + }); +};
Summary by CodeRabbit
New Features
Bug Fixes / Validation