Add helper to finalise interrupted validations

[WhyNotHugo]

trafficstars

Find receipts that seem to have failed a previous submission and call revalidate for them.

[npazosmendez]

Buenas! Cómo te imaginabas la implementación de esto? Una que se me ocurre es agregarle un tercer "estado" RESULT_PENDING a ReceiptValidation, y que el procedimiento para validar un Receipt sea:

1. Crear ReceiptValidation con result=RESULT_PENDING
2. Validar el Receipt
3. Setear result=RESULT_APPROVED o result=RESULT_REJECTED al ReceiptValidation existente, según corresponda.

Luego la app al arrancar busca todos los ReceiptValidation en pending y llama revalidate para ellos.

Otra opción más barata podría ser un write ahead log. Te ahorras las llamadas extra a la DB, pero es más laburo de implementación y probablemente requiera persistent storage.

PD: excelente laburo este proyecto

[npazosmendez]

Ah, estuve mirando más en detalle el código y veo que sería más simple, por las garantías que ya da la implementación actual: buscar los Receipt sin un ReceiptValidation pero con un receipt_number. Esos son los candidatos a validaciones interrumpidas.

[WhyNotHugo]

Creo que esto podría funcionar:

for receipt in Receipt.objects.filter(
    receipt_number__isnull=False,
    validation__isnull=True,
):
    receipt.revalidate()

[WhyNotHugo]

Corrección: ese ejemplo saltearía comprobantes con alguna validación fallida.

[WhyNotHugo]

Capaz así?

for receipt in Receipt.objects.filter(
    receipt_number__isnull=False,
).exclude(
    validation__result=ReceiptValidation.RESULT_APPROVED,
):
    receipt.revalidate()

[npazosmendez]

Suena bien :+1: . Salvo que me esté perdiendo algo, el código actual nunca crea un ReceiptValidation con ReceiptValidation.RESULT_REJECTED ante errores, solamente ReceiptValidation.RESULT_APPROVED:

https://github.com/WhyNotHugo/django-afip/blob/7d08ed4bd28dc3e7302a040d7fee06435586aaf2/django_afip/models.py#L956-L985

En ese caso, tu versión anterior también debería funcionar.

Pero no sé si en algún momento se usó el RESULT_REJECTED, y la DB podría tener alguno con result = RESULT_REJECTED.

[npazosmendez]

Tema relacionado: que opinás de que el revalidate setee receipt_number=None en caso de que no haya sido validado? Así como hace validate para los fallidos:

https://github.com/WhyNotHugo/django-afip/blob/7d08ed4bd28dc3e7302a040d7fee06435586aaf2/django_afip/models.py#L981

Si un Receipt no validado queda con receipt_number != None, al llamar Receipt.validate() se va a intentar usar ese receipt_number ya seteado, que probablemente sea viejo.

Edit: y ahora que lo pienso, no setearlo en None hace que se intente revalidar para siempre.

[WhyNotHugo]

Puede que valga la pena eliminar ReceiptValidation.RESULT_REJECTED completamente. Por ahora dejo un issue como recordatorio: https://github.com/WhyNotHugo/django-afip/issues/214

que opinás de que el revalidate setee receipt_number=None en caso de que no haya sido validado?

Puede haber problemas de concurrencia:

• [thread1] validate setea el receipt_number y empieza a comunicarse con el AFIP.
• [thread2] revalidate encuentra el comprobante a medio validar, consulta al AFIP el estado.
• [thread2] revalidate recibe su respuesta antes que validate. revalidate elimina el receipt_number.
• [thread1] guarda los datos de validación, pero el receipt_number queda perdido.

Es poco probable pero posible.

Creo que habría que lockear las fila durante la validación para evitar este problema.

[npazosmendez]

Buen catch. Estuve pensando en esto de setear el receipt_number en null, acá lo que pensé:

Opcion 1: Lockear las filas

Te dejo el walltext oculto, pero TLDR: creo que se pueden lockear para asegurar la integridad de datos, pero me surgen algunos caveats que hacen medio mala la UX.

walltext

Estuve viendo de cambiar validate para que lockee las filas, algo así (se poco de Django, perdón de antemano):

def validate(...):
      ...
      qs = self.filter(validation__isnull=True).check_groupable()

      # Return early if queryset is empty:
      first = qs.first()
      if first is None:
          return []

      qs.order_by("issued_date", "id")._assign_numbers()
      with transaction.atomic():
          qs = qs.filter(receipt_number__isnull=False).select_for_update()
          ...
          check_response(response)
          errs = []
          for cae_data in response.FeDetResp.FECAEDetResponse:
              validation = ReceiptValidation.objects.create(
              ...

          # Remove the number from ones that failed to validate:
          qs.filter(validation__isnull=True).update(receipt_number=None)

De yapa, esto debería hacer al validate() thread safe (en cuanto a la integridad de datos, no en cuanto a excepciones de AFIP), por poner el qs.filter(validation__isnull=True).update(receipt_number=None) adentro de la transacción. Por otro lado revalidate() haría algo como:

def revalidate(self) -> ReceiptValidation | None:
    ...
    if receipt_data.Resultado == ReceiptValidation.RESULT_APPROVED:
        ...
        return validation
    # Update atomically, skip if it was just validated
    Receipt.objects.filter(pk=self.pk, validation__isnull=True).update(
        receipt_number=None,
    )
    return None

Tendría que pensar un poco más para ver si me estoy periendo algún caso, pero hay algo de esto que no me gusta: si bien (creo) que esto hace que la race condition de tu ejemplo no pierda el receipt_number de una validación, lo que sí ocurre es que no se valida un Receipt al que el usuario llamó .validate(), se skipea. Se mantiene integridad, pero es un comportamiento medio inesperado.

Una que se me ocurre es, luego del qs = qs.filter(receipt_number__isnull=False).select_for_update(), checkear si el set de receipts que quedó es el mismo, y si no, agregar un error a errs (o throwear?). No haría a revalidate y validate thread safe "entre sí", pero al menos el comportamiento es más intuitivo.

Otra opción es empezar a jugar con locks con timeouts.

Opcion 2: comparar con último comprobante

Esta se me ocurrió después, mucho más simple: que el revalidate compare el Receipt.receipt_number con FECompUltimoAutorizado, probablemente dentro de una transacción por si cambia el point of sales y receipt type. Si ya fue usado, borrarlo.

Si no fue usado y se lo deja seteado, todavía podría haber problemas: podría asignarsele el mismo receipt_number a otro Receipt, y si se intentan validar juntos, qs.validate() va a throwear. Lo que podemos hacer es que el validate haga un except <El numero o fecha del comprobante no se corresponde con el proximo a autorizar. ..>, y setee todos los receipt_number = None si tal cosa pasa. Así, el próximo intento debería funcionar. Esto podría hacerse siempre de hecho, más allá del contexto de este problema.

---

PD: tal vez esté dandole mucha vuelta a un caso bastante anómalo :sweat_smile: . La verdad es que la implementación actual no tiene ningún problema de integridad de datos. Para clarificar, lo que estoy queriendo resolver sería: (1) que no se apilen los Receipts no validados por crashes, y (2) que Receipts.validate() para un receipt no validado por crashes no throwee.

[WhyNotHugo]

Me parece que la opción 1 va bien :+1:

[npazosmendez]

Acá hay un primer draft con la opción 1: https://github.com/WhyNotHugo/django-afip/pull/217