Dangerous mutable defaults in combination with RelationList schema fields

[deiferni]

Harvest: OneGov GEVER - Softwarepflege (Refactoring/Bug)

When setting RelationList schema fields programmatically and not trough a form we run into a gotcha.

The missing_value attribute of a z3c-form field is used as soon as an object has no default_value i.e. after creating an object trough the command-line. Because the fields field needs a list as a missing_value, we will fall into the "mutable keyword argument"-python gotcha. The fields default list will be shared between the object-instances.

Unfortunately the z3c-form field does not provide a missing_value-factory (like the defaultFactory) which would be necessary to fix this issue properly.

As a workaround we reassign the field with a new list when it is being non-empty for the first time, for example for IProposal.relatedItems and ISubmittedProposal.excerpts, see https://github.com/4teamwork/opengever.core/blob/45aebffe97ba245b0b30e08359a76f52b7b685fb/opengever/meeting/proposal.py#L629-L644.

This is very dangerous though since the probability that we forget the re-initialize the list is very high in my opinion. Also we certainly don't want do debug/fix references to the same list instance from different content items in production.

Two related ideas to address this, which may or may not work:

• Attempt to intercept when the field is being set for the first time and re-initialize/re-populate the value.
• Somehow use immutable default/missing values (e.g. a tuple) and create a RelationList subclass that accepts tuple as defaults.

input/other ideas to address this are very much welcome 😉 !

[jone]

Can we use defaultFactory=list instead of default=[] in the field configuration?

[lukasgraf]

As discussed with @deiferni:

• Excellent suggestion by @jone to use defaultFactory=list. That would solve the issue for default values for new instances
• If our hypothesis is true, and the problem actually exists as described, tracking down affected object with shared mutable list instances and fixing them is going to be a major pain
• Missing values unfortunately can't be addressed with the factory approach (there's no such thing as a missingValueFactory)

[lukasgraf]

Before we tackle this, we need to first confirm that this is actually a problem in the way we suspect it is. I'm currently sceptical, because

• this would most likely have surfaced a while ago, since lots of RelationLists for example would be incorrectly shared between objects
• it's easy to address on the zope.schema side (if value is an empty instance of a mutable type, create a fresh instance of type(value). They might well have though of this

[lukasgraf]

I did some analysis on this, and in some situations, this indeed can be a problem. Specifically, when disabling our default value patches, using a field which has a missing_value that is mutable but no default value, and programmatically creating content, shared / entangled data structures can be produced.

However, for end users using the usual forms, this shouldn't be an issue:

• The SequenceDataConverter already handles this casefor missing values by returning a(new) empty list
• Default values are handled by a deepcopy in plone.dexterity's _default_from_schema

So at the present time, it's not an urgent issue in my opinion. There might be some edge cases, but in most cases it should work just fine.

=> Moving to milestone 2019.1.

[phgross]

@lukasgraf kannst du hier eine Einschätzung liefern: Wie man das Problem lösen könnte inkl. Aufwandschätzung?

[Rotonen]

I just hit this as a test leak - submitting new documents to self.proposal in the tests creates a ballooning leaking relation list for self.proposal, which is a performance hit, but also causes run order dependent flakiness as some vocabularies now have broken unique constraints after self.subdocument has been added to them twice within the test run and thus the proposal overview of self.proposal cannot be rendered.

A local reproduction can be found with: while GEVER_CACHE_TEST_DB=true bin/test -vvv --shuffle -x -1 -t TestSubmitAdditionalDocumentsIntegration; do sleep 0.1; done

Hopefully this is just a case of bad init by the builder for self.proposal in the fixtures. At least it is differently implemented than the ones for document or task:

https://github.com/4teamwork/opengever.core/blob/eeadf0d03a997bb990b725b27160568225a33ac6/opengever/testing/builders/dx.py#L418-L419

https://github.com/4teamwork/opengever.core/blob/eeadf0d03a997bb990b725b27160568225a33ac6/opengever/testing/builders/dx.py#L166-L171

https://github.com/4teamwork/opengever.core/blob/eeadf0d03a997bb990b725b27160568225a33ac6/opengever/testing/builders/dx.py#L215-L217

[elioschmutz]

While extending a functionality for proposals (https://github.com/4teamwork/opengever.core/issues/6013), I run into this bug.

How to reproduce it

1. Create a proposal
2. Add attachments
3. Submit the proposal
4. Add additional attachments
5. Reload the proposal overview with normal reload and hard reload

=> The attachments are flacking

~The fix suggested by @jone to use a defaultFactory=list would solve the problem for new proposals. But there is still the question how to handle existing proposals (and missing values)~