Project

General

Profile

Development #38049

ne pas modifier formdata quand il est copié ou picklé

Added by Benjamin Dauvergne 3 months ago. Updated 9 days ago.

Status:
Solution déployée
Priority:
Normal
Target version:
-
Start date:
28 Nov 2019
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

FormData.__getstate__ modifie directement self.__dict__ au lieu de le copier avant.

0001-formdata-do-not-modify-__dict__-in-__getstate__-3804.patch View (1.02 KB) Benjamin Dauvergne, 28 Nov 2019 12:32 PM

Associated revisions

Revision 6a63b819 (diff)
Added by Benjamin Dauvergne 11 days ago

formdata: do not modify dict in getstate (#38049)

It suppress the _formdef cache, which can break things with transient
formdata created from the FormData class instead of a FormDef.data_class().

Ideally transient FormData should be never copied as the copy is lacking
a _formdef attribute and _names is not initialised so
FormData.get_formdef() will never work.

History

#1 Updated by Benjamin Dauvergne 3 months ago

#2 Updated by Frédéric Péters 3 months ago

Ça pose quel problème, qui peut être exposé dans un test ?

#3 Updated by Benjamin Dauvergne 3 months ago

Ça ma causé des soucis sur #33186 (initialisation d'un brouillon), quand j'ai voulu ajouté le support "pickle-lazy" il fallait à la fois ajouter la nouvelle variable linked dans FormData.get_substitution_variables() mais aussi la supprimer1 dans FormData.get_static_substitution_variables() sinon linked était copié par deecopy() à un moment et le formdata transient se retrouver sans ._formdef.

On peut voir les traces du à ce souci sur jenkins2.

C'est pas en soi un bug existant puisque tout est fait pour l'éviter actuellement (je ne sais pas si c'est ça qui a mené au nettoyage de lazy avec des del), mais ça évitera à un autre d'avoir à s'arracher les cheveux plus tard.

1 https://git.entrouvert.org/wcs.git/commit/?h=wip/33186-Initialisation-d-un-brouillon&id=c13a885c982a8dc7297f4f5f29dad0f75d6665bd

2 https://jenkins.entrouvert.org/job/wcs-wip/job/wip%252F33186-Initialisation-d-un-brouillon/3/

#4 Updated by Benjamin Dauvergne 17 days ago

branche rebasée, pour moi le problème est toujours là sous-jacent (clairement faire un .copy() ne va rien casser).

#5 Updated by Frédéric Péters 17 days ago

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

Pour mémoire j'ai du mal à valider du changement de code juste sur un hypothétique futur sur lequel je devrais sans doute prendre plus de temps pour réfléchir. Mais j'ai un attrait pour avoir du code uniforme et donc l'argument comme quoi c'est déjà fait ainsi ailleurs, il marcherait sans me demander d'effort :

    # don't pickle _formata cache
    def __getstate__(self):
        odict = self.__dict__.copy()
        if '_formdata' in odict:
            del odict['_formdata']
        if '_display_parts' in odict:
            del odict['_display_parts']
        return odict

#6 Updated by Benjamin Dauvergne 16 days ago

Frédéric Péters a écrit :

Pour mémoire j'ai du mal à valider du changement de code juste sur un hypothétique futur sur lequel je devrais sans doute prendre plus de temps pour réfléchir. Mais j'ai un attrait pour avoir du code uniforme et donc l'argument comme quoi c'est déjà fait ainsi ailleurs, il marcherait sans me demander d'effort :

[...]

J'en savais foutre rien que c'était déjà comme ça sur une autre classe, c'est une découverte indépendante.

#7 Updated by Benjamin Dauvergne 11 days ago

  • Status changed from Solution validée to Résolu (à déployer)
commit 6a63b81928f28211eaab824840798a5954b374b2
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Thu Nov 28 12:29:41 2019 +0100

    formdata: do not modify __dict__ in __getstate__ (#38049)

    It suppress the _formdef cache, which can break things with transient
    formdata created from the FormData class instead of a FormDef.data_class().

    Ideally transient FormData should be never copied as the copy is lacking
    a _formdef attribute and _names is not initialised so
    FormData.get_formdef() will never work.

#8 Updated by Frédéric Péters 9 days ago

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

Also available in: Atom PDF