spring data jdbc: saveAll on null id's entities calls update statement

[chrshnv]

saveAll on null id's entities calls update statement.

List<AppConfiguration> configurationsToSave = List.of(accessTokenConfiguration, refreshTokenConfiguration, expiredAtConfiguration);

configurationsToSave.forEach(it -> log.info(it.getId() != null ? it.getId().toString() : null));

appConfigurationRepository
		.saveAll(List.of(accessTokenConfiguration, refreshTokenConfiguration, expiredAtConfiguration));

logs with saveAll:

2025-05-30T11:30:41.694+05:00  INFO 11461 --- [chats] [nio-8080-exec-1] r.t.o.c.service.impl.BitrixTokenService  : null
2025-05-30T11:30:41.694+05:00  INFO 11461 --- [chats] [nio-8080-exec-1] r.t.o.c.service.impl.BitrixTokenService  : null
2025-05-30T11:30:41.694+05:00  INFO 11461 --- [chats] [nio-8080-exec-1] r.t.o.c.service.impl.BitrixTokenService  : null
2025-05-30T11:30:41.724+05:00 ERROR 11461 --- [chats] [nio-8080-exec-1] o.a.c.c.C.[.[.[/].[dispatcherServlet]    : Servlet.service() for servlet [dispatcherServlet] in context with path [] threw exception [Request processing failed: org.springframework.data.relational.core.conversion.DbActionExecutionException: Failed to execute DbAction.UpdateRoot(entity=ru.trek.ozon.chats.domain.AppConfiguration@658327e7)] with root cause

org.springframework.dao.IncorrectUpdateSemanticsDataAccessException: Failed to update entity [ru.trek.ozon.chats.domain.AppConfiguration@658327e7]; Id [2f0a0c62-1a97-4ca4-a8ba-2c4b501e2459] not found in database```

[mipo256]

Thanks for reaching out @chrshnv

Please, provide minimal reproducible example. It is very hard to deduce the preconditions in your example.

[chrshnv]

https://github.com/chrshnv/save-all-data-jdbc-bug

[mipo256]

Thank you, @chrshnv.

I think the issue is that you're using BeforeConvertCallback and not BeforeSaveCallback for setting the id. The BeforeConvertCallback is invoked too early. Because of that, the id is assigned and the IsNewStrategy makes a decision that your entity is not new, and therefore the SQL UPDATE statement is issued.

Please, change the callbacks, the problem should go away.

[chrshnv]

ok, then why when I use "save" instead of "saveAll" - it inserts instead of update? you can try on 'save' branch in MRE repo

[mipo256]

@chrshnv I'm sorry, I have misled you, the BeforeConvertCallback is indeed the correct one, my apologies.

And I think I even know where the problem is, it is right here:

	@Override
	public <T> List<T> saveAll(Iterable<T> instances) {
		return doInBatch(instances, (first) -> (second -> changeCreatorSelectorForSave(first).apply(second)));
	}

We apply the change creator on the first and not the second argument. I'll provide the PR.

Thank you, @chrshnv

[mipo256]

After taking a closer look at it.

Indeed, when we have the BeforeConvertCallback with id generation logic within, the in case of:

1. `save(T) --> INSERT is triggered, as it should and documented in the Jens's article.
2. saveAll(Iterable<T>) --> UPDATE is triggered, and it is not correct

Now, the reason why it actually happens is that when we do just save(T), we first, determine the RootAggregateChange, and only then call the BeforeConvertCallback, BeforeSaveCallback etc.

In case of saveAll(Iterable<T>), it turns out, that we call the BeforeConvertCallback before we infer the RootAggregateChange, which leads to Update action, and the incorrect update sematics error.

I think the best approach here would be to align the saveAll(Iterable<T>) with save(T), meaning, that the BeforeConvertCallback callback should be invoked after the RootAggregateChange is assembled.

What are your thoughts on this, Jens @schauder, Mark @mp911de?

[underbell]

It seems to be the same issue that I encountered. I have created an MRE for reporting the issue. Please refer to it.
https://github.com/underbell/jdbc-audit-test-boot3.5.0

[mp911de]

I consider this a bug and we should align saving for both cases.