SQLMergeClause.executeCompositeMerge() tries to insert when a conflict exists #940

Closed
cowwoc opened this Issue Sep 19, 2014 · 10 comments

Comments

Projects
None yet
2 participants
@cowwoc
Contributor

cowwoc commented Sep 19, 2014

executeCompositeMerge() invokes getIds() which attempts to detect whether the row being merged already exists or not. The problem is that getIds() adds all columns to the WHERE clause.

If the list of keys is different from the list of columns (meaning, we are merging 4 columns but only 2 of them are keys) then getIds() always returns an empty list and we proceed to insert which fails.

Expected behavior: getIds() should only add keys() to the WHERE clause. In fact, it looks like you never actually use the return value except to check if the list was empty so it probably makes more sense to: SELECT * FROM table WHERE EXISTS(key1, key2) and return a boolean.

@cowwoc cowwoc changed the title from SQLMergeClause.executeCompositeMerge() fails to detect conflicts to SQLMergeClause.executeCompositeMerge() tries to insert when a conflict exists Sep 19, 2014

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Sep 19, 2014

Contributor

It further looks like the code fails to take composite keys into considerations. Any time you see keys.get(0) it fails to take the remaining keys into consideration (this causes it to update rows it should not). This is a critical bug in my view, because it can lead to data loss.

Contributor

cowwoc commented Sep 19, 2014

It further looks like the code fails to take composite keys into considerations. Any time you see keys.get(0) it fails to take the remaining keys into consideration (this causes it to update rows it should not). This is a critical bug in my view, because it can lead to data loss.

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Sep 19, 2014

Contributor

I've having problems building QueryDSL. I get javadoc build errors under JDK 8. Does the build require JDK 7?

Contributor

cowwoc commented Sep 19, 2014

I've having problems building QueryDSL. I get javadoc build errors under JDK 8. Does the build require JDK 7?

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Sep 19, 2014

Contributor

Take a look at cowwoc@fe878d9 and let me know what you think. I believe it'll solve the problem but I cannot test it locally because:

  1. The root project fails to build due to a Javadoc error under JDK 8.
  2. The SQL project fails to build because maven.cubrid.org returns HTTP 503 (maybe it's down for maintenance)

I'll check back tomorrow.

Contributor

cowwoc commented Sep 19, 2014

Take a look at cowwoc@fe878d9 and let me know what you think. I believe it'll solve the problem but I cannot test it locally because:

  1. The root project fails to build due to a Javadoc error under JDK 8.
  2. The SQL project fails to build because maven.cubrid.org returns HTTP 503 (maybe it's down for maintenance)

I'll check back tomorrow.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Sep 21, 2014

Member

@cowwoc Thanks again for your input. I added a PR. Could you take a look?

Member

timowest commented Sep 21, 2014

@cowwoc Thanks again for your input. I added a PR. Could you take a look?

@timowest timowest added this to the 3.5.0 milestone Sep 21, 2014

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Sep 22, 2014

Contributor

Looks good by visual inspection. I can't test the actual code until I get the build working on my end.

Contributor

cowwoc commented Sep 22, 2014

Looks good by visual inspection. I can't test the actual code until I get the build working on my end.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Sep 27, 2014

Member

@cowwoc Did you now have a change to look at the PR?

Member

timowest commented Sep 27, 2014

@cowwoc Did you now have a change to look at the PR?

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Sep 29, 2014

Member

Tomorrow is the planned release date for 3.5.0. I can include the fix if someone could comment on the PR.

Member

timowest commented Sep 29, 2014

Tomorrow is the planned release date for 3.5.0. I can include the fix if someone could comment on the PR.

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Sep 29, 2014

Contributor

@timowest I plan on trying this PR later on today.

Contributor

cowwoc commented Sep 29, 2014

@timowest I plan on trying this PR later on today.

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Sep 29, 2014

Contributor

@timowest Tested. This PR works for me.

Contributor

cowwoc commented Sep 29, 2014

@timowest Tested. This PR works for me.

@Shredder121 Shredder121 closed this in #950 Sep 30, 2014

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Sep 30, 2014

Member

Released in 3.5.0

Member

timowest commented Sep 30, 2014

Released in 3.5.0

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