Skip to content
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

GeneratedKeyHolder.getKey() should accept on-duplicate-key result on MySQL [SPR-17283] #21816

Closed
spring-projects-issues opened this issue Sep 18, 2018 · 5 comments
Assignees
Labels
in: data status: declined type: enhancement

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

member sound opened SPR-17283 and commented

There is an issue in spring-jdbc since 2015:

Root cause seems to be using mysql with ON DUPLICATE KEY UPDATE id=LAST_INSERT_ID(id);

If the database row already exists, some columns should only be updated and the correct id of the column should be returned, without an extra SELECT. Therefore the LAST_INSERT_UD function is used.

Problem, as described in the question: "The keyholder is assuming two rows have been modified (even though only one has been) and is incorrectly returning the ID plus the next sequential ID (i.e. the ID plus 1)."

You can reproduce it by creating initial data in mysql, and updating that data as follows:

create mytable (id bigint AUTO INCREMENT, fixed_val varchar, dynamic_val varchar, primary key(id));

INSERT INTO my_table (fixed_val, dynamic_val) VALUES ("fixed1", "val1");
INSERT INTO my_table (fixed_val, dynamic_val) VALUES ("fixed2", "val2");

@Autowired
private JdbcTemplate jdbcTemplate;

private static final String SQL = "INSERT INTO my_table
                (fixed_val, dynamic_val) VALUES (?,?)
                ON DUPLICATE KEY UPDATE id = last_insert_id(id), dynamic_val = VALUES(dynamic_val)";

public long upsert() {
	KeyHolder key = new GeneratedKeyHolder();

	jdbcTemplate.update(con -> {
		final PreparedStatement ps = con.prepareStatement(SQL, Statement.RETURN_GENERATED_KEYS);
		ps.setObject("fixed1", "val3");
		return ps;
	}, key);

	return key.getKey().longValue();
}

upsert();

Exception:

org.springframework.dao.InvalidDataAccessApiUsageException: The getKeys method should only be used when keys for a single row are returned.  The current key list contains keys for multiple rows: [{GENERATED_KEY=2}, {GENERATED_KEY=3}]
	at org.springframework.jdbc.support.GeneratedKeyHolder.getKeys(GeneratedKeyHolder.java:96) ~[spring-jdbc-5.0.9.RELEASE.jar:5.0.9.RELEASE]
	at org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:204) ~[spring-core-5.0.9.RELEASE.jar:5.0.9.RELEASE]
	at org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.invokeJoinpoint(CglibAopProxy.java:746) ~[spring-aop-5.0.9.RELEASE.jar:5.0.9.RELEASE]
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:163) ~[spring-aop-5.0.9.RELEASE.jar:5.0.9.RELEASE]
	at org.springframework.cache.interceptor.CacheInterceptor.lambda$invoke$0(CacheInterceptor.java:53) ~[spring-context-5.0.9.RELEASE.jar:5.0.9.RELEASE]
	at org.springframework.cache.interceptor.CacheAspectSupport.invokeOperation(CacheAspectSupport.java:336) ~[spring-context-5.0.9.RELEASE.jar:5.0.9.RELEASE]
	at org.springframework.cache.interceptor.CacheAspectSupport.execute(CacheAspectSupport.java:391) ~[spring-context-5.0.9.RELEASE.jar:5.0.9.RELEASE]
	at org.springframework.cache.interceptor.CacheAspectSupport.execute(CacheAspectSupport.java:316) ~[spring-context-5.0.9.RELEASE.jar:5.0.9.RELEASE]
	at org.springframework.cache.interceptor.CacheInterceptor.invoke(CacheInterceptor.java:61) ~[spring-context-5.0.9.RELEASE.jar:5.0.9.RELEASE]
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:185) ~[spring-aop-5.0.9.RELEASE.jar:5.0.9.RELEASE]
	at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:688) ~[spring-aop-5.0.9.RELEASE.jar:5.0.9.RELEASE]
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:514) [na:na]
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) [na:na]
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1135) [na:na]
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) [na:na]
	at java.base/java.lang.Thread.run(Thread.java:844) [na:na]

Affects: 5.0.9

Reference URL: https://stackoverflow.com/questions/30107388/mysql-on-duplicate-key-update-with-primary-key-and-unique-key

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Juergen Hoeller commented

There's nothing special that we assume here: JdbcTemplate just extracts all the entries from the generated-keys result set that the JDBC driver returns to it.

Are you asking for GeneratedKeyHolder.getKey() to leniently ignore more than one entry, simply extracting the first key and returning it?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

member sound commented

So this is actually a java jdbc driver issue?

 

Well, if one could be sure that the first returned generated key is always the desired one, that could be an option. But I'd prefer spring encapsulating that logic, if possible. So for {{GeneratedKeyHolder}}, yes that would be a nice improvement.

 

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Juergen Hoeller commented

Indeed, the MySQL JDBC driver simply returns it that way, as far as I can see. On review, I'm not sure whether it's an acceptable compromise to simply ignore further keys automatically; the application code itself is in a better position to make such an assumption there.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 19, 2018

member sound commented

So as the root cause is not in spring itself, I would leave this as a won't fix, as this is a very special case indeed.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 19, 2018

Juergen Hoeller commented

OK, closing this ticket for the time being then. If there is anything we can do in Spring's JdbcTemplate processing, or any safe assumptions we can make there based on JDBC driver indications, feel free to reopen it.

@spring-projects-issues spring-projects-issues added status: declined in: data type: enhancement labels Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data status: declined type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants