Skip to content

Commit

Permalink
fix concurrency problem on PayPal handler
Browse files Browse the repository at this point in the history
  • Loading branch information
@cbellone
cbellone committed Jan 25, 2020
1 parent 3e51c56 commit dc92b0e
Show file tree
Hide file tree
Showing 8 changed files with 200 additions and 209 deletions.
20 changes: 16 additions & 4 deletions src/main/java/alfio/controller/payment/PayPalCallbackController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public class PayPalCallbackController {
@GetMapping("/confirm")
public String payPalSuccess(@PathVariable("eventName") String eventName,
@PathVariable("reservationId") String reservationId,
@RequestParam(value = "paymentId", required = false) String payPalPaymentId,
@RequestParam(value = "token", required = false) String payPalPaymentId,
@RequestParam(value = "PayerID", required = false) String payPalPayerID,
@RequestParam(value = "hmac") String hmac) {

Expand All @@ -68,15 +68,27 @@ public String payPalSuccess(@PathVariable("eventName") String eventName,
payPalManager.saveToken(res.getId(), ev, token);
return "redirect:/event/" + ev.getShortName() + "/reservation/" +res.getId() + "/overview";
} else {
return payPalCancel(ev.getShortName(), res.getId());
return payPalCancel(ev.getShortName(), res.getId(), payPalPaymentId, hmac);
}
}

@GetMapping("/cancel")
public String payPalCancel(@PathVariable("eventName") String eventName,
@PathVariable("reservationId") String reservationId) {
@PathVariable("reservationId") String reservationId,
@RequestParam(value = "token", required = false) String payPalPaymentId,
@RequestParam(value = "hmac") String hmac) {

payPalManager.removeToken(reservationId);
if(eventRepository.findOptionalByShortName(eventName).isEmpty()) {
return "redirect:/";
}

Optional<TicketReservation> optionalReservation = ticketReservationManager.findById(reservationId);

if(optionalReservation.isEmpty()) {
return "redirect:/event/" + eventName;
}

payPalManager.removeToken(optionalReservation.get(), payPalPaymentId);
return "redirect:/event/"+eventName+"/reservation/"+reservationId+"/overview";
}
}
7 changes: 5 additions & 2 deletions src/main/java/alfio/manager/TicketReservationManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -482,11 +482,14 @@ public PaymentResult performPayment(PaymentSpecification spec,
return PaymentResult.failed("error.STEP2_UNABLE_TO_TRANSITION");
}

TicketReservation reservation = ticketReservationRepository.findReservationById(spec.getReservationId());
TicketReservation reservation = null;

try {
PaymentResult paymentResult;
ticketReservationRepository.lockReservationForUpdate(spec.getReservationId());
reservation = ticketReservationRepository.findReservationByIdForUpdate(spec.getReservationId());
if(reservation.getStatus() == COMPLETE) {
return PaymentResult.successful("");
}
//save billing data in case we have to go back to PENDING
ticketReservationRepository.updateBillingData(spec.getVatStatus(), reservation.getSrcPriceCts(), reservation.getFinalPriceCts(),
reservation.getVatCts(), reservation.getDiscountCts(), reservation.getCurrencyCode(), spec.getVatNr(), spec.getVatCountryCode(), spec.isInvoiceRequested(), spec.getReservationId());
Expand Down
350 changes: 158 additions & 192 deletions src/main/java/alfio/manager/payment/PayPalManager.java
View file

Large diffs are not rendered by default.

3 changes: 1 addition & 2 deletions src/main/java/alfio/manager/payment/PaymentManagerUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,13 @@ static void invalidateExistingTransactions(String reservationId, TransactionRepo
invalidateExistingTransactions(reservationId, transactionRepository, null);
}


static void invalidateExistingTransactions(String reservationId, TransactionRepository transactionRepository, PaymentProxy paymentProxy) {
// temporary, until we allow multiple transactions for a reservation
int invalidated = transactionRepository.invalidateForReservation(reservationId, paymentProxy != null ? paymentProxy.name() : null);
if(invalidated > 0) {
log.debug("invalidated {} existing transactions", invalidated);
}
// assert that there is no active transaction left
Validate.isTrue(transactionRepository.loadOptionalByReservationId(reservationId).isEmpty(), "There is already a transaction registered for reservation %s");
Validate.isTrue(transactionRepository.loadOptionalByReservationId(reservationId).isEmpty(), "There is already a transaction registered for reservation %s", reservationId);
}
}
3 changes: 3 additions & 0 deletions src/main/java/alfio/repository/TicketReservationRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,9 @@ int postponePayment(@Bind("reservationId") String reservationId, @Bind("status")
@Query("select * from tickets_reservation where id = :id")
TicketReservation findReservationById(@Bind("id") String id);

@Query("select * from tickets_reservation where id = :id for update")
TicketReservation findReservationByIdForUpdate(@Bind("id") String id);

@Query("select * from tickets_reservation where id = :id")
Optional<TicketReservation> findOptionalReservationById(@Bind("id") String id);

Expand Down
3 changes: 3 additions & 0 deletions src/main/java/alfio/repository/TransactionRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ int updateIfStatus(@Bind("transactionId") int id,
@Query(SELECT_VALID_BY_RESERVATION_ID)
Optional<Transaction> loadOptionalByReservationId(@Bind("reservationId") String reservationId);

@Query(SELECT_VALID_BY_RESERVATION_ID + " and status = :status")
Optional<Transaction> loadOptionalByReservationIdAndStatus(@Bind("reservationId") String reservationId, @Bind("status") Transaction.Status status);

@Query("select * from b_transaction where id = :id and status = :status")
Optional<Transaction> loadOptionalByIdAndStatus(@Bind("id") int id, @Bind("status") Transaction.Status status);

Expand Down
6 changes: 5 additions & 1 deletion src/main/java/alfio/util/HttpUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,11 @@ private HttpUtils() {
public static final String AUTHORIZATION = "Authorization";

public static boolean callSuccessful(HttpResponse<?> response) {
return response.statusCode() >= 200 && response.statusCode() < 300;
return statusCodeIsSuccessful(response.statusCode());
}

public static boolean statusCodeIsSuccessful(int statusCode) {
return statusCode >= 200 && statusCode < 300;
}

public static HttpResponse<String> postForm(HttpClient httpClient, String url, Map<String, String> params) throws IOException, InterruptedException {
Expand Down
17 changes: 9 additions & 8 deletions src/test/java/alfio/manager/TicketReservationManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ void init() {
when(ticketReservation.getVatCts()).thenReturn(0);
when(ticketReservation.getDiscountCts()).thenReturn(0);
when(ticketReservation.getCurrencyCode()).thenReturn(EVENT_CURRENCY);
when(ticketReservationRepository.findReservationByIdForUpdate(RESERVATION_ID)).thenReturn(ticketReservation);
when(ticketReservationRepository.findReservationById(RESERVATION_ID)).thenReturn(ticketReservation);
when(ticketReservationRepository.getAdditionalInfo(any())).thenReturn(mock(TicketReservationAdditionalInfo.class));
organization = new Organization(ORGANIZATION_ID, "org", "desc", ORG_EMAIL);
Expand Down Expand Up @@ -243,7 +244,7 @@ void init() {
when(configurationManager.getForSystem(ConfigurationKeys.BASE_URL)).thenReturn(baseUrlConf);
when(configurationManager.getFor(eq(ConfigurationKeys.BASE_URL), any())).thenReturn(baseUrlConf);
when(configurationManager.hasAllConfigurationsForInvoice(eq(event))).thenReturn(false);
when(ticketReservationRepository.findReservationById(RESERVATION_ID)).thenReturn(ticketReservation);
when(ticketReservationRepository.findReservationByIdForUpdate(RESERVATION_ID)).thenReturn(ticketReservation);
when(ticket.getId()).thenReturn(TICKET_ID);
when(ticket.getSrcPriceCts()).thenReturn(10);
when(ticketCategory.getId()).thenReturn(TICKET_CATEGORY_ID);
Expand Down Expand Up @@ -846,7 +847,7 @@ private void testPaidReservation(boolean enableTicketTransfer, boolean expectSuc
if(expectSuccess) {
assertTrue(result.isSuccessful());
assertEquals(Optional.of(TRANSACTION_ID), result.getGatewayId());
verify(ticketReservationRepository).lockReservationForUpdate(eq(RESERVATION_ID));
verify(ticketReservationRepository).findReservationByIdForUpdate(eq(RESERVATION_ID));
verify(ticketReservationRepository, atLeastOnce()).findReservationById(RESERVATION_ID);
verify(ticketReservationRepository).updateBillingData(eq(PriceContainer.VatStatus.INCLUDED), eq(100), eq(100), eq(0), eq(0), eq(EVENT_CURRENCY), eq("123456"), eq("IT"), eq(true), eq(RESERVATION_ID));

Expand Down Expand Up @@ -885,7 +886,7 @@ void returnFailureCodeIfPaymentNotSuccessful() {
assertFalse(result.getGatewayId().isPresent());
assertEquals(Optional.of("error-code"), result.getErrorCode());
verify(ticketReservationRepository).updateTicketReservation(eq(RESERVATION_ID), eq(TicketReservationStatus.IN_PAYMENT.toString()), anyString(), anyString(), isNull(), isNull(), anyString(), isNull(), isNull(), eq(PaymentProxy.STRIPE.toString()), isNull());
verify(ticketReservationRepository).lockReservationForUpdate(eq(RESERVATION_ID));
verify(ticketReservationRepository).findReservationByIdForUpdate(eq(RESERVATION_ID));
verify(ticketReservationRepository).updateReservationStatus(eq(RESERVATION_ID), eq(TicketReservationStatus.PENDING.toString()));
verify(configurationManager, never()).hasAllConfigurationsForInvoice(eq(event));
verify(ticketReservationRepository).updateBillingData(eq(PriceContainer.VatStatus.INCLUDED), eq(100), eq(100), eq(0), eq(0), eq(EVENT_CURRENCY), eq("12345"), eq("IT"), eq(true), eq(RESERVATION_ID));
Expand Down Expand Up @@ -915,7 +916,7 @@ void handleOnSitePaymentMethod() {
assertTrue(result.isSuccessful());
assertEquals(Optional.of(TicketReservationManager.NOT_YET_PAID_TRANSACTION_ID), result.getGatewayId());
verify(ticketReservationRepository).updateTicketReservation(eq(RESERVATION_ID), eq(TicketReservationStatus.COMPLETE.toString()), anyString(), anyString(), isNull(), isNull(), anyString(), anyString(), any(), eq(PaymentProxy.ON_SITE.toString()), isNull());
verify(ticketReservationRepository).lockReservationForUpdate(eq(RESERVATION_ID));
verify(ticketReservationRepository).findReservationByIdForUpdate(eq(RESERVATION_ID));
verify(ticketRepository).updateTicketsStatusWithReservationId(eq(RESERVATION_ID), eq(TicketStatus.TO_BE_PAID.toString()));
verify(specialPriceRepository).updateStatusForReservation(eq(singletonList(RESERVATION_ID)), eq(SpecialPrice.Status.TAKEN.toString()));
verify(waitingQueueManager).fireReservationConfirmed(eq(RESERVATION_ID));
Expand Down Expand Up @@ -943,7 +944,7 @@ void handleOfflinePaymentMethod() {
assertTrue(result.isSuccessful());
assertEquals(Optional.of(TicketReservationManager.NOT_YET_PAID_TRANSACTION_ID), result.getGatewayId());
verify(waitingQueueManager, never()).fireReservationConfirmed(eq(RESERVATION_ID));
verify(ticketReservationRepository).lockReservationForUpdate(eq(RESERVATION_ID));
verify(ticketReservationRepository).findReservationByIdForUpdate(eq(RESERVATION_ID));
verify(configurationManager).hasAllConfigurationsForInvoice(eq(event));
verify(ticketReservationRepository).updateBillingData(eq(PriceContainer.VatStatus.INCLUDED), eq(100), eq(100), eq(0), eq(0), eq(EVENT_CURRENCY), eq("123456"), eq("IT"), eq(true), eq(RESERVATION_ID));
}
Expand All @@ -965,7 +966,7 @@ void confirmOfflinePayments() {
TicketReservation copy = copy(reservation);
Event event = copy(this.event);
when(ticketReservationRepository.findOptionalReservationById(eq(RESERVATION_ID))).thenReturn(Optional.of(copy));
when(ticketReservationRepository.findReservationById(eq(RESERVATION_ID))).thenReturn(copy);
when(ticketReservationRepository.findReservationByIdForUpdate(eq(RESERVATION_ID))).thenReturn(copy);
when(ticketRepository.updateTicketsStatusWithReservationId(eq(RESERVATION_ID), eq(TicketStatus.ACQUIRED.toString()))).thenReturn(1);
when(ticketReservationRepository.updateTicketReservation(eq(RESERVATION_ID), eq(COMPLETE.toString()), anyString(), anyString(), isNull(), isNull(), anyString(), isNull(), any(), eq(PaymentProxy.OFFLINE.toString()), isNull())).thenReturn(1);
when(configurationManager.getFor(eq(VAT_NR), any())).thenReturn(new ConfigurationManager.MaybeConfiguration(VAT_NR, new ConfigurationKeyValuePathLevel(null, "vatnr", null)));
Expand Down Expand Up @@ -1146,7 +1147,7 @@ void doNotSendReminderIfPreviousNotifications() {
int ticketId = 2;
when(ticket.getId()).thenReturn(ticketId);
when(ticketRepository.findAllAssignedButNotYetNotifiedForUpdate(EVENT_ID)).thenReturn(singletonList(ticket));
when(ticketReservationRepository.findReservationById(eq(RESERVATION_ID))).thenReturn(ticketReservation);
when(ticketReservationRepository.findReservationByIdForUpdate(eq(RESERVATION_ID))).thenReturn(ticketReservation);

when(eventRepository.findByReservationId(RESERVATION_ID)).thenReturn(event);
when(event.getZoneId()).thenReturn(ZoneId.systemDefault());
Expand All @@ -1173,7 +1174,7 @@ void doNotSendReminderIfTicketHasAlreadyBeenModified() {
int ticketId = 2;
when(ticket.getId()).thenReturn(ticketId);
when(ticketRepository.findAllAssignedButNotYetNotifiedForUpdate(EVENT_ID)).thenReturn(singletonList(ticket));
when(ticketReservationRepository.findReservationById(eq(RESERVATION_ID))).thenReturn(ticketReservation);
when(ticketReservationRepository.findReservationByIdForUpdate(eq(RESERVATION_ID))).thenReturn(ticketReservation);

when(eventRepository.findByReservationId(RESERVATION_ID)).thenReturn(event);
when(event.getZoneId()).thenReturn(ZoneId.systemDefault());
Expand Down

0 comments on commit dc92b0e

Please sign in to comment.