Project

General

Profile

Bug #44644

Crash sur /api/agenda/foo/meetings/bar/datetimes/ si des événements se chevauchent pour un même guichet

Added by Emmanuel Cazenave 6 days ago. Updated 4 days ago.

Status:
Solution déployée
Priority:
Normal
Category:
-
Target version:
-
Start date:
30 Jun 2020
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

Ce qui peut se produire (peut-être concurrence d'accès).

A l'arrivée ça fait :

Traceback (most recent call last):
  File "/usr/lib/chrono/manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/usr/lib/python3/dist-packages/django/core/management/__init__.py", line 364, in execute_from_command_line
    utility.execute()
  File "/usr/lib/python3/dist-packages/django/core/management/__init__.py", line 356, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/lib/python3/dist-packages/hobo/multitenant/management/commands/tenant_command.py", line 140, in run_from_argv
    klass.run_from_argv(args)
  File "/usr/lib/python3/dist-packages/django/core/management/base.py", line 283, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/lib/python3/dist-packages/django/core/management/base.py", line 330, in execute
    output = self.handle(*args, **options)
  File "/usr/lib/python3/dist-packages/hobo/multitenant/management/commands/runscript.py", line 34, in handle
    runpy.run_module(module_name)
  File "/usr/lib/python3.5/runpy.py", line 208, in run_module
    return _run_code(code, {}, init_globals, run_name, mod_spec)
  File "/usr/lib/python3.5/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/tmp/t.py", line 24, in <module>
    for x in generator_of_unique_slots:
  File "/tmp/t.py", line 14, in unique_slots
    all_slots = list(get_all_slots(agenda, meeting_type, unique=True))
  File "/usr/lib/python3/dist-packages/chrono/api/views.py", line 163, in get_all_slots
    for desk_id, values in itertools.groupby(booked_events, lambda be: be[0])
  File "/usr/lib/python3/dist-packages/chrono/api/views.py", line 163, in <genexpr>
    for desk_id, values in itertools.groupby(booked_events, lambda be: be[0])
  File "/usr/lib/python3/dist-packages/chrono/interval.py", line 72, in from_ordered
    return cls(iterable, already_sorted=True)
  File "/usr/lib/python3/dist-packages/chrono/interval.py", line 57, in __init__
    raise ValueError('not well ordered: ! %s <= %s' % ((last_begin, last_end), (begin, end)))

0001-api-support-overlapping-events-44644.patch View (2.64 KB) Emmanuel Cazenave, 30 Jun 2020 06:32 PM

0001-api-support-overlapping-events-44644.patch View (2.75 KB) Emmanuel Cazenave, 01 Jul 2020 12:47 PM


Related issues

Related to Chrono - Bug #44676: rendez-vous : empecher au niveau base la création de conflit sur des rdv Nouveau 01 Jul 2020

Associated revisions

Revision 4dc54814 (diff)
Added by Emmanuel Cazenave 4 days ago

api: support overlapping events (#44644)

Revision 53a15007 (diff)
Added by Emmanuel Cazenave 4 days ago

api: support overlapping events (#44644)

History

#2 Updated by Emmanuel Cazenave 6 days ago

  • Status changed from Nouveau to En cours
  • Assignee set to Emmanuel Cazenave

#3 Updated by Emmanuel Cazenave 6 days ago

Le test reproduit bien la trace.

Le fix est un tir dans le noir, je comprends pas vraiment ce que fait IntervalSet.

#5 Updated by Lauréline Guerin 6 days ago

Je pense que le fix serait plutôt:

diff --git a/chrono/api/views.py b/chrono/api/views.py
index 2f0bc6d..284ea1d 100644
--- a/chrono/api/views.py
+++ b/chrono/api/views.py
@@ -169,7 +169,7 @@ def get_all_slots(base_agenda, meeting_type, resources=None, unique=False):
             )
             .exclude(booking__cancellation_datetime__isnull=False)
             # ordering is important for the later groupby, it works like sort | uniq
-            .order_by('desk_id', 'start_datetime')
+            .order_by('desk_id', 'start_datetime', 'meeting_type__duration')
             .values_list('desk_id', 'start_datetime', 'meeting_type__duration')
         )
         # compute exclusion set by desk from all bookings, using

Pour être sûr que start + duration ( = end ) est bien dans le bon ordre

J'étais tombée sur ce cas pour les réservations de ressources, et j'ai pas pensé à vérifier l'existant

#6 Updated by Lauréline Guerin 6 days ago

  • Status changed from Solution proposée to En cours

#7 Updated by Thomas Noël 6 days ago

Sauf si j'ai rien compris, tout ça me semble contourner le vrai problème, à savoir que deux rendez-vous ont été pris sur le même guichet au même moment, parce qu'on n'a aucune contrainte SQL sur cela et que deux transactions vont être acceptées tranquillement.

#8 Updated by Benjamin Dauvergne 6 days ago

Thomas Noël a écrit :

Sauf si j'ai rien compris, tout ça me semble contourner le vrai problème, à savoir que deux rendez-vous ont été pris sur le même guichet au même moment, parce qu'on n'a aucune contrainte SQL sur cela et que deux transactions vont être acceptées tranquillement.

Oui, +1 pour Thomas qui a bien suivi en cours de SQL. Je dirai unicité sur (desk_id, start_datetime) mais faudra un index partiel avec une clause WHERE pour que ça ne s'applique que si cancellation_datetime__isnull=True; à l'endroit de la prise de rendez-vous il faudrait utiliser get-or-create sur (desk_id, start_datetime) on peut ajouter agenda_id mais il découle de desk_id normalement (le schéma n'est pas en forme normale je ne sais laquelle).

PS1: dans un autre ticket, la proposition de Laureline suffit ici (qui est la même que la mienne)
PS2: mais d'abord il faut vérifier si le cas ne s'est pas produit énormément ailleurs avant, sinon la migration ne passera jamais.

#9 Updated by Thomas Noël 5 days ago

Benjamin Dauvergne a écrit :

Je dirai unicité sur (desk_id, start_datetime)

Ca suffira pas totalement (même si c'est déjà ça), on pourra toujours ajouter une résa 13h15-13h30 alors que 13h00-14h00 vient d'être reservé dans la transaction précédente. Je pense que c'est le genre de truc compliqué qu'on ne règle qu'à coup de procédures stockées (et on ne va pas aller vers ça, je pense). Et oui, tout ça est pour un autre ticket.

Mais je ne suis pas super chaud pour appliquer le patch proposé ici : il cache le problème sous le tapis (on ne devrait pas avoir besoin de ce patch si les écritures étaient bien contraintes).

#10 Updated by Benjamin Dauvergne 5 days ago

Thomas Noël a écrit :

Benjamin Dauvergne a écrit :

Je dirai unicité sur (desk_id, start_datetime)

Ca suffira pas totalement (même si c'est déjà ça), on pourra toujours ajouter une résa 13h15-13h30 alors que 13h00-14h00 vient d'être reservé dans la transaction précédente. Je pense que c'est le genre de truc compliqué qu'on ne règle qu'à coup de procédures stockées (et on ne va pas aller vers ça, je pense). Et oui, tout ça est pour un autre ticket.

Non il y a aussi des index d'exclusion en Postgresql, mais ça demande que end_datetime soit matérialisé dans la table on ne peut pas demander à postgres de le calculer à la volée depuis meeting_type__duration comme on le fait dans le code.

https://www.postgresql.org/docs/9.4/sql-createtable.html#SQL-CREATETABLE-EXCLUDE

Ça donnerait un truc comme ça :

EXCLUDE USING GIST (desk_id WITH =, tstzrange(start_datetime, end_datetime) WITH &&)

Mais je ne suis pas super chaud pour appliquer le patch proposé ici : il cache le problème sous le tapis (on ne devrait pas avoir besoin de ce patch si les écritures étaient bien contraintes).

En attendant on a aucun moyen d'empêcher ce bug, il faut appliquer ce patch sinon quand le bug arrive ça empêche toute réservation, pour l'instant on peut vivre avec quelques rdv qui se chevauchent plutôt qu'avec des applications qui ne marchent pas.

#11 Updated by Thomas Noël 5 days ago

Benjamin Dauvergne a écrit :

Mais je ne suis pas super chaud pour appliquer le patch proposé ici : il cache le problème sous le tapis (on ne devrait pas avoir besoin de ce patch si les écritures étaient bien contraintes).

En attendant on a aucun moyen d'empêcher ce bug, il faut appliquer ce patch sinon quand le bug arrive ça empêche toute réservation, pour l'instant on peut vivre avec quelques rdv qui se chevauchent plutôt qu'avec des applications qui ne marchent pas.

Ok, allons-y alors. Un mix patch de Lauréline + test d'Emmanuel comme ça tout le monde est content ?

#12 Updated by Emmanuel Cazenave 5 days ago

Lauréline Guerin a écrit :

Je pense que le fix serait plutôt:

Voilà.

#13 Updated by Thomas Noël 5 days ago

  • Status changed from Solution proposée to Solution validée

#14 Updated by Thomas Noël 5 days ago

  • Related to Bug #44676: rendez-vous : empecher au niveau base la création de conflit sur des rdv added

#15 Updated by Frédéric Péters 4 days ago

  • Status changed from Solution validée to Résolu (à déployer)

Poussé, ainsi que vers une branche hotfix.

commit 53a150078b81d5dd5789a66eb7a1e4249bd3d100
Author: Emmanuel Cazenave <ecazenave@entrouvert.com>
Date:   Tue Jun 30 18:13:59 2020 +0200

    api: support overlapping events (#44644)

#16 Updated by Frédéric Péters 4 days ago

  • Status changed from Résolu (à déployer) to Solution déployée

Also available in: Atom PDF