QueryDSL-SQL addBatch() with null values #74

Closed
H0 opened this Issue Dec 21, 2011 · 14 comments

Comments

Projects
None yet
2 participants
@H0

H0 commented Dec 21, 2011

QueryDsl classes generated from this:

CREATE TABLE FOO(ID INT IDENTITY(1,1), c1 int null,c2 int null)

Code snippets:

QFoo f= QFoo.foo;
SQLInsertClause sic = new SQLInsertClause(c, new H2Templates(), f);
sic.columns(f.c1,f.c2).values(null,null).addBatch();
sic.columns(f.c1,f.c2).values(null,1).addBatch();
sic.execute();

or

QFoo f= QFoo.foo;
SQLInsertClause sic = new SQLInsertClause(c, new H2Templates(), f);
Foo f1=new Foo();
sic.populate(f1).addBatch();
f1=new Foo();
f1.setC1(1);
sic.populate(f1).addBatch();
sic.execute();

Result:

Exception in thread "main" java.lang.IllegalArgumentException: org.h2.jdbc.JdbcSQLException: Invalid value "1" for parameter "parameterIndex" [90008-161]
    at com.mysema.query.sql.dml.AbstractSQLClause.setParameters(AbstractSQLClause.java:66)
    at com.mysema.query.sql.dml.SQLInsertClause.createStatement(SQLInsertClause.java:239)
    at com.mysema.query.sql.dml.SQLInsertClause.execute(SQLInsertClause.java:292)
    at querydsl.bug.BugTest.main(BugTest.java:59)
Caused by: org.h2.jdbc.JdbcSQLException: Invalid value "1" for parameter "parameterIndex" [90008-161]
    at org.h2.message.DbException.getJdbcSQLException(DbException.java:329)
    at org.h2.message.DbException.get(DbException.java:169)
    at org.h2.message.DbException.getInvalidValueException(DbException.java:215)
    at org.h2.jdbc.JdbcPreparedStatement.setParameter(JdbcPreparedStatement.java:1270)
    at org.h2.jdbc.JdbcPreparedStatement.setInt(JdbcPreparedStatement.java:308)
    at com.mysema.query.sql.types.IntegerType.setValue(IntegerType.java:41)
    at com.mysema.query.sql.types.IntegerType.setValue(IntegerType.java:24)
    at com.mysema.query.sql.Configuration.set(Configuration.java:93)
    at com.mysema.query.sql.dml.AbstractSQLClause.setParameters(AbstractSQLClause.java:64)
    ... 3 more

Seems that engine not recognizing nulls as necessary to be included(as "NULL") in prepared stmt, this behavoir is not valid in cases like above.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 8, 2012

Member

@H0 Could you verify that it works now for you?

Member

timowest commented Feb 8, 2012

@H0 Could you verify that it works now for you?

@H0

This comment has been minimized.

Show comment
Hide comment
@H0

H0 Feb 12, 2012

@timowest
With 2.3.1.BUILD-SNAPSHOT from mvn repo source.mysema.com/maven2/snapshots/, first code snippet runs w/o errors, second(using beans) fails with same error.

NB: Using artifacts from git is tricky: "cd querydsl-root && mvn install -P sql -DskipTests=true".

H0 commented Feb 12, 2012

@timowest
With 2.3.1.BUILD-SNAPSHOT from mvn repo source.mysema.com/maven2/snapshots/, first code snippet runs w/o errors, second(using beans) fails with same error.

NB: Using artifacts from git is tricky: "cd querydsl-root && mvn install -P sql -DskipTests=true".

@H0 H0 closed this Feb 12, 2012

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 12, 2012

Member

The second issue is a bit more difficult to fix. I can either use an INSERT statements with parameter slots for all columns or do some normalization of the batches to make sure that all columns are covered.

Member

timowest commented Feb 12, 2012

The second issue is a bit more difficult to fix. I can either use an INSERT statements with parameter slots for all columns or do some normalization of the batches to make sure that all columns are covered.

@H0 H0 reopened this Feb 12, 2012

timowest added a commit that referenced this issue Feb 12, 2012

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 12, 2012

Member

@H0 You need an easier way to build without tests?

Member

timowest commented Feb 12, 2012

@H0 You need an easier way to build without tests?

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 12, 2012

Member

I found now a way to support the second issue. You can use an alterantive Mapper in the populate call to bind also the null values explicitly :

Mapper<Object> mapper = DefaultMapper.WITH_NULL_BINDINGS; 
...
sic.populate(f1, mapper).addBatch();
Member

timowest commented Feb 12, 2012

I found now a way to support the second issue. You can use an alterantive Mapper in the populate call to bind also the null values explicitly :

Mapper<Object> mapper = DefaultMapper.WITH_NULL_BINDINGS; 
...
sic.populate(f1, mapper).addBatch();

timowest added a commit that referenced this issue Feb 12, 2012

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 12, 2012

Member

When you use the default mapper then there is a risk of an error if the binding columns don't match in a batch insert. Using the null bindings enabled mapper all binding columns are always used.

Member

timowest commented Feb 12, 2012

When you use the default mapper then there is a risk of an error if the binding columns don't match in a batch insert. Using the null bindings enabled mapper all binding columns are always used.

@H0

This comment has been minimized.

Show comment
Hide comment
@H0

H0 Feb 12, 2012

You need an easier way to build without tests?
@timowest Just looking for nicer way to use sources without installing them in local mvn repo, but looks like it SHOULD be done that way: so, mvn install or use mysema snapshots repo, they both not so scary after all :)

I found now a way to support the second issue
It works, thanks!
Just interesting: this is most elegant way from this project architecture pov? Idea: maintaining fields mask and finally constructing prepared statement field list based on that mask.

H0 commented Feb 12, 2012

You need an easier way to build without tests?
@timowest Just looking for nicer way to use sources without installing them in local mvn repo, but looks like it SHOULD be done that way: so, mvn install or use mysema snapshots repo, they both not so scary after all :)

I found now a way to support the second issue
It works, thanks!
Just interesting: this is most elegant way from this project architecture pov? Idea: maintaining fields mask and finally constructing prepared statement field list based on that mask.

@H0 H0 closed this Feb 12, 2012

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 13, 2012

Member

I'll close this when it's released

Just interesting: this is most elegant way from this project architecture pov? Idea: maintaining fields mask and finally constructing prepared statement field list based on that mask.
This could definitely be improved and supplying a field mask might work. You can open a new ticket for it, if you like.

Member

timowest commented Feb 13, 2012

I'll close this when it's released

Just interesting: this is most elegant way from this project architecture pov? Idea: maintaining fields mask and finally constructing prepared statement field list based on that mask.
This could definitely be improved and supplying a field mask might work. You can open a new ticket for it, if you like.

@timowest timowest reopened this Feb 13, 2012

@H0

This comment has been minimized.

Show comment
Hide comment
@H0

H0 Feb 14, 2012

@timowest
Another problem: both snippets fails with classes from following table:

CREATE TABLE FOO(ID INT IDENTITY(1,1), c1 int null,c2 int null, c3 int not null)

H0 commented Feb 14, 2012

@timowest
Another problem: both snippets fails with classes from following table:

CREATE TABLE FOO(ID INT IDENTITY(1,1), c1 int null,c2 int null, c3 int not null)
@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 14, 2012

Member

c3 is not null, isn't that understandable?

Member

timowest commented Feb 14, 2012

c3 is not null, isn't that understandable?

@H0

This comment has been minimized.

Show comment
Hide comment
@H0

H0 Feb 14, 2012

Sorry for inattention, actual situation is like this:

CREATE TABLE BAR(ID INT IDENTITY(1,1), c1 int null,c2 int default 1 not null)

But i cant handle assigning default value even with PreparedStatement:

PreparedStatement ps = c.prepareStatement("INSERT INTO BAR (c1,c2) values (?, ?)");
ps.setInt(1, 2);
//ps.setDefaultValue(); - not exist
ps.addBatch();
//...
ps.executeBatch();

Maybe this can be done via setObject(), but i don know how.

H0 commented Feb 14, 2012

Sorry for inattention, actual situation is like this:

CREATE TABLE BAR(ID INT IDENTITY(1,1), c1 int null,c2 int default 1 not null)

But i cant handle assigning default value even with PreparedStatement:

PreparedStatement ps = c.prepareStatement("INSERT INTO BAR (c1,c2) values (?, ?)");
ps.setInt(1, 2);
//ps.setDefaultValue(); - not exist
ps.addBatch();
//...
ps.executeBatch();

Maybe this can be done via setObject(), but i don know how.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 14, 2012

Member

Maybe the default value can be retrieved via JDBC parameter metadata. But that might be a bit fragile and probably not portable.

Member

timowest commented Feb 14, 2012

Maybe the default value can be retrieved via JDBC parameter metadata. But that might be a bit fragile and probably not portable.

@H0

This comment has been minimized.

Show comment
Hide comment
@H0

H0 Feb 16, 2012

From common sense this should be as simple as providing DEFAULT keyword(like NULL) for particular parameter, but there is no viable JDBC-way of doing this without messing with driver, and more than that: while most DBs support insert into bar (c1,c2) values (1, default), they(h2,mssql2005) cannot insert into bar (c1,c2) values (1, coalesce(null, default)) facepalm.

H0 commented Feb 16, 2012

From common sense this should be as simple as providing DEFAULT keyword(like NULL) for particular parameter, but there is no viable JDBC-way of doing this without messing with driver, and more than that: while most DBs support insert into bar (c1,c2) values (1, default), they(h2,mssql2005) cannot insert into bar (c1,c2) values (1, coalesce(null, default)) facepalm.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 19, 2012

Member

released in 2.3.2

Member

timowest commented Feb 19, 2012

released in 2.3.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment