Column name replacement caused by @Column annotation produces invalid SQL for property named 'in'

[focAsbrand]

Rewriting this into an issue since we were advised that it'll be more useful than a Google Group post 😉 :

Expected behavior

Properties named 'in' can be used in Java code by specifying a different column name using the @column annotation.

Actual behavior

Properties named 'in' with a @column annotation produce SQL syntax errors since the SQL keyword 'in' is also being replaced by the column name specified for the property.
For example, if we have a property named 'in' with a @column(name="inputName") annotation and the correctly created SQL would be:

select t0.id, t0.status, t0.name, t0.inputName, t0.smallnote, t0.anniversary, t0.cretime, t0.updtime, t0.version, t0.billing_address_id, t0.shipping_address_id from o_customer t0 where t0.name in (?,?)

the SQL-"in" of the where clause also mistakenly gets replaced by the column name, producing

select t0.id, t0.status, t0.name, t0.inputName, t0.smallnote, t0.anniversary, t0.cretime, t0.updtime, t0.version, t0.billing_address_id, t0.shipping_address_id from o_customer t0 where t0.name t0.inputName (?,?)

which leads to a SQLException due to invalid syntax.

Steps to reproduce

Add a property named 'in' with an arbitrary column name by using @column(name = "...") to any entity, for example Customer in ebean-test:

  @Size(max = 40)
  @Column(name = "inputName")
  String in;

And start all tests, which will produce several failures, for example EqlParserTest.where_in_when_namedParams_withNoWhitespace:

Error:  Tests run: 83, Failures: 0, Errors: 7, Skipped: 3, Time elapsed: 0.311 s <<< FAILURE! - in io.ebean.xtest.internal.server.grammer.EqlParserTest
Error:  io.ebean.xtest.internal.server.grammer.EqlParserTest.where_in_when_namedParams_withNoWhitespace  Time elapsed: 0.003 s  <<< ERROR!
javax.persistence.PersistenceException: 
Query threw SQLException:Syntax error in SQL statement "select t0.id, t0.status, t0.name, t0.inputName, t0.smallnote, t0.anniversary, t0.cretime, t0.updtime, t0.version, t0.billing_address_id, t0.shipping_address_id from o_customer t0 where t0.name [*]t0.inputName (?,?)"; SQL statement:
select t0.id, t0.status, t0.name, t0.inputName, t0.smallnote, t0.anniversary, t0.cretime, t0.updtime, t0.version, t0.billing_address_id, t0.shipping_address_id from o_customer t0 where t0.name t0.inputName (?,?) [42000-212] Bind values:[null] Query was:select t0.id, t0.status, t0.name, t0.inputName, t0.smallnote, t0.anniversary, t0.cretime, t0.updtime, t0.version, t0.billing_address_id, t0.shipping_address_id from o_customer t0 where t0.name t0.inputName (?,?)

[rbygrave]

The question becomes ... why don't you change that logical from property from in to something else?

[focbenz]

We did ... the property's maiden name ist now inParams.
So we do not have an impediment but wanted to point this out to the community and get your opinion on if this should be treated as a bug and in that case deliver a test case for it.

As Tegmango pointed out, he tried to use the Google Groups mailing list first (https://groups.google.com/g/ebean/c/zW-Ng_nhHNo), but got no response. So sorry for spamming the typical Ebean issues watcher 😇
Feel free to close this one if you think the effort is not worth it to make @Column(name = "somethingElseThanIn") work in that special SQL keyword case. We can also add some documentation near the @Column annotation that with specific SQL reserved keywords like in it might be better to just avoid them.

[rbygrave]

(https://groups.google.com/g/ebean/c/zW-Ng_nhHNo),

Ah, I'm not sure why I didn't see that. If someone could add any sort of reply onto that google group post ... to check that I see it in my email that would be good.

point this out to the community

Yup, cool.

get your opinion on if this should be treated as a bug

There isn't enough information yet. This issue mentions EqlParserTest so maybe it is only hit via EQL parsing? If so maybe that is worth looking at, but there are other places where logical to physical parsing mapping occurs - so really we need more specific failing tests.

[focAsbrand]

I have compiled a list of failing tests due to SQL syntax errors caused by adding the optional property in the Customer class (extracted out of FOCONIS@fb0994b):

• io.ebean.xtest.internal.server.grammer.EqlParserTest.where_in_when_namedParams_withNoWhitespace
• io.ebean.xtest.internal.server.grammer.EqlParserTest.where_inOrEmpty_withVals
• io.ebean.xtest.internal.server.grammer.EqlParserTest.where_inOrEmpty_withValsAsSet
• io.ebean.xtest.internal.server.grammer.EqlParserTest.where_in_when_namedParams
• io.ebean.xtest.internal.server.grammer.EqlParserTest.where_in
• io.ebean.xtest.base.UpdateQueryTest.query_asUpdate_idIn
• io.ebean.xtest.base.DtoQuery2Test
• io.ebean.xtest.json.JsonContextTest.test_json_loadContext
• org.tests.query.lazy.TestQueryDefaultBatchSize.test_findEach
• org.tests.query.TestQueryInIdTypeConversion.test
• org.tests.query.TestWhereRawClause.testRaw_bindExpansion_subquery
• org.tests.query.TestWhereRawClause.testRaw_bindExpansion
• org.tests.query.TestWhereRawClause.testRawOrEmpty_when_notEmpty
• org.tests.aggregateformula.TestAggregateFormula.minOnManyToOne_withFetchQueryWithJoin
• org.tests.aggregateformula.TestAggregateFormula.minOnManyToOne_withFetchQuery
• org.tests.cache.TestBeanFetchJoinCache.fetchCache_when_someHits
• org.tests.cache.TestBeanFetchJoinCache.fetchGroup_fetchCache_partial
• org.tests.basic.TestLoadBeanCache.testLoadWithFindMap
• org.tests.basic.TestLazyLoadInCache.testLoadInCache
• org.tests.basic.TestBatchLazy.testMe
• org.tests.basic.TestDeleteByIdCollection.test
• org.tests.basic.TestUpdate.testReuse
• org.tests.basic.TestUpdate.testNormal
• org.tests.docstore.CustomerReportTest.testFromJson
• org.tests.batchload.TestQueryJoin.test
• org.tests.batchload.TestSecondaryQueries.fetchLazy
• org.tests.batchload.TestSecondaryQueries.fetchQuery
• org.tests.batchload.TestLazyJoin2.testLazyOnNonLoaded
• org.tests.repository.TestBeanRepository.test
• org.tests.rawsql.nativesql.TestNativeSqlBasic.partialAssocIncludingOracle
• org.tests.rawsql.nativesql.TestNativeSqlBasic.partialAndLazyLoad
• org.tests.rawsql.TestRawSqlOrmQuery.testPaging
• org.tests.rawsql.TestRawSqlOrmQuery.testFirstRowsMaxRows
• org.tests.text.json.TestTextJsonInvokeLazy.test
• org.tests.text.json.TestTextJsonReadManyLazyLoad.test_lazyLoadBoth
• org.tests.text.json.TestTextJsonReadManyLazyLoad.test_lazyLoadCust

An example stack trace:

2022-04-25T10:42:49.6579511Z javax.persistence.PersistenceException: 
2022-04-25T10:42:49.6580056Z Query threw SQLException:Syntax error in SQL statement "select t0.id, t0.name from o_customer t0 where t0.id [*]t0.inputName (?,?,?,?,?)"; SQL statement:
2022-04-25T10:42:49.6580932Z select t0.id, t0.name from o_customer t0 where t0.id t0.inputName (?,?,?,?,?) [42000-212] Bind values:[null] Query was:select t0.id, t0.name from o_customer t0 where t0.id t0.inputName (?,?,?,?,?)
2022-04-25T10:42:49.6581566Z 	at io.ebean.config.dbplatform.SqlCodeTranslator.translate(SqlCodeTranslator.java:80)
2022-04-25T10:42:49.6582154Z 	at io.ebean.config.dbplatform.DatabasePlatform.translate(DatabasePlatform.java:245)
2022-04-25T10:42:49.6582741Z 	at io.ebeaninternal.server.query.CQueryEngine.translate(CQueryEngine.java:138)
2022-04-25T10:42:49.6583376Z 	at io.ebeaninternal.server.query.DefaultOrmQueryEngine.translate(DefaultOrmQueryEngine.java:43)
2022-04-25T10:42:49.6587300Z 	at io.ebeaninternal.server.core.OrmQueryRequest.translate(OrmQueryRequest.java:58)
2022-04-25T10:42:49.6588183Z 	at io.ebeaninternal.server.query.CQuery.createPersistenceException(CQuery.java:647)
2022-04-25T10:42:49.6588805Z 	at io.ebeaninternal.server.query.CQueryEngine.findMany(CQueryEngine.java:370)
2022-04-25T10:42:49.6589520Z 	at io.ebeaninternal.server.query.DefaultOrmQueryEngine.findMany(DefaultOrmQueryEngine.java:131)
2022-04-25T10:42:49.6590160Z 	at io.ebeaninternal.server.core.OrmQueryRequest.findList(OrmQueryRequest.java:404)
2022-04-25T10:42:49.6590737Z 	at io.ebeaninternal.server.core.DefaultServer.findList(DefaultServer.java:1444)
2022-04-25T10:42:49.6591302Z 	at io.ebeaninternal.server.core.DefaultServer.findList(DefaultServer.java:1423)
2022-04-25T10:42:49.6591896Z 	at io.ebeaninternal.server.core.DefaultBeanLoader.executeQuery(DefaultBeanLoader.java:154)
2022-04-25T10:42:49.6592672Z 	at io.ebeaninternal.server.core.DefaultBeanLoader.loadBean(DefaultBeanLoader.java:137)
2022-04-25T10:42:49.6593260Z 	at io.ebeaninternal.server.core.DefaultServer.loadBean(DefaultServer.java:536)`

The stack traces of the failures all share that the exception is produced during the find/update operation of CQueryEngine, i.e.

2022-04-25T10:42:33.1766741Z 	at io.ebeaninternal.server.query.CQueryEngine.findMany(CQueryEngine.java:370)
2022-04-25T10:42:33.1767362Z 	at io.ebeaninternal.server.query.DefaultOrmQueryEngine.findMany(DefaultOrmQueryEngine.java:131)

for the findList/findMap operations or

2022-04-25T10:42:23.3980550Z 	at io.ebean.config.dbplatform.SqlCodeTranslator.translate(SqlCodeTranslator.java:80)
2022-04-25T10:42:23.3981160Z 	at io.ebean.config.dbplatform.DatabasePlatform.translate(DatabasePlatform.java:245)
2022-04-25T10:42:23.3981746Z 	at io.ebeaninternal.server.query.CQueryEngine.translate(CQueryEngine.java:138)
2022-04-25T10:42:23.3982348Z 	at io.ebeaninternal.server.query.CQueryEngine.executeUpdate(CQueryEngine.java:87)
2022-04-25T10:42:23.3982922Z 	at io.ebeaninternal.server.query.CQueryEngine.update(CQueryEngine.java:76)

for the update operations.

[rPraml]

I tried briefly to narrow down the problem:

CQueryPredicates::parsePropertiesToDbColumns tries to convert properties to db columns

 private String deriveWhere(DeployParser deployParser) {
    return parse(whereExprSql, deployParser);
  }

is invoked with whereExprSql = "id in (?,?,?,?,?)"
The return value is ${}id ${}inputName (?,?,?,?,?)

This sounds logical, as the deployParser is not intelligent enough to detect that 'in' is an operand here and no property.
(corner case: in in (?,?,?,?,?) the first in is a property, the second an operand)

I think a fix (if there even is an easy one) should be located somewhere here: https://github.com/ebean-orm/ebean/blob/master/ebean-core/src/main/java/io/ebeaninternal/server/deploy/DeployParser.java#L68
There are already some special handlings for table name implemented (see skipWordConvert())
Perhaps one can say that once a property has been converted, the next word must be an operand and must not be converted.
I'll think about it again

[rPraml]

The question becomes ... why don't you change that logical from property from in to something else?

@focbenz @Tegmango AFAIR from our last discussion, there was an existing interface

interface InterFace {
   String getIn();
   String getOut();
}

And you had an Entity implements InterFace right?
So, it was obvious to name the properties in and out

I've made some experiments, if I can modify the DeployParser to detect, if in is a property or an operator:
I thougt it would make sense to treat every word after an replacement as operator. This fixed most of the test, but things like sum(status * name) did not work, as the * isn't handled in the word loop but in findWordStart()
Then I checked, if the word is really an operator. This would work, as long you do not have raw SQL like concat(out, in) (Note: concat(in, out) would work.

diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/DeployParser.java b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/DeployParser.java
index 184e7ad18..52c92d644 100644
--- a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/DeployParser.java
+++ b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/DeployParser.java
@@ -65,12 +65,16 @@ public abstract class DeployParser {
     this.source = source;
     this.sourceLength = source.length();
     this.sb = new StringBuilder(source.length() + 20);
+    boolean lastReplaced = false;
     while (nextWord()) {
-      if (skipWordConvert()) {
+      if (skipWordConvert()
+        || lastReplaced && isOperator()) {
         sb.append(word);
         priorWord = word;
+        lastReplaced = false;
       } else {
         String deployWord = convertWord();
+        lastReplaced = !word.equals(deployWord); // replacement happened
         sb.append(deployWord);
         priorWord = deployWord;
       }
@@ -86,6 +90,19 @@ public abstract class DeployParser {
     return sb.toString();
   }

+  boolean isOperator() {
+    switch (word) {
+      case "in":
+      case "between":
+      case "and":
+      case "or":
+      case "not": // list all operators here (like,exists,any,some,...)
+        return true;
+      default:
+        return false;
+    }
+  }
+
   boolean skipWordConvert() {
     return false;
   }

I think this requires some rewrite of the DeployParser, where I really do not know, if it is worth, because there could be many situations, where you can trick the parser 1=1 in (true)

I thought with @ColumnName you are safe, if you use a reserved property name. (We often use key and value)
But for now, I would say, reserved names should be avoided whenever possible, especially if they are operators.

@rbygrave what do you think, should this be fixed or is a restriction of ebean

[rbygrave]

is invoked with whereExprSql = "id in (?,?,?,?,?)" The return value is ${}id ${}inputName (?,?,?,?,?)

I think the question is, is there a design change that would skip CQueryPredicates::parsePropertiesToDbColumns for the non-raw sql case. (Approximately, change such that expressions that are not raw() don't get the "raw sql parsing")

That is, produce "${}id in (?,?,?,?,?)" and skip the parsing for this expression altogether. Note: the ${} prefix here means "root level so replace ${} with the root level table alias which is generally t0. ${customer.billingAddress}city means replace ${customer.billingAddress} with the table alias for the join to customer.billingAddress (for example, that might be t1).

Currently, "raw expression parsing" occurs on all the expressions - not just raw(). That is, while we generally write query.where().eq("customer.name", "rob") we can actually do query.where().eq("any_sql_function_we_like(customer.name)", "rob") ... the eq expression actually gets the same "raw sql parsing" applied to it.

That is, we are encouraged via documentation to use raw() for cases like this via: query.where().raw("any_sql_function_we_like(customer.name) = ?", "rob") ... but actually the raw parsing also occurs for eq, in etc - well, all expressions in fact. That is, CQueryPredicates::parsePropertiesToDbColumns operates on the whole where clause, not "just the raw parts".

So that thinking heads towards ... maybe ebean should not do that and instead be stricter such that only raw expressions get the full "raw parsing".

so, maybe the problem becomes more obvious if we look at the example:

query.where().raw("any_sql_function_we_like(in) in (?,?,?)", "a", "b, "c")

... because in exists twice as both "logical property name" AND "sql operator in". BUT, if we ever hit this case we can instead NOT use the logical property name in and use the physical t0.input_name instead like:

query.where().raw("any_sql_function_we_like(t0.input_name) in (?,?,?)", "a", "b, "c")

... but today, the raw parser would still replace that in operator with ${}input_name ... so it wouldn't quite work. We'd need some ability to turn off the raw parsing of this expression -> "ebean, please don't parse this because I have a logical bean property that is also the same name as a sql operator I'm using" (in, exists etc)

Hmmm.

[rbygrave]

Hmmm, I wonder if its just a case of

1. changing what is produced before it gets to the parser:

current -> instead produce

id in (?,?) -> ${}id in (?,?)
in in (?,?) -> ${}in in (?,?)
customer.name = ? -> ${customer}name = ?
customer.billingAddress.city in (?,?) -> ${customer.billingAddress}city in (?,?)

2. only apply the parsing to the raw() expressions

.raw("any_sql_function_we_like(customer.name) in (?,?,?)", "a", "b, "c")

produces ...

$ebean[any_sql_function_we_like(customer.name) in (?,?,?)]ebean$

... and the parser only works on content between $ebean[ and ]ebean$ and outputs for that section of the content

any_sql_function_we_like(${customer}name) in (?,?,?)

3. Have the ability to turn it off altogether like adding a .rawUnparsed() expression:

.rawUnparsed("any_sql_function_we_like(t0.input_name) in (?,?,?)", "a", "b, "c")

that produces content without the $ebean[ and ]ebean$ ... so skips parsing. We are safe to have in as a logical bean property when using .rawUnparsed() - it's not going to try and convert in to t0.input_name here.

[rbygrave]

Ok, I'm going to transfer this into Discusssions -> Backlog

The outstanding action, is to investigate if we can and should improve the parsing as described in the last post.