Backlog: Column name replacement caused by @Column annotation produces invalid SQL for property named 'in' #2725
Replies: 9 comments
-
|
The question becomes ... why don't you change that logical from property from |
Beta Was this translation helpful? Give feedback.
-
|
We did ... the property's maiden name ist now 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 😇 |
Beta Was this translation helpful? Give feedback.
-
|
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.
Yup, cool.
There isn't enough information yet. This issue mentions |
Beta Was this translation helpful? Give feedback.
-
|
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):
An example stack trace: The stack traces of the failures all share that the exception is produced during the find/update operation of CQueryEngine, i.e. for the findList/findMap operations or for the update operations. |
Beta Was this translation helpful? Give feedback.
-
|
I tried briefly to narrow down the problem: CQueryPredicates::parsePropertiesToDbColumns tries to convert properties to db columns is invoked with This sounds logical, as the deployParser is not intelligent enough to detect that 'in' is an operand here and no property. 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 |
Beta Was this translation helpful? Give feedback.
-
@focbenz @Tegmango AFAIR from our last discussion, there was an existing interface interface InterFace {
String getIn();
String getOut();
}And you had an I've made some experiments, if I can modify the DeployParser to detect, if 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 I thought with @rbygrave what do you think, should this be fixed or is a restriction of ebean |
Beta Was this translation helpful? Give feedback.
-
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 That is, produce Currently, "raw expression parsing" occurs on all the expressions - not just That is, we are encouraged via documentation to use So that thinking heads towards ... maybe ebean should not do that and instead be stricter such that only so, maybe the problem becomes more obvious if we look at the example:
... because
... but today, the raw parser would still replace that Hmmm. |
Beta Was this translation helpful? Give feedback.
-
|
Hmmm, I wonder if its just a case of
current -> instead produce
produces ...
... and the parser only works on content between
that produces content without the |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
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:
And start all tests, which will produce several failures, for example EqlParserTest.where_in_when_namedParams_withNoWhitespace:
Beta Was this translation helpful? Give feedback.
All reactions