Copilot commented on code in PR #2329:
URL: https://github.com/apache/age/pull/2329#discussion_r2797241198
##########
drivers/nodejs/src/index.ts:
##########
@@ -36,21 +139,166 @@ function AGTypeParse (input: string) {
return printer.getResult()
}
-async function setAGETypes (client: Client, types: typeof pgTypes) {
- await client.query(`
- CREATE EXTENSION IF NOT EXISTS age;
- LOAD 'age';
- SET search_path = ag_catalog, "$user", public;
- `)
+/**
+ * Options for setAGETypes configuration.
+ */
+interface SetAGETypesOptions {
+ /**
+ * If true, will attempt to CREATE EXTENSION IF NOT EXISTS age.
+ * Defaults to false. Set to true only if the connected user has
+ * sufficient privileges.
+ */
+ createExtension?: boolean
+}
+
+/**
+ * Configures the pg client to properly parse AGE agtype results.
+ *
+ * This function:
+ * 1. Loads the AGE extension into the session
+ * 2. Sets the search_path to include ag_catalog
+ * 3. Registers the agtype type parser
+ *
+ * @param client - A connected pg Client instance
+ * @param types - The pg types module for registering type parsers
+ * @param options - Optional configuration settings
+ * @throws Error if AGE extension is not installed or agtype is not found
+ */
+async function setAGETypes (client: Client, types: typeof pgTypes, options?:
SetAGETypesOptions) {
+ const createExtension = options?.createExtension ?? false
+
+ if (createExtension) {
+ await client.query('CREATE EXTENSION IF NOT EXISTS age;')
+ }
+
+ await client.query("LOAD 'age';")
+ await client.query('SET search_path = ag_catalog, "$user", public;')
Review Comment:
`setAGETypes` calls `LOAD 'age'` directly; when the AGE library/extension
isn’t available, the thrown pg error is typically low-level (e.g., missing
shared library) and bypasses the more descriptive “agtype type not found”
message. Consider wrapping the LOAD/search_path/type-OID block in a try/catch
and rethrowing a clearer error that mentions installing/creating the extension
and/or using `createExtension: true`.
##########
drivers/nodejs/src/index.ts:
##########
@@ -36,21 +139,166 @@ function AGTypeParse (input: string) {
return printer.getResult()
}
-async function setAGETypes (client: Client, types: typeof pgTypes) {
- await client.query(`
- CREATE EXTENSION IF NOT EXISTS age;
- LOAD 'age';
- SET search_path = ag_catalog, "$user", public;
- `)
+/**
+ * Options for setAGETypes configuration.
+ */
+interface SetAGETypesOptions {
+ /**
+ * If true, will attempt to CREATE EXTENSION IF NOT EXISTS age.
+ * Defaults to false. Set to true only if the connected user has
+ * sufficient privileges.
+ */
+ createExtension?: boolean
+}
+
+/**
+ * Configures the pg client to properly parse AGE agtype results.
+ *
+ * This function:
+ * 1. Loads the AGE extension into the session
+ * 2. Sets the search_path to include ag_catalog
+ * 3. Registers the agtype type parser
+ *
+ * @param client - A connected pg Client instance
+ * @param types - The pg types module for registering type parsers
+ * @param options - Optional configuration settings
+ * @throws Error if AGE extension is not installed or agtype is not found
+ */
+async function setAGETypes (client: Client, types: typeof pgTypes, options?:
SetAGETypesOptions) {
+ const createExtension = options?.createExtension ?? false
+
+ if (createExtension) {
+ await client.query('CREATE EXTENSION IF NOT EXISTS age;')
+ }
+
+ await client.query("LOAD 'age';")
+ await client.query('SET search_path = ag_catalog, "$user", public;')
- const oidResults = await client.query(`
- select typelem
- from pg_type
- where typname = '_agtype';`)
+ const oidResults = await client.query(
+ "SELECT typelem FROM pg_type WHERE typname = '_agtype';"
+ )
- if (oidResults.rows.length < 1) { throw new Error() }
+ if (oidResults.rows.length < 1) {
+ throw new Error(
+ 'AGE agtype type not found. Ensure the AGE extension is installed ' +
+ 'and loaded in the current database. Run CREATE EXTENSION age; first.'
+ )
+ }
types.setTypeParser(oidResults.rows[0].typelem, AGTypeParse)
}
-export { setAGETypes, AGTypeParse }
+/**
+ * Column definition for Cypher query results.
+ */
+interface CypherColumn {
+ /** Column alias name */
+ name: string
+ /** Column type (defaults to 'agtype') */
+ type?: string
+}
+
+/**
+ * Executes a Cypher query safely against an AGE graph.
+ *
+ * This function validates the graph name to prevent SQL injection,
+ * properly quotes the Cypher query using dollar-quoting, and builds
+ * the required SQL wrapper.
+ *
+ * @param client - A connected pg Client instance (with AGE types set)
+ * @param graphName - The target graph name (must be a valid AGE graph name)
+ * @param cypher - The Cypher query string
+ * @param columns - Column definitions for the result set
+ * @returns The query result
+ * @throws Error if graphName is invalid or query fails
+ *
+ * @example
+ * ```typescript
+ * const result = await queryCypher(
+ * client,
+ * 'my_graph',
+ * 'MATCH (n:Person) WHERE n.name = $name RETURN n',
+ * [{ name: 'n' }]
+ * );
+ * ```
+ */
+async function queryCypher<T extends QueryResultRow = any> (
+ client: Client,
+ graphName: string,
+ cypher: string,
+ columns: CypherColumn[] = [{ name: 'result' }]
+): Promise<QueryResult<T>> {
+ // Validate graph name against injection
+ validateGraphName(graphName)
+
+ // Validate column definitions
+ if (!columns || columns.length === 0) {
+ throw new Error('At least one column definition is required')
+ }
+
+ for (const col of columns) {
+ if (!col.name || typeof col.name !== 'string') {
+ throw new Error('Column name must be a non-empty string')
+ }
+ // Column names must be valid SQL identifiers
+ if (!VALID_SQL_IDENTIFIER.test(col.name)) {
+ throw new Error(
+ `Invalid column name: '${col.name}'. Column names must be valid SQL
identifiers.`
+ )
+ }
+ if (col.type && !VALID_SQL_IDENTIFIER.test(col.type)) {
+ throw new Error(
+ `Invalid column type: '${col.type}'. Column types must be valid SQL
type identifiers.`
+ )
+ }
+ }
+
+ // Build column list safely
+ const columnList = columns
+ .map(col => `${col.name} ${col.type ?? 'agtype'}`)
+ .join(', ')
+
+ // Safely dollar-quote the cypher query
+ const { quoted } = cypherDollarQuote(cypher)
+
+ const sql = `SELECT * FROM cypher('${graphName}', ${quoted}) AS
(${columnList});`
+
+ return client.query<T>(sql)
+}
+
+/**
+ * Creates a new graph safely.
+ *
+ * @param client - A connected pg Client instance
+ * @param graphName - Name for the new graph (must be a valid AGE graph name)
+ * @throws Error if graphName is invalid
+ */
+async function createGraph (client: Client, graphName: string): Promise<void> {
+ validateGraphName(graphName)
+ await client.query(`SELECT * FROM ag_catalog.create_graph('${graphName}');`)
+}
+
+/**
+ * Drops an existing graph safely.
+ *
+ * @param client - A connected pg Client instance
+ * @param graphName - Name of the graph to drop (must be a valid AGE graph
name)
+ * @param cascade - If true, drop dependent objects (default: false)
+ * @throws Error if graphName is invalid
+ */
+async function dropGraph (client: Client, graphName: string, cascade: boolean
= false): Promise<void> {
+ validateGraphName(graphName)
+ await client.query(`SELECT * FROM ag_catalog.drop_graph('${graphName}',
${cascade});`)
+}
Review Comment:
`dropGraph` interpolates `cascade` directly into the SQL string. Even though
it’s typed as `boolean` in TS, this library is consumable from plain JS and a
crafted non-boolean value could be injected into the query. Prefer using a
parameterized query for `graphName`/`cascade` (or at least validate `typeof
cascade === 'boolean'` before interpolation).
##########
drivers/nodejs/test/index.test.ts:
##########
@@ -17,86 +17,223 @@
* under the License.
*/
-import { types, Client, QueryResultRow } from 'pg'
-import { setAGETypes } from '../src'
+import { types, Client } from 'pg'
+import {
+ setAGETypes,
+ queryCypher,
+ createGraph,
+ dropGraph,
+ validateGraphName,
+ validateLabelName
+} from '../src'
const config = {
- user: 'postgres',
- host: '127.0.0.1',
- database: 'postgres',
- password: 'agens',
- port: 5432
+ user: process.env.PGUSER || 'postgres',
+ password: process.env.PGPASSWORD || 'agens',
+ host: process.env.PGHOST || '127.0.0.1',
+ database: process.env.PGDATABASE || 'postgres',
+ port: parseInt(process.env.PGPORT || '5432', 10)
}
-const testGraphName = 'age-test'
+const testGraphName = 'age_test'
describe('Pre-connected Connection', () => {
let client: Client | null
beforeAll(async () => {
client = new Client(config)
await client.connect()
- await setAGETypes(client, types)
- await client.query(`SELECT create_graph('${testGraphName}');`)
+ await setAGETypes(client, types, { createExtension: false })
Review Comment:
The integration tests now call `setAGETypes(..., { createExtension: false
})`, which makes the test suite depend on the target database already having
`CREATE EXTENSION age;` applied. Previously the tests were self-contained via
the implicit `CREATE EXTENSION IF NOT EXISTS` in `setAGETypes`. Consider using
`{ createExtension: true }` in tests (or explicitly creating the extension in
test setup) to avoid environment-dependent failures.
```suggestion
await setAGETypes(client, types, { createExtension: true })
```
##########
drivers/nodejs/src/index.ts:
##########
@@ -17,14 +17,117 @@
* under the License.
*/
-import { Client } from 'pg'
+import { Client, QueryResult, QueryResultRow } from 'pg'
import pgTypes from 'pg-types'
import { CharStreams, CommonTokenStream } from 'antlr4ts'
import { AgtypeLexer } from './antlr4/AgtypeLexer'
import { AgtypeParser } from './antlr4/AgtypeParser'
import CustomAgTypeListener from './antlr4/CustomAgTypeListener'
import { ParseTreeWalker } from 'antlr4ts/tree'
+/**
+ * Valid graph name pattern aligned with Apache AGE's internal validation
+ * and Neo4j/openCypher naming conventions. Allows letters, digits,
+ * underscores, plus dots and hyphens in middle positions.
+ *
+ * Start: letter or underscore
+ * Middle: letter, digit, underscore, dot, or hyphen
+ * End: letter, digit, or underscore
+ */
+const VALID_GRAPH_NAME = /^[A-Za-z_][A-Za-z0-9_.\-]*[A-Za-z0-9_]$/
+
+/**
+ * Valid label name pattern aligned with Apache AGE's internal validation.
+ * Labels follow stricter identifier rules than graph names — dots and
+ * hyphens are NOT permitted in label names.
+ */
+const VALID_LABEL_NAME = /^[A-Za-z_][A-Za-z0-9_]*$/
+
Review Comment:
The identifier validation patterns are documented as “aligned with Apache
AGE’s internal validation”, but they currently only allow ASCII
letters/digits/underscore (plus dot/hyphen for graphs). AGE’s own name
validation accepts non-ASCII alphabetic characters for graph/label names (see
`regress/sql/name_validation.sql`, e.g. `mydätabase`, `myläbelx`). Either
broaden the regexes to support Unicode letters/numbers (e.g. Unicode property
escapes with the `u` flag) or adjust the docs/error messages to clearly state
the driver intentionally restricts names beyond AGE’s rules.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
