Error when attempting to use PostgreSQL JSON operators

[danielbobbert]

When I try to use a PostreSQL JSON operator containing a question mark, org.hibernate.reactive.pool.impl.PostgresParameters will turn the question mark into a $n parameter filler, which breaks the query. Escaping the question mark doesn't help either, since the escaped question mark is then passed to PostgreSQL verbatim, which is also invalid.

So I either get
error executing SQL statement [ERROR: syntax error at or near "\" (42601)] [select e1_0.id from test.Entity e1_0 where ((jsonb_extract_path(e1_0.child_entity,'flags') $1| array[$2])=true] caused by java.sql.SQLException: ERROR: syntax error at or near "$"
because the question mark is replaced, or
error executing SQL statement [ERROR: syntax error at or near "\" (42601)] [select e1_0.id from test.Entity e1_0 where ((jsonb_extract_path(e1_0.child_entity,'flags') \?| array[$1])=true] caused by java.sql.SQLException: ERROR: syntax error at or near "\"
because the escaped question mark is left unchanged (but including the escape sequence), while the correct final SQL would look like this:
select e1_0.id from test.Entity e1_0 where ((jsonb_extract_path(e1_0.child_entity,'flags') ?| array[$1])=true
(question mark unaltered and unescaped).

PostgresParameters$Parser should be improved such that

• it either doesn't replace question marks if they are immediately followed by | or &, such that operators ?| and ?& can be used directly, or
• it offers a way to escape question marks in a way that this escape sequence is turned into a single question mark in the final SQL, e.g. recognize ?? and turn that into ?

The second option would even be preferable, because a single question mark can also be a valid operator, as in jsonb ? text → boolean.

See https://www.postgresql.org/docs/current/functions-json.html for details on JSON operators.

If changing PostgresParameters is not an option, it would be helpful if the selection of the Parameter substitution could be overridden by the application, so that I could plugin my own handler. But currently Parameters.instance(Dialect) is all static, so even though the selection is dialect-specific, there is no way to provide a custom Parameters implementation (through a hook in the Dialect or some other kind of configuration).

In previous versions of Hibernate Reactive, I could work around this by overwriting Dialect.addSqlHintOrComment() because that was being called after parameter substitution, so I could "fix" the SQL and do my own (un)escaping of JSON operators, but with the latest version of Hibernate Reactive, this is no longer possible, because parameter substitution is called last.

[danielbobbert]

Just dug even deeper and found out that the problem is actually caused by ReactiveDeferredResultSetAccess, line 151.
At this point, my query has already been processed, with the ?| operator being in place unmodified, and the parameter mapped to array[$1], but then another call to Parameters.instance( dialect ).process( getFinalSql() ) is issued which breaks the operator as described above.

There is even a comment in line 150:
// I'm not sure calling Parameters here is necessary, the query should already have the right parameters

Indeed, just skipping line 151 (or changing it to final String sql = getFinalSql();) instead of calling Parameters.process() would solve the problem.

[DavideD]

Thanks for the feedback, I will look into this soon

[gavinking]

@danielbobbert what is the context of this? A native SQL query?

[danielbobbert]

We have defined hibernate custom functions on our extension of the Postgres dialect. These custom functions attempt to make JSON operators available to HQL queries (we are using Quarkus Panache).

[DavideD]

We cannot just remove the parameter substitution (some tests fail without it), but I will look into it.

[DavideD]

@gavinking Is it safe to say that we shouldn't parse native queries?
This would mean that the user has to write native queries with the right syntax for the parameters.
In our case it means that, for PostgreSQL, they have to specify parameters using the $.

[gavinking]

I agree that we should not parse them. But I don't see why we can't tokenize them, which is all you need here.

Parsing is hard, especially when you don't completely know the grammar of the language you're parsing! Scanning for tokens is close to trivial.

[DavideD]

What I meant to say is: wouldn't be better to just avoid any manipulation of native queries and pass them to the Vert.x SQL client as they are? It seems a nice feature to have for a user and less work for us. I'm not sure what we gain from the tokenization.

[gavinking]

Well I mean that means no named parameters, and we would be taking away a standard feature of JPA.

[danielbobbert]

Also, in our case it's (strictly speaking) not a "native query". We provide the use of the JSON operator as a custom function to JPA (by extending org.hibernate.query.sqm.function.AbstractSqmSelfRenderingFunctionDescriptor), and use that in JPA/Panache queries, so I expect the whole query to undergo some parsing anyways.
That's why I suggested the addition of some reliable form of escape sequence, that could be used, no matter whether it is used in a native query or as part of a custom function.

[danielbobbert]

If it's too hard to come up with a general solution to the problem: It would already be sufficient, if I could plug my own "Parameters" instance, so that I could handle special escape sequences myself. The problem is, that (as already stated above) Parameters.instance(Dialect) is static, so there is no way to provide a custom "Parameters" instance. But since we are using our own Dialect already (in order to provide the hibernate custom functions), a simple extension of the "Parameters" class would allow us to do so:

/* Hibernate, Relational Persistence for Idiomatic Java
 *
 * SPDX-License-Identifier: Apache-2.0
 * Copyright: Red Hat Inc. and Hibernate Authors
 */
package org.hibernate.reactive.pool.impl;

import org.hibernate.dialect.CockroachDialect;
import org.hibernate.dialect.Dialect;
import org.hibernate.dialect.DialectDelegateWrapper;
import org.hibernate.dialect.PostgreSQLDialect;
import org.hibernate.dialect.SQLServerDialect;

import java.util.HashMap;
import java.util.Map;

/**
 * Some databases have a different parameter syntax, which
 * the Vert.x {@link io.vertx.sqlclient.SqlClient} does not abstract.
 * This class converts JDBC/ODBC-style {@code ?} parameters generated
 * by Hibernate ORM to the native format.
 */
public abstract class Parameters {

	// a mapping of custom Parameters objects for custom dialects
	private static final Map<Class<? extends Dialect>, Parameters> CUSTOM_PARAMETERS = new HashMap<>();

	private static final Parameters NO_PARSING = new Parameters() {
		@Override
		public String process(String sql) {
			return sql;
		}

		@Override
		public String process(String sql, int parameterCount) {
			return sql;
		}

		@Override
		public String processLimit(String sql, Object[] parameterArray, boolean hasOffset) {
			return sql;
		}
	};

	/**
	 * Register a custom Parameters parser for the given type of dialect
	 */
	public static void addCustomMapping(Class<? extends Dialect> dialectClass, Parameters parameters) {
		CUSTOM_PARAMETERS.put(dialectClass, parameters);
	}

	public static Parameters instance(Dialect dialect) {
		if ( dialect instanceof DialectDelegateWrapper ) {
			dialect = ( (DialectDelegateWrapper) dialect ).getWrappedDialect();
		}
		// check potential custom parameters first
		Parameters parameters = CUSTOM_PARAMETERS.get(dialect.getClass());
		if (parameters != null) {
			return parameters;
		}
		if (dialect instanceof PostgreSQLDialect || dialect instanceof CockroachDialect ) return PostgresParameters.INSTANCE;
		if (dialect instanceof SQLServerDialect) return SQLServerParameters.INSTANCE;
		return NO_PARSING;
	}

	public static boolean isProcessingNotRequired(String sql) {
		return sql == null
				// There aren't any parameters
				|| sql.indexOf('?') == -1;
	}

	public abstract String process(String sql);

	public abstract String process(String sql, int parameterCount);

	public abstract String processLimit(String sql, Object[] parameterArray, boolean hasOffset);
}