Misleading error message for SQLDeleteClause #518

Closed
cowwoc opened this Issue Oct 12, 2013 · 6 comments

Comments

Projects
None yet
2 participants
@cowwoc
Contributor

cowwoc commented Oct 12, 2013

I tried deleting a row by invoking:

QPermissionClosure parent = new QPermissionClosure("parent");
QPermissionClosure link = new QPermissionClosure("link");
QPermissionClosure child = new QPermissionClosure("child");
long rows = new SQLDeleteClause(connection, dialect, link).where(parent.childId.eq(parentId),
    child.parentId.eq(childId), link.parentId.eq(parent.parentId),
    link.childId.eq(child.childId)).execute();

QueryDSL threw an exception:

java.lang.IllegalArgumentException: Undeclared path 'parent'. Add this path as a source to the query to be able to reference it.
    at com.mysema.query.types.ValidatingVisitor.visit(ValidatingVisitor.java:68) ~[querydsl-core-3.2.3.jar:na]
    at com.mysema.query.types.ValidatingVisitor.visit(ValidatingVisitor.java:30) ~[querydsl-core-3.2.3.jar:na]
    at com.mysema.query.types.PathImpl.accept(PathImpl.java:94) ~[querydsl-core-3.2.3.jar:na]
    at com.mysema.query.types.ValidatingVisitor.visit(ValidatingVisitor.java:55) ~[querydsl-core-3.2.3.jar:na]
    at com.mysema.query.types.ValidatingVisitor.visit(ValidatingVisitor.java:30) ~[querydsl-core-3.2.3.jar:na]
    at com.mysema.query.types.OperationImpl.accept(OperationImpl.java:91) ~[querydsl-core-3.2.3.jar:na]
    at com.mysema.query.DefaultQueryMetadata.validate(DefaultQueryMetadata.java:350) ~[querydsl-core-3.2.3.jar:na]
    at com.mysema.query.DefaultQueryMetadata.addWhere(DefaultQueryMetadata.java:202) ~[querydsl-core-3.2.3.jar:na]
    at com.mysema.query.sql.dml.SQLDeleteClause.where(SQLDeleteClause.java:164) ~[querydsl-sql-3.2.3.jar:na]

but in actuality it is impossible to reference more than one table from the DELETE statement so this error message is misleading. I believe the correct approach is to use DELETE ... WHERE EXISTS (subquery). Ideally QueryDSL should point users in the right direction by telling them that:

  • delete operations only support referencing a single table, and
  • if they need to reference multiple tables they should look into DELETE ... WHERE EXISTS (subquery)

Perhaps this? IllegalArgumentException: Undeclared path 'parent'. A delete operation can only reference a single table. Consider this alternative: DELETE ... WHERE EXISTS (subquery)

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Oct 24, 2013

Member

How do you suggest to handle the different messages? I am asking since the ValidatingVisitor is declared in querydsl-core.

I am not yet sure what solution would be best. Rethrowing and customizing the ValidatingVisitor came to my mind.

Member

timowest commented Oct 24, 2013

How do you suggest to handle the different messages? I am asking since the ValidatingVisitor is declared in querydsl-core.

I am not yet sure what solution would be best. Rethrowing and customizing the ValidatingVisitor came to my mind.

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Oct 24, 2013

Contributor

It sounds to me like you need to run an SQL-specific validator, which means this code has to move from querydsl-core to querydsl-sql, at least for the SQL module. Meaning, if a visitor exists inside the sub-module, use it. Otherwise, bubble up and let core's default validator run instead.

Is that what you meant when you talked about rethrowing and customizing ValidatingVisitor?

Contributor

cowwoc commented Oct 24, 2013

It sounds to me like you need to run an SQL-specific validator, which means this code has to move from querydsl-core to querydsl-sql, at least for the SQL module. Meaning, if a visitor exists inside the sub-module, use it. Otherwise, bubble up and let core's default validator run instead.

Is that what you meant when you talked about rethrowing and customizing ValidatingVisitor?

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Oct 28, 2013

Member

But the validation logic is the same the error message is just different.

Member

timowest commented Oct 28, 2013

But the validation logic is the same the error message is just different.

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Oct 29, 2013

Contributor

Perhaps have two validation subclasses inherit from the same base class, but throw different error messages in each case?

Contributor

cowwoc commented Oct 29, 2013

Perhaps have two validation subclasses inherit from the same base class, but throw different error messages in each case?

timowest added a commit that referenced this issue Oct 29, 2013

Make error message customizable
Use customized error message in SQLDeleteClause #518
@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Nov 17, 2013

Member

Released in 3.3.0.BETA1

Member

timowest commented Nov 17, 2013

Released in 3.3.0.BETA1

@timowest timowest closed this Nov 17, 2013

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Nov 18, 2013

Contributor

Excellent work. Thanks!

Contributor

cowwoc commented Nov 18, 2013

Excellent work. Thanks!

@timowest timowest added this to the 3.3.0 milestone Apr 13, 2014

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