From 7201d91f8412958a019fe003f663263acc44700a Mon Sep 17 00:00:00 2001 From: Sylvain Jermini Date: Wed, 9 Jan 2019 13:06:59 +0100 Subject: [PATCH 1/2] use nested transaction and save point, part 1/n --- .../manager/AdminReservationManager.java | 8 +- .../AdminReservationRequestManager.java | 7 +- .../alfio/manager/NotificationManager.java | 5 +- .../manager/TicketReservationManager.java | 77 +++++++++++-------- 4 files changed, 57 insertions(+), 40 deletions(-) diff --git a/src/main/java/alfio/manager/AdminReservationManager.java b/src/main/java/alfio/manager/AdminReservationManager.java index 223b4ab6e7..617dcb715c 100644 --- a/src/main/java/alfio/manager/AdminReservationManager.java +++ b/src/main/java/alfio/manager/AdminReservationManager.java @@ -52,6 +52,7 @@ import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; import org.springframework.stereotype.Component; import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionDefinition; import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.support.DefaultTransactionDefinition; import org.springframework.transaction.support.TransactionTemplate; @@ -160,9 +161,10 @@ public Result updateReservation(String eventName, String reservationId, } public Result>> createReservation(AdminReservationModification input, String eventName, String username) { - DefaultTransactionDefinition definition = new DefaultTransactionDefinition(); + DefaultTransactionDefinition definition = new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_NESTED); TransactionTemplate template = new TransactionTemplate(transactionManager, definition); return template.execute(status -> { + var savepoint = status.createSavepoint(); try { Result>> result = eventRepository.findOptionalByShortNameForUpdate(eventName) .map(e -> validateTickets(input, e)) @@ -170,12 +172,12 @@ public Result>> createReservation(AdminRese .orElse(Result.error(ErrorCode.EventError.NOT_FOUND)); if (!result.isSuccess()) { log.debug("Error during update of reservation eventName: {}, username: {}, reservation: {}", eventName, username, AdminReservationModification.summary(input)); - status.setRollbackOnly(); + status.rollbackToSavepoint(savepoint); } return result; } catch (Exception e) { log.error("Error during update of reservation eventName: {}, username: {}, reservation: {}", eventName, username, AdminReservationModification.summary(input)); - status.setRollbackOnly(); + status.rollbackToSavepoint(savepoint); return Result.error(singletonList(ErrorCode.custom(e instanceof DuplicateReferenceException ? "duplicate-reference" : "", e.getMessage()))); } }); diff --git a/src/main/java/alfio/manager/AdminReservationRequestManager.java b/src/main/java/alfio/manager/AdminReservationRequestManager.java index 6fd07b82f0..42c4571a06 100644 --- a/src/main/java/alfio/manager/AdminReservationRequestManager.java +++ b/src/main/java/alfio/manager/AdminReservationRequestManager.java @@ -115,20 +115,21 @@ public Pair processPendingReservations() { } private Result, Event>> processReservation(AdminReservationRequest request, Pair p) { - DefaultTransactionDefinition definition = new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_REQUIRES_NEW); + DefaultTransactionDefinition definition = new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_NESTED); TransactionTemplate template = new TransactionTemplate(transactionManager, definition); return template.execute(status -> { + var savepoint = status.createSavepoint(); try { String eventName = p.getLeft().getShortName(); String username = p.getRight().getUsername(); Result, Event>> result = adminReservationManager.createReservation(request.getBody(), eventName, username) .flatMap(r -> adminReservationManager.confirmReservation(eventName, r.getLeft().getId(), username)); if(!result.isSuccess()) { - status.setRollbackOnly(); + status.rollbackToSavepoint(savepoint); } return result; } catch(Exception ex) { - status.setRollbackOnly(); + status.rollbackToSavepoint(savepoint); return Result.error(singletonList(ErrorCode.custom("", ex.getMessage()))); } }); diff --git a/src/main/java/alfio/manager/NotificationManager.java b/src/main/java/alfio/manager/NotificationManager.java index c7e2eb4b2d..94557cd5a8 100644 --- a/src/main/java/alfio/manager/NotificationManager.java +++ b/src/main/java/alfio/manager/NotificationManager.java @@ -43,7 +43,9 @@ import org.springframework.security.crypto.codec.Hex; import org.springframework.stereotype.Component; import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionDefinition; import org.springframework.transaction.annotation.Transactional; +import org.springframework.transaction.support.DefaultTransactionDefinition; import org.springframework.transaction.support.TransactionTemplate; import java.io.ByteArrayOutputStream; @@ -100,7 +102,8 @@ public NotificationManager(Mailer mailer, this.emailMessageRepository = emailMessageRepository; this.eventRepository = eventRepository; this.organizationRepository = organizationRepository; - this.tx = new TransactionTemplate(transactionManager); + DefaultTransactionDefinition definition = new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_NESTED); + this.tx = new TransactionTemplate(transactionManager, definition); this.configurationManager = configurationManager; GsonBuilder builder = new GsonBuilder(); builder.registerTypeAdapter(Mailer.Attachment.class, new AttachmentConverter()); diff --git a/src/main/java/alfio/manager/TicketReservationManager.java b/src/main/java/alfio/manager/TicketReservationManager.java index e5aaafe696..f127a7bab1 100644 --- a/src/main/java/alfio/manager/TicketReservationManager.java +++ b/src/main/java/alfio/manager/TicketReservationManager.java @@ -117,6 +117,7 @@ public class TicketReservationManager { private final TemplateManager templateManager; private final TransactionTemplate requiresNewTransactionTemplate; private final TransactionTemplate serializedTransactionTemplate; + private final TransactionTemplate nestedTransactionTemplate; private final WaitingQueueManager waitingQueueManager; private final TicketFieldRepository ticketFieldRepository; private final AdditionalServiceRepository additionalServiceRepository; @@ -189,6 +190,7 @@ public TicketReservationManager(EventRepository eventRepository, DefaultTransactionDefinition serialized = new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_REQUIRES_NEW); serialized.setIsolationLevel(TransactionDefinition.ISOLATION_SERIALIZABLE); this.serializedTransactionTemplate = new TransactionTemplate(transactionManager, serialized); + this.nestedTransactionTemplate = new TransactionTemplate(transactionManager, new DefaultTransactionDefinition((TransactionDefinition.PROPAGATION_NESTED))); this.ticketFieldRepository = ticketFieldRepository; this.additionalServiceRepository = additionalServiceRepository; this.additionalServiceItemRepository = additionalServiceItemRepository; @@ -905,13 +907,16 @@ public void cleanupExpiredOfflineReservations(Date expirationDate) { private void cleanupOfflinePayment(String reservationId) { try { - Event event = eventRepository.findByReservationId(reservationId); - boolean enabled = configurationManager.getBooleanConfigValue(Configuration.from(event.getOrganizationId(), event.getId(), AUTOMATIC_REMOVAL_EXPIRED_OFFLINE_PAYMENT), true); - if(enabled) { - deleteOfflinePayment(event, reservationId, true, false, null); - } else { - log.trace("Will not cleanup reservation with id {} because the automatic removal has been disabled", reservationId); - } + nestedTransactionTemplate.execute((tc) -> { + Event event = eventRepository.findByReservationId(reservationId); + boolean enabled = configurationManager.getBooleanConfigValue(Configuration.from(event.getOrganizationId(), event.getId(), AUTOMATIC_REMOVAL_EXPIRED_OFFLINE_PAYMENT), true); + if (enabled) { + deleteOfflinePayment(event, reservationId, true, false, null); + } else { + log.trace("Will not cleanup reservation with id {} because the automatic removal has been disabled", reservationId); + } + return null; + }); } catch (Exception e) { log.error("error during reservation cleanup (id "+reservationId+")", e); } @@ -1446,19 +1451,22 @@ public void sendReminderForOptionalData() { } private void sendOptionalDataReminder(Pair> eventAndTickets) { - Event event = eventAndTickets.getLeft(); - int daysBeforeStart = configurationManager.getIntConfigValue(Configuration.from(event.getOrganizationId(), event.getId(), ConfigurationKeys.ASSIGNMENT_REMINDER_START), 10); - List tickets = eventAndTickets.getRight().stream().filter(t -> !ticketFieldRepository.hasOptionalData(t.getId())).collect(toList()); - Set notYetNotifiedReservations = tickets.stream().map(Ticket::getTicketsReservationId).distinct().filter(rid -> findByIdForNotification(rid, event.getZoneId(), daysBeforeStart).isPresent()).collect(toSet()); - tickets.stream() - .filter(t -> notYetNotifiedReservations.contains(t.getTicketsReservationId())) - .forEach(t -> { - int result = ticketRepository.flagTicketAsReminderSent(t.getId()); - Validate.isTrue(result == 1); - Map model = TemplateResource.prepareModelForReminderTicketAdditionalInfo(organizationRepository.getById(event.getOrganizationId()), event, t, ticketUpdateUrl(event, t.getUuid())); - Locale locale = Optional.ofNullable(t.getUserLanguage()).map(Locale::forLanguageTag).orElseGet(() -> findReservationLanguage(t.getTicketsReservationId())); - notificationManager.sendSimpleEmail(event, t.getEmail(), messageSource.getMessage("reminder.ticket-additional-info.subject", new Object[]{event.getDisplayName()}, locale), () -> templateManager.renderTemplate(event, TemplateResource.REMINDER_TICKET_ADDITIONAL_INFO, model, locale)); - }); + nestedTransactionTemplate.execute(ts -> { + Event event = eventAndTickets.getLeft(); + int daysBeforeStart = configurationManager.getIntConfigValue(Configuration.from(event.getOrganizationId(), event.getId(), ConfigurationKeys.ASSIGNMENT_REMINDER_START), 10); + List tickets = eventAndTickets.getRight().stream().filter(t -> !ticketFieldRepository.hasOptionalData(t.getId())).collect(toList()); + Set notYetNotifiedReservations = tickets.stream().map(Ticket::getTicketsReservationId).distinct().filter(rid -> findByIdForNotification(rid, event.getZoneId(), daysBeforeStart).isPresent()).collect(toSet()); + tickets.stream() + .filter(t -> notYetNotifiedReservations.contains(t.getTicketsReservationId())) + .forEach(t -> { + int result = ticketRepository.flagTicketAsReminderSent(t.getId()); + Validate.isTrue(result == 1); + Map model = TemplateResource.prepareModelForReminderTicketAdditionalInfo(organizationRepository.getById(event.getOrganizationId()), event, t, ticketUpdateUrl(event, t.getUuid())); + Locale locale = Optional.ofNullable(t.getUserLanguage()).map(Locale::forLanguageTag).orElseGet(() -> findReservationLanguage(t.getTicketsReservationId())); + notificationManager.sendSimpleEmail(event, t.getEmail(), messageSource.getMessage("reminder.ticket-additional-info.subject", new Object[]{event.getDisplayName()}, locale), () -> templateManager.renderTemplate(event, TemplateResource.REMINDER_TICKET_ADDITIONAL_INFO, model, locale)); + }); + return null; + }); } Stream getNotifiableEventsStream() { @@ -1473,19 +1481,22 @@ Stream getNotifiableEventsStream() { private void sendAssignmentReminder(Pair> p) { try { - Event event = p.getLeft(); - ZoneId eventZoneId = event.getZoneId(); - int quietPeriod = configurationManager.getIntConfigValue(Configuration.from(event.getOrganizationId(), event.getId(), ConfigurationKeys.ASSIGNMENT_REMINDER_INTERVAL), 3); - p.getRight().stream() - .map(id -> findByIdForNotification(id, eventZoneId, quietPeriod)) - .filter(Optional::isPresent) - .map(Optional::get) - .forEach(reservation -> { - Map model = prepareModelForReservationEmail(event, reservation); - ticketReservationRepository.updateLatestReminderTimestamp(reservation.getId(), ZonedDateTime.now(eventZoneId)); - Locale locale = findReservationLanguage(reservation.getId()); - notificationManager.sendSimpleEmail(event, reservation.getEmail(), messageSource.getMessage("reminder.ticket-not-assigned.subject", new Object[]{event.getDisplayName()}, locale), () -> templateManager.renderTemplate(event, TemplateResource.REMINDER_TICKETS_ASSIGNMENT_EMAIL, model, locale)); - }); + nestedTransactionTemplate.execute(ts -> { + Event event = p.getLeft(); + ZoneId eventZoneId = event.getZoneId(); + int quietPeriod = configurationManager.getIntConfigValue(Configuration.from(event.getOrganizationId(), event.getId(), ConfigurationKeys.ASSIGNMENT_REMINDER_INTERVAL), 3); + p.getRight().stream() + .map(id -> findByIdForNotification(id, eventZoneId, quietPeriod)) + .filter(Optional::isPresent) + .map(Optional::get) + .forEach(reservation -> { + Map model = prepareModelForReservationEmail(event, reservation); + ticketReservationRepository.updateLatestReminderTimestamp(reservation.getId(), ZonedDateTime.now(eventZoneId)); + Locale locale = findReservationLanguage(reservation.getId()); + notificationManager.sendSimpleEmail(event, reservation.getEmail(), messageSource.getMessage("reminder.ticket-not-assigned.subject", new Object[]{event.getDisplayName()}, locale), () -> templateManager.renderTemplate(event, TemplateResource.REMINDER_TICKETS_ASSIGNMENT_EMAIL, model, locale)); + }); + return null; + }); } catch (Exception ex) { log.warn("cannot send reminder message", ex); } From 57f70773d869f0da5c39ff492a76831cd469aa6e Mon Sep 17 00:00:00 2001 From: Sylvain Jermini Date: Wed, 9 Jan 2019 13:38:19 +0100 Subject: [PATCH 2/2] add tests --- ...cketReservationManagerIntegrationTest.java | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/src/test/java/alfio/manager/TicketReservationManagerIntegrationTest.java b/src/test/java/alfio/manager/TicketReservationManagerIntegrationTest.java index 773417f5f5..6e46140cb2 100644 --- a/src/test/java/alfio/manager/TicketReservationManagerIntegrationTest.java +++ b/src/test/java/alfio/manager/TicketReservationManagerIntegrationTest.java @@ -36,6 +36,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; @@ -46,6 +47,7 @@ import java.time.LocalTime; import java.time.ZonedDateTime; import java.util.*; +import java.util.function.Supplier; import java.util.stream.Collectors; import static alfio.test.util.IntegrationTestUtil.AVAILABLE_SEATS; @@ -88,6 +90,9 @@ public class TicketReservationManagerIntegrationTest extends BaseIntegrationTest @Autowired private AdditionalServiceRepository additionalServiceRepository; + @Autowired + private NamedParameterJdbcTemplate jdbcTemplate; + @Before public void ensureConfiguration() { IntegrationTestUtil.ensureMinimalConfiguration(configurationRepository); @@ -387,4 +392,94 @@ public void testDeletePendingPaymentUnboundedCategory() { result = ticketReservationManager.performPayment(specification2, reservationCost, Optional.empty(), Optional.of(PaymentProxy.OFFLINE)); assertTrue(result.isSuccessful()); } + + + @Test + public void testCleanupExpiredReservations() { + List categories = Arrays.asList( + new TicketCategoryModification(null, "default", 10, + new DateTimeModification(LocalDate.now(), LocalTime.now()), + new DateTimeModification(LocalDate.now(), LocalTime.now()), + DESCRIPTION, BigDecimal.TEN, false, "", true, null, null, null, null, null)); + Pair eventAndUsername = initEvent(categories, organizationRepository, userManager, eventManager, eventRepository); + Event event = eventAndUsername.getKey(); + + TicketCategory bounded = ticketCategoryRepository.findByEventId(event.getId()).stream().filter(TicketCategory::isBounded).findFirst().orElseThrow(IllegalStateException::new); + + + TicketReservationModification tr = new TicketReservationModification(); + tr.setAmount(10); + tr.setTicketCategoryId(bounded.getId()); + + TicketReservationWithOptionalCodeModification mod = new TicketReservationWithOptionalCodeModification(tr, Optional.empty()); + + + + Date now = new Date(); + + final Supplier> idsPendingQuery = () -> jdbcTemplate.queryForList("select id from tickets_reservation where validity < :date and status = 'PENDING'", Collections.singletonMap("date", now), String.class); + + Assert.assertTrue(idsPendingQuery.get().isEmpty()); + + String reservationId = ticketReservationManager.createTicketReservation(event, Arrays.asList(mod), Collections.emptyList(), DateUtils.addDays(new Date(), -2), Optional.empty(), Optional.empty(), Locale.ENGLISH, false); + + List reservationIdPending = idsPendingQuery.get(); + Assert.assertEquals(1, reservationIdPending.size()); + Assert.assertEquals(reservationId, reservationIdPending.get(0)); + + ticketReservationManager.cleanupExpiredReservations(now); + + Assert.assertTrue(idsPendingQuery.get().isEmpty()); + } + + @Test + public void testCleanupOfflineExpiredReservations() { + List categories = Arrays.asList( + new TicketCategoryModification(null, "default", 10, + new DateTimeModification(LocalDate.now(), LocalTime.now()), + new DateTimeModification(LocalDate.now(), LocalTime.now()), + DESCRIPTION, BigDecimal.TEN, false, "", true, null, null, null, null, null)); + Pair eventAndUsername = initEvent(categories, organizationRepository, userManager, eventManager, eventRepository); + Event event = eventAndUsername.getKey(); + + TicketCategory bounded = ticketCategoryRepository.findByEventId(event.getId()).stream().filter(TicketCategory::isBounded).findFirst().orElseThrow(IllegalStateException::new); + + + TicketReservationModification tr = new TicketReservationModification(); + tr.setAmount(10); + tr.setTicketCategoryId(bounded.getId()); + + TicketReservationWithOptionalCodeModification mod = new TicketReservationWithOptionalCodeModification(tr, Optional.empty()); + + Date past = DateUtils.addDays(new Date(), -2); + Date now = new Date(); + + String reservationId = ticketReservationManager.createTicketReservation(event, Arrays.asList(mod), Collections.emptyList(), past, Optional.empty(), Optional.empty(), Locale.ENGLISH, false); + + final Supplier> idsOfflinePayment = () -> jdbcTemplate.queryForList("select id from tickets_reservation where validity < :date and status = 'OFFLINE_PAYMENT'", Collections.singletonMap("date", now), String.class); + + Assert.assertTrue(idsOfflinePayment.get().isEmpty()); + + TotalPrice reservationCost = ticketReservationManager.totalReservationCostWithVAT(reservationId); + PaymentSpecification specification = new PaymentSpecification(reservationId, null, reservationCost.getPriceWithVAT(), + event, "email@example.com", new CustomerName("full name", "full", "name", event), + "billing address", null, Locale.ENGLISH, true, false, null, "IT", "123456", PriceContainer.VatStatus.INCLUDED, true, false); + PaymentResult result = ticketReservationManager.performPayment(specification, reservationCost, Optional.empty(), Optional.of(PaymentProxy.OFFLINE)); + assertTrue(result.isSuccessful()); + + + // + Assert.assertEquals(1, jdbcTemplate.update("update tickets_reservation set validity = :date where id = :id", Map.of("date", past, "id", reservationId))); + + // + List idsOffline = idsOfflinePayment.get(); + + Assert.assertEquals(1, idsOffline.size()); + Assert.assertEquals(reservationId, idsOffline.get(0)); + + ticketReservationManager.cleanupExpiredOfflineReservations(now); + + Assert.assertTrue(idsOfflinePayment.get().isEmpty()); + + } } \ No newline at end of file