WCM-456: Write toggle must not break checkin

core/docs/changelog/WCM-456.change

@@ -0,0 +1 @@
+WCM-456: Keep WebDAVProperties API consistent, operate with string values independent of connector toggles. Instead convert property values only, when metadata columns are origin or source.

core/src/zeit/cms/content/dav.py

@@ -15,8 +15,6 @@
 import zope.xmlpickle
 
 from zeit.cms.content.interfaces import WRITEABLE_ON_CHECKIN
-from zeit.cms.content.sources import FEATURE_TOGGLES
-from zeit.connector.models import Content as ConnectorModel
 from zeit.connector.resource import PropertyKey
 import zeit.cms.content.caching
 import zeit.cms.content.interfaces
@@ -189,19 +187,7 @@ def toProperty(self, value):
 )
 @zope.interface.implementer(zeit.cms.content.interfaces.IDAVPropertyConverter)
 class IntProperty(UnicodeProperty):
-    def __init__(self, context, properties, propertykey):
-        super().__init__(context, properties, propertykey)
-        self.has_sql_type = ConnectorModel.column_by_name(*propertykey) is not None
-
-    def fromProperty(self, value):
-        if self.has_sql_type and FEATURE_TOGGLES.find('read_metadata_columns'):
-            return value
-        return super().fromProperty(value)
-
-    def toProperty(self, value):
-        if self.has_sql_type and FEATURE_TOGGLES.find('write_metadata_columns'):
-            return value
-        return super().toProperty(value)
+    pass
 
 
 @zope.component.adapter(
@@ -327,20 +313,15 @@ def toProperty(self, value):
 class BoolProperty:
     def __init__(self, context, properties, propertykey):
         self.context = context
-        self.has_sql_type = ConnectorModel.column_by_name(*propertykey) is not None
 
     def fromProperty(self, value):
-        if self.has_sql_type and FEATURE_TOGGLES.find('read_metadata_columns'):
-            return value
         return self._fromProperty(value)
 
     @staticmethod
     def _fromProperty(value):
         return value.lower() in ('yes', 'true')
 
     def toProperty(self, value):
-        if self.has_sql_type and FEATURE_TOGGLES.find('write_metadata_columns'):
-            return value
         return self._toProperty(value)
 
     @staticmethod
@@ -355,13 +336,10 @@ def _toProperty(value):
 class DatetimeProperty:
     def __init__(self, context, properties, propertykey):
         self.context = context
-        self.has_sql_type = ConnectorModel.column_by_name(*propertykey) is not None
 
     def fromProperty(self, value):
         if not value:
             return None
-        if self.has_sql_type and FEATURE_TOGGLES.find('read_metadata_columns'):
-            return value
         return self._fromProperty(value)
 
     @staticmethod
@@ -372,8 +350,6 @@ def _fromProperty(value):
         return date.in_tz('UTC')
 
     def toProperty(self, value):
-        if self.has_sql_type and FEATURE_TOGGLES.find('write_metadata_columns'):
-            return value
         return self._toProperty(value)
 
     @staticmethod
@@ -407,7 +383,6 @@ class CollectionTextLineProperty:
     SPLIT_PATTERN = re.compile(r'(?!\\);')
 
     def __init__(self, context, value_type, properties, propertykey):
-        self.has_sql_type = ConnectorModel.column_by_name(*propertykey) is not None
         self.context = context
         self.value_type = value_type
         self.properties = properties
@@ -418,8 +393,6 @@ def __init__(self, context, value_type, properties, propertykey):
             self._type = self._type[0]
 
     def fromProperty(self, value):
-        if self.has_sql_type and FEATURE_TOGGLES.find('read_metadata_columns'):
-            return value
         typ = zope.component.getMultiAdapter(
             (self.value_type, self.properties, self.propertykey),
             zeit.cms.content.interfaces.IDAVPropertyConverter,
@@ -444,8 +417,6 @@ def fromProperty(self, value):
         return self._type(result)
 
     def toProperty(self, value):
-        if self.has_sql_type and FEATURE_TOGGLES.find('write_metadata_columns'):
-            return value
         typ = zope.component.getMultiAdapter(
             (self.value_type, self.properties, self.propertykey),
             zeit.cms.content.interfaces.IDAVPropertyConverter,

core/src/zeit/connector/filesystem.py

@@ -11,9 +11,7 @@
 import zope.app.file.image
 import zope.interface
 
-from zeit.cms.content.sources import FEATURE_TOGGLES
 from zeit.connector.interfaces import ID_NAMESPACE, CannonicalId
-from zeit.connector.models import Content
 import zeit.cms.config
 import zeit.cms.content.dav
 import zeit.connector.converter
@@ -254,27 +252,13 @@ def _get_properties(self, id):
             return properties
 
         properties.update(parse_properties(xml))
-        self._convert_sql_types(properties)
 
         if zeit.connector.interfaces.RESOURCE_TYPE_PROPERTY not in properties:
             properties[zeit.connector.interfaces.RESOURCE_TYPE_PROPERTY] = self._guess_type(id)
 
         self.property_cache[id] = properties
         return properties
 
-    def _convert_sql_types(self, properties):
-        if not FEATURE_TOGGLES.find('read_metadata_columns'):
-            return
-        for key, value in properties.items():
-            properties[key] = self._convert_sql(key, value)
-
-    def _convert_sql(self, key, value):
-        column = Content.column_by_name(*key)
-        if column is None:
-            return value
-        converter = zeit.connector.interfaces.IConverter(column)
-        return converter.deserialize(value)
-
     def _guess_type(self, id):
         path = self._path(id)
         if os.path.isdir(path):

core/src/zeit/connector/mock.py

@@ -12,7 +12,6 @@
 import pendulum
 import zope.event
 
-from zeit.cms.content.sources import FEATURE_TOGGLES
 from zeit.connector.interfaces import (
     ID_NAMESPACE,
     UUID_PROPERTY,
@@ -362,17 +361,8 @@ def _get_properties(self, id):
             properties = super()._get_properties(id)
         else:
             properties = properties.copy()
-            self._convert_sql_types(properties)
         return properties
 
-    def _convert_sql(self, key, value):
-        if (
-            FEATURE_TOGGLES.find('write_metadata_columns')
-            or FEATURE_TOGGLES.find('write_metadata_columns_strict')
-        ) and not isinstance(value, str):
-            return value
-        return super()._convert_sql(key, value)
-
     def _set_properties(self, id, properties):
         stored_properties = self._get_properties(id)
         for (name, namespace), value in properties.items():
@@ -386,10 +376,7 @@ def _set_properties(self, id, properties):
                 stored_properties.pop((name, namespace), None)
                 continue
 
-            if not (
-                FEATURE_TOGGLES.find('write_metadata_columns')
-                or FEATURE_TOGGLES.find('write_metadata_columns_strict')
-            ) and not isinstance(value, str):
+            if not isinstance(value, str):
                 raise ValueError('Expected str, got %s: %r' % (type(value), value))
             stored_properties[(name, namespace)] = value
         self._properties[id] = stored_properties

core/src/zeit/connector/models.py

@@ -226,7 +226,8 @@ def to_webdav(self):
                 namespace, name = column.info['namespace'], column.info['name']
                 value = getattr(self, column.name)
                 if value is not None:
-                    props[(name, self.NS + namespace)] = value
+                    converter = zeit.connector.interfaces.IConverter(column)
+                    props[(name, self.NS + namespace)] = converter.serialize(value)
 
         if self.lock:
             props[('lock_principal', INTERNAL_PROPERTY)] = self.lock.principal
@@ -261,31 +262,24 @@ def from_webdav(self, props):
                 if value is DeleteProperty:
                     setattr(self, column.name, None)
                     continue
+                if not isinstance(value, str):
+                    raise ValueError('Expected str, got %r' % value)
 
                 converter = zeit.connector.interfaces.IConverter(column)
-                # Need to support mixed properties (str/typed) for these cases:
-                # - toggle write, but no toggle read: properties read from
-                #   connector are still str, properties written by content layer
-                #   are already typed
-                # - existing workingcopies may still have str properties
-                # - existing cache entries may still have str properties
-                # Thus, we also need to keep a sufficient grace period after
-                # toggle read is enabled before removing the toggles from the code.
-                if isinstance(value, str):
-                    value = converter.deserialize(value)
-
-                setattr(self, column.name, value)
+                setattr(self, column.name, converter.deserialize(value))
+
                 if FEATURE_TOGGLES.find('write_metadata_columns_strict'):
                     props.pop((name, self.NS + namespace), None)
-                else:
-                    props[name, self.NS + namespace] = converter.serialize(value)
 
         unsorted = collections.defaultdict(dict)
         for (k, ns), v in props.items():
             if v is DeleteProperty:
                 continue
             if ns == INTERNAL_PROPERTY:
                 continue
+            if not isinstance(v, str):
+                raise ValueError('Expected str, got %r' % v)
+
             unsorted[ns.replace(self.NS, '', 1)][k] = v
         self.unsorted = unsorted
 

core/src/zeit/connector/testing.py

@@ -195,6 +195,7 @@ def setUp(self):
     def tearDown(self):
         self['sql_connection'].close()
         del self['sql_connection']
+        del os.environ['PGDATABASE']
 
     def testSetUp(self):
         """Sets up a transaction savepoint, which will be rolled back

core/src/zeit/connector/tests/test_contract.py

@@ -9,9 +9,6 @@
 import transaction
 import zope.interface.verify
 
-from zeit.cms.checkout.helper import checked_out
-from zeit.cms.content.sources import FEATURE_TOGGLES
-from zeit.cms.testcontenttype.testcontenttype import ExampleContentType
 from zeit.connector.interfaces import (
     CopyError,
     DeleteProperty,
@@ -882,82 +879,3 @@ class ContractZopeSQL(
     copy_inherited_functions(ContractLock, locals())
     copy_inherited_functions(ContractSearch, locals())
     copy_inherited_functions(ContractCache, locals())
-
-
-class ContractProperties:
-    def setUp(self):
-        super().setUp()
-        FEATURE_TOGGLES.set('read_metadata_columns')
-        FEATURE_TOGGLES.set('write_metadata_columns')
-        self.repository['testcontent'] = ExampleContentType()
-
-    def test_converts_scalar_types_on_read(self):
-        example_date = pendulum.datetime(2010, 1, 1, 0, 0)
-        ressort = 'Wirtschaft'
-        volume_year = 2024
-        published = False
-        self.repository.connector.changeProperties(
-            'http://xml.zeit.de/testcontent',
-            {
-                ('date_created', 'http://namespaces.zeit.de/CMS/document'): example_date,
-                ('ressort', 'http://namespaces.zeit.de/CMS/print'): ressort,
-                ('year', 'http://namespaces.zeit.de/CMS/document'): volume_year,
-                ('published', 'http://namespaces.zeit.de/CMS/workflow'): published,
-            },
-        )
-        self.assertEqual(
-            example_date,
-            zeit.cms.workflow.interfaces.IModified(self.repository['testcontent']).date_created,
-        )
-        self.assertEqual(ressort, self.repository['testcontent'].printRessort)
-        self.assertEqual(volume_year, self.repository['testcontent'].year)
-        self.assertEqual(
-            published,
-            zeit.cms.workflow.interfaces.IPublishInfo(self.repository['testcontent']).published,
-        )
-
-    def test_delete_property_from_column(self):
-        id = 'http://xml.zeit.de/testcontent'
-        example_date = pendulum.datetime(2010, 1, 1, 0, 0)
-        prop = ('date_created', 'http://namespaces.zeit.de/CMS/document')
-        connector = self.repository.connector
-        connector.changeProperties(id, {prop: example_date})
-        transaction.commit()
-        connector.changeProperties(id, {prop: DeleteProperty})
-        transaction.commit()
-        res = connector[id]
-        self.assertNotIn(prop, res.properties)
-
-    def test_converts_scalar_types_on_write(self):
-        example_date = pendulum.datetime(2010, 1, 1, 0, 0)
-        with checked_out(self.repository['testcontent']) as co:
-            zeit.cms.workflow.interfaces.IModified(co).date_created = example_date
-        resource = self.repository.connector['http://xml.zeit.de/testcontent']
-        self.assertEqual(
-            example_date,
-            resource.properties[('date_created', 'http://namespaces.zeit.de/CMS/document')],
-        )
-
-    def test_converts_channels_on_read(self):
-        value = (('International', 'Nahost'), ('Wissen', None))
-        self.repository.connector.changeProperties(
-            'http://xml.zeit.de/testcontent',
-            {('channels', 'http://namespaces.zeit.de/CMS/document'): value},
-        )
-        self.assertEqual(value, self.repository['testcontent'].channels)
-
-    def test_converts_channels_on_write(self):
-        value = (('International', 'Nahost'), ('Wissen', None))
-        with checked_out(self.repository['testcontent']) as co:
-            co.channels = value
-        self.assertEqual(value, self.repository['testcontent'].channels)
-
-
-class PropertiesSQL(ContractProperties, zeit.cms.testing.FunctionalTestCase):
-    layer = zeit.connector.testing.SQL_CONTENT_LAYER
-    copy_inherited_functions(ContractProperties, locals())
-
-
-class PropertiesMock(ContractProperties, zeit.cms.testing.FunctionalTestCase):
-    layer = zeit.cms.testing.ZOPE_LAYER
-    copy_inherited_functions(ContractProperties, locals())

core/src/zeit/connector/tests/test_mock.py

@@ -5,7 +5,6 @@
 import pendulum
 import transaction
 
-from zeit.cms.content.sources import FEATURE_TOGGLES
 from zeit.connector.search import SearchVar as SV
 import zeit.connector.testing
 
@@ -66,40 +65,3 @@ def test_switch_of_locking(self):
         transaction.commit()
         res = self.get_resource('foo')
         self.connector['http://xml.zeit.de/testing/foo'] = res
-
-
-class MockTypeConversionTest(zeit.connector.testing.MockTest):
-    def test_converts_sql_properties_on_read(self):
-        params = [
-            [
-                ('workflow', 'published'),
-                True,
-                'yes',
-            ],
-            [
-                ('document', 'date_created'),
-                pendulum.datetime(1970, 1, 1),
-                '1970-01-01T00:00:00+00:00',
-            ],
-            [
-                ('document', 'channels'),
-                (
-                    ('International', 'Nahost'),
-                    ('Wissen', None),
-                ),
-                'International Nahost;Wissen',
-            ],
-            [
-                ('document', 'channels'),
-                (),
-                '',
-            ],
-        ]
-        for prop, val_py, val_str in params:
-            prop = (prop[1], f'http://namespaces.zeit.de/CMS/{prop[0]}')
-            FEATURE_TOGGLES.unset('read_metadata_columns')
-            res = self.add_resource('foo', properties={prop: val_str})
-            self.assertEqual(res.properties[prop], val_str)
-            FEATURE_TOGGLES.set('read_metadata_columns')
-            res = self.connector[res.id]
-            self.assertEqual(res.properties[prop], val_py)