SQL: select for update with uniqueResult #327

Closed
ssaarela opened this Issue Jan 17, 2013 · 4 comments

Comments

Projects
None yet
2 participants
@ssaarela
Contributor

ssaarela commented Jan 17, 2013

Select for update with AbstractSQLQuery.uniqueResult produces illegal SQL on Oracle:

select * from (
  select ...
  from ...
  where ...
  for update
) where rownum <= ?

-> ORA-00907: missing right parenthesis

Putting the "for update" at the end of the query should work

select * from (
  ...
) where rownum <= ?
for update

Anyhow, I'm not quite sure I like that uniqueResult (as well as singleResult) wrap the actual query with rownum-wrapper-select. It would probably be safer and at least more transparent if it would use the query as such and then process only the first row. Using those methods implies that user knows there is only one relevant match.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 18, 2013

Member

Both singleResult and uniqueResult have implied limits in all modules. singleResult implies that the user is interested only in the first row and uniqueResult that only a single row result is valid.

Member

timowest commented Jan 18, 2013

Both singleResult and uniqueResult have implied limits in all modules. singleResult implies that the user is interested only in the first row and uniqueResult that only a single row result is valid.

@ssaarela

This comment has been minimized.

Show comment
Hide comment
@ssaarela

ssaarela Jan 18, 2013

Contributor

Thanks for the fix!

I agree about the semantics of those methods, just not the implementation as it modifies the request in a way that may even break the query. It would be easy enough to achieve the same semantics without query modification:

singleResult(x)
  iter = iterate(x)
  return iter.hasNext() ? iter.next() : null
  // close
}
uniqueResult(x)
  // as singleResult but throws an exception if iter.hasNext() after the first next() call
Contributor

ssaarela commented Jan 18, 2013

Thanks for the fix!

I agree about the semantics of those methods, just not the implementation as it modifies the request in a way that may even break the query. It would be easy enough to achieve the same semantics without query modification:

singleResult(x)
  iter = iterate(x)
  return iter.hasNext() ? iter.next() : null
  // close
}
uniqueResult(x)
  // as singleResult but throws an exception if iter.hasNext() after the first next() call
@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 18, 2013

Member

I agree about the semantics of those methods, just not the implementation as it modifies the request in a way that may even break the query.

It doesn't break anymore and is a valid optimization that is also utilized in JPA implementations. The forUpdate() case was broken for explicit and implied limit/offset cases, not only for uniqueResult and singleResult.

Member

timowest commented Jan 18, 2013

I agree about the semantics of those methods, just not the implementation as it modifies the request in a way that may even break the query.

It doesn't break anymore and is a valid optimization that is also utilized in JPA implementations. The forUpdate() case was broken for explicit and implied limit/offset cases, not only for uniqueResult and singleResult.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 20, 2013

Member

Released in 3.0.0.BETA2

Member

timowest commented Feb 20, 2013

Released in 3.0.0.BETA2

@timowest timowest closed this Feb 20, 2013

@timowest timowest added this to the 3.0.0 milestone Apr 14, 2014

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