fix(personal): make delete requests idempotent
This commit is contained in:
+13
-2
@@ -73,16 +73,27 @@ public class PersonalCleanupService {
|
|||||||
and o.create_by = i.owner_user_id
|
and o.create_by = i.owner_user_id
|
||||||
and cast(json_unquote(json_extract(o.ext1, '$.itemId')) as unsigned) = i.id
|
and cast(json_unquote(json_extract(o.ext1, '$.itemId')) as unsigned) = i.id
|
||||||
where binary i.tenant_id = binary ? and i.owner_user_id = ? and i.id = ?
|
where binary i.tenant_id = binary ? and i.owner_user_id = ? and i.id = ?
|
||||||
and i.status in ('QUEUED','PARSING','READY','FAILED')
|
and i.status in ('QUEUED','PARSING','READY','FAILED','DELETING')
|
||||||
for update
|
for update
|
||||||
""", owner.tenantId(), owner.userId(), itemId);
|
""", owner.tenantId(), owner.userId(), itemId);
|
||||||
} catch (EmptyResultDataAccessException ex) {
|
} catch (EmptyResultDataAccessException ex) {
|
||||||
throw new ServiceException(ITEM_NOT_FOUND);
|
throw new ServiceException(ITEM_NOT_FOUND);
|
||||||
}
|
}
|
||||||
|
String status = String.valueOf(item.get("status"));
|
||||||
|
if ("DELETING".equals(status)) {
|
||||||
|
List<Map<String, Object>> jobs = jdbc.queryForList("""
|
||||||
|
select id from aihr_personal_cleanup_job
|
||||||
|
where tenant_id = ? and owner_user_id = ? and item_id = ?
|
||||||
|
and status in ('PENDING','RETRY')
|
||||||
|
order by id limit 1
|
||||||
|
for update
|
||||||
|
""", owner.tenantId(), owner.userId(), itemId);
|
||||||
|
if (jobs.size() != 1) throw new ServiceException("PERSONAL_CLEANUP_STATE_INVALID");
|
||||||
|
return number(jobs.get(0), "id");
|
||||||
|
}
|
||||||
if (item.get("oss_id") != null && item.get("owned_oss_id") == null) {
|
if (item.get("oss_id") != null && item.get("owned_oss_id") == null) {
|
||||||
throw new ServiceException(ITEM_NOT_FOUND);
|
throw new ServiceException(ITEM_NOT_FOUND);
|
||||||
}
|
}
|
||||||
String status = String.valueOf(item.get("status"));
|
|
||||||
int hidden = jdbc.update("""
|
int hidden = jdbc.update("""
|
||||||
update aihr_personal_item
|
update aihr_personal_item
|
||||||
set status = 'DELETING', update_time = now()
|
set status = 'DELETING', update_time = now()
|
||||||
|
|||||||
+43
@@ -23,8 +23,10 @@ import static org.mockito.Mockito.inOrder;
|
|||||||
import static org.mockito.Mockito.mock;
|
import static org.mockito.Mockito.mock;
|
||||||
import static org.mockito.Mockito.never;
|
import static org.mockito.Mockito.never;
|
||||||
import static org.mockito.Mockito.verify;
|
import static org.mockito.Mockito.verify;
|
||||||
|
import static org.mockito.Mockito.verifyNoInteractions;
|
||||||
import static org.mockito.Mockito.when;
|
import static org.mockito.Mockito.when;
|
||||||
import static org.mockito.Mockito.doThrow;
|
import static org.mockito.Mockito.doThrow;
|
||||||
|
import static org.mockito.Mockito.times;
|
||||||
|
|
||||||
@Tag("dev")
|
@Tag("dev")
|
||||||
class PersonalCleanupServiceTest {
|
class PersonalCleanupServiceTest {
|
||||||
@@ -71,6 +73,47 @@ class PersonalCleanupServiceTest {
|
|||||||
verify(jdbc, never()).update(contains("insert into aihr_personal_cleanup_job"), any(), any(), any(), any());
|
verify(jdbc, never()).update(contains("insert into aihr_personal_cleanup_job"), any(), any(), any(), any());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void repeatedDeleteReturnsExistingOwnerJobWithoutDuplicateInsertOrCleanup() {
|
||||||
|
JdbcTemplate jdbc = mock(JdbcTemplate.class);
|
||||||
|
PersonalVectorStore vectors = mock(PersonalVectorStore.class);
|
||||||
|
PersonalCleanupService.OssCleanup oss = mock(PersonalCleanupService.OssCleanup.class);
|
||||||
|
when(jdbc.queryForMap(contains("for update"), eq("000000"), eq(101L), eq(9L)))
|
||||||
|
.thenReturn(item("READY"), item("DELETING"));
|
||||||
|
when(jdbc.update(contains("set status = 'DELETING'"), eq("000000"), eq(101L), eq(9L), eq("READY")))
|
||||||
|
.thenReturn(1);
|
||||||
|
when(jdbc.update(contains("insert into aihr_personal_cleanup_job"), any(), eq("000000"), eq(101L), eq(9L)))
|
||||||
|
.thenReturn(1);
|
||||||
|
when(jdbc.queryForList(contains("from aihr_personal_cleanup_job"), eq("000000"), eq(101L), eq(9L)))
|
||||||
|
.thenReturn(List.of(Map.of("id", 7001L)));
|
||||||
|
PersonalCleanupService service = PersonalCleanupService.forTest(jdbc, vectors, oss, () -> 7001L,
|
||||||
|
action -> action.get());
|
||||||
|
|
||||||
|
assertEquals(7001L, service.requestDelete(OWNER, 9L));
|
||||||
|
assertEquals(7001L, service.requestDelete(OWNER, 9L));
|
||||||
|
|
||||||
|
verify(jdbc, times(1)).update(contains("insert into aihr_personal_cleanup_job"), eq(7001L),
|
||||||
|
eq("000000"), eq(101L), eq(9L));
|
||||||
|
verify(jdbc).queryForList(contains("from aihr_personal_cleanup_job"), eq("000000"), eq(101L), eq(9L));
|
||||||
|
verifyNoInteractions(vectors, oss);
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void deletingItemWithoutOwnerJobFailsWithStableRecoveryError() {
|
||||||
|
JdbcTemplate jdbc = mock(JdbcTemplate.class);
|
||||||
|
when(jdbc.queryForMap(contains("for update"), eq("000000"), eq(101L), eq(9L)))
|
||||||
|
.thenReturn(item("DELETING"));
|
||||||
|
when(jdbc.queryForList(contains("from aihr_personal_cleanup_job"), eq("000000"), eq(101L), eq(9L)))
|
||||||
|
.thenReturn(List.of());
|
||||||
|
PersonalCleanupService service = PersonalCleanupService.forTest(jdbc, mock(PersonalVectorStore.class),
|
||||||
|
mock(PersonalCleanupService.OssCleanup.class), () -> 7002L, action -> action.get());
|
||||||
|
|
||||||
|
ServiceException error = assertThrows(ServiceException.class, () -> service.requestDelete(OWNER, 9L));
|
||||||
|
|
||||||
|
assertEquals("PERSONAL_CLEANUP_STATE_INVALID", error.getMessage());
|
||||||
|
verify(jdbc, never()).update(contains("insert into aihr_personal_cleanup_job"), any(), any(), any(), any());
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void cleanupUsesFixedOrderAndIsIdempotent() {
|
void cleanupUsesFixedOrderAndIsIdempotent() {
|
||||||
JdbcTemplate jdbc = mock(JdbcTemplate.class);
|
JdbcTemplate jdbc = mock(JdbcTemplate.class);
|
||||||
|
|||||||
Reference in New Issue
Block a user