New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify table comparison behaviour #906

Closed
cowwoc opened this Issue Aug 22, 2014 · 10 comments

Comments

Projects
None yet
3 participants
@cowwoc
Contributor

cowwoc commented Aug 22, 2014

Correct me if I'm wrong but I believe QueryDSL-SQL is allowing users to construct invalid SQL.

It allows table comparison in the form tableA <> tableB whereas I am only expecting to be able to express field comparison in the form tableA.field <> tableB.field. Here is my query:

QPermissionClosure parent = new QPermissionClosure("parent");
QPermissionClosure child = new QPermissionClosure("child");

new SQLQuery(connection, configuration).
where(parent.childId.eq(parentId), child.parentId.eq(childId), parent.ne(child)).
list(parent.parentId, child.childId, parent.distance.add(child.distance).add(1));

where parent and child are tables. I don't think parent.ne(child) should be allowed.

@johnjaylward

This comment has been minimized.

Show comment
Hide comment
@johnjaylward

johnjaylward Aug 22, 2014

Q Types are not "tables" they are individual records in the table. When you use a comparison like that you are implicitly telling Query DSL that you want to compare 2 rows based on primary keys.

Q Types are not "tables" they are individual records in the table. When you use a comparison like that you are implicitly telling Query DSL that you want to compare 2 rows based on primary keys.

@johnjaylward

This comment has been minimized.

Show comment
Hide comment
@johnjaylward

johnjaylward Aug 22, 2014

You example, translated to SQL would look something like this (assuming your example even ran, because you don't have a "From" clause):

SELECT parent.parentId, child.childId, (parent.distance + child.distance + 1)
FROM PermissionClosure parent, PermissionClosure child
WHERE parent.childId = parentIdParam AND child.parentId = childIdParam AND parent.id <> child.id

You example, translated to SQL would look something like this (assuming your example even ran, because you don't have a "From" clause):

SELECT parent.parentId, child.childId, (parent.distance + child.distance + 1)
FROM PermissionClosure parent, PermissionClosure child
WHERE parent.childId = parentIdParam AND child.parentId = childIdParam AND parent.id <> child.id

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Aug 22, 2014

Contributor

Gotcha. Perhaps this should be mentioned in the Javadoc (the fact that comparing rows implies comparing their primary keys)?

PS: In my case, the table has no primary key which brings up the question of what happens then.

Contributor

cowwoc commented Aug 22, 2014

Gotcha. Perhaps this should be mentioned in the Javadoc (the fact that comparing rows implies comparing their primary keys)?

PS: In my case, the table has no primary key which brings up the question of what happens then.

@johnjaylward

This comment has been minimized.

Show comment
Hide comment
@johnjaylward

johnjaylward Aug 22, 2014

In your case you would get a runtime exception, either from your database, or query dsl, I don't remember which (Maybe Timo or someone on the QueryDSL team could give a better answer there). Generally it's a good idea to have a primary, if not at least a unique key. If you have a unique key, you should see if it can double as a multi-field primary at least in your model definition (not necessary in the database itself)

In your case you would get a runtime exception, either from your database, or query dsl, I don't remember which (Maybe Timo or someone on the QueryDSL team could give a better answer there). Generally it's a good idea to have a primary, if not at least a unique key. If you have a unique key, you should see if it can double as a multi-field primary at least in your model definition (not necessary in the database itself)

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Aug 22, 2014

Contributor

Got it thanks. So I guess the only outstanding issue is the documentation update.

Contributor

cowwoc commented Aug 22, 2014

Got it thanks. So I guess the only outstanding issue is the documentation update.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Aug 22, 2014

Member

Currently this equality contract is not yet implemented in Querydsl. The primary key based condition generation is fine for me.

Member

timowest commented Aug 22, 2014

Currently this equality contract is not yet implemented in Querydsl. The primary key based condition generation is fine for me.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Aug 23, 2014

Member

@cowwoc Could you take a look at the pull request?

Member

timowest commented Aug 23, 2014

@cowwoc Could you take a look at the pull request?

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Aug 23, 2014

Contributor

Looks good to me. I didn't test the code (because my own application no longer needs this comparison) but I reviewed your changes and they make sense. Thank you!

Contributor

cowwoc commented Aug 23, 2014

Looks good to me. I didn't test the code (because my own application no longer needs this comparison) but I reviewed your changes and they make sense. Thank you!

@timowest timowest added this to the 3.4.3 milestone Aug 23, 2014

@timowest timowest closed this in #907 Aug 24, 2014

@timowest timowest changed the title from Disallow table comparison to Clarify table comparison behaviour Aug 24, 2014

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Aug 24, 2014

Member

I updated the issue title to reflect the situation better.

Member

timowest commented Aug 24, 2014

I updated the issue title to reflect the situation better.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Aug 31, 2014

Member

Released in 3.4.3

Member

timowest commented Aug 31, 2014

Released in 3.4.3

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