diff --git a/core/docs/changelog/WCM-456.change b/core/docs/changelog/WCM-456.change new file mode 100644 index 0000000000..17667d0327 --- /dev/null +++ b/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. diff --git a/core/src/zeit/cms/content/dav.py b/core/src/zeit/cms/content/dav.py index 5a73d5a920..98f9d3ac23 100644 --- a/core/src/zeit/cms/content/dav.py +++ b/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,11 +313,8 @@ 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 @@ -339,8 +322,6 @@ 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, diff --git a/core/src/zeit/connector/filesystem.py b/core/src/zeit/connector/filesystem.py index 50e9e1ee16..4fa03b535f 100644 --- a/core/src/zeit/connector/filesystem.py +++ b/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,7 +252,6 @@ 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) @@ -262,19 +259,6 @@ def _get_properties(self, 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): diff --git a/core/src/zeit/connector/mock.py b/core/src/zeit/connector/mock.py index b422cdc329..812ef67dac 100644 --- a/core/src/zeit/connector/mock.py +++ b/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 diff --git a/core/src/zeit/connector/models.py b/core/src/zeit/connector/models.py index 21b096bd33..0edc0b50b9 100644 --- a/core/src/zeit/connector/models.py +++ b/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,24 +262,14 @@ 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(): @@ -286,6 +277,9 @@ def from_webdav(self, props): 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 diff --git a/core/src/zeit/connector/testing.py b/core/src/zeit/connector/testing.py index ecac4247e1..99f1042a32 100644 --- a/core/src/zeit/connector/testing.py +++ b/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 diff --git a/core/src/zeit/connector/tests/test_contract.py b/core/src/zeit/connector/tests/test_contract.py index 4d952ddf12..1878da162b 100644 --- a/core/src/zeit/connector/tests/test_contract.py +++ b/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()) diff --git a/core/src/zeit/connector/tests/test_mock.py b/core/src/zeit/connector/tests/test_mock.py index ea445b827d..fd769b7bca 100644 --- a/core/src/zeit/connector/tests/test_mock.py +++ b/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) diff --git a/core/src/zeit/connector/tests/test_postgresql.py b/core/src/zeit/connector/tests/test_postgresql.py index eec3b69c43..b5db1d1364 100644 --- a/core/src/zeit/connector/tests/test_postgresql.py +++ b/core/src/zeit/connector/tests/test_postgresql.py @@ -2,6 +2,7 @@ from unittest import mock import datetime +from pytest import raises from sqlalchemy import func, select from sqlalchemy import text as sql from sqlalchemy.exc import IntegrityError @@ -9,9 +10,12 @@ import pendulum import transaction +from zeit.cms.checkout.helper import checked_out from zeit.cms.content.sources import FEATURE_TOGGLES from zeit.cms.repository.interfaces import ConflictError -from zeit.connector.interfaces import INTERNAL_PROPERTY +from zeit.cms.testcontenttype.testcontenttype import ExampleContentType +from zeit.cms.workflow.interfaces import IModified +from zeit.connector.interfaces import INTERNAL_PROPERTY, DeleteProperty from zeit.connector.models import Content, Lock from zeit.connector.postgresql import _unlock_overdue_locks from zeit.connector.resource import Resource, WriteableCachedResource @@ -344,39 +348,20 @@ def test_properties_are_written_simultaneously_to_separate_column_and_unsorted(s FEATURE_TOGGLES.set('write_metadata_columns') FEATURE_TOGGLES.set('read_metadata_columns') timestamp = pendulum.datetime(1980, 1, 1) - res = self.add_resource('foo', properties={('date_created', f'{NS}document'): timestamp}) - self.assertEqual(timestamp, res.properties[('date_created', f'{NS}document')]) - content = self.connector._get_content(res.id) - self.assertEqual({'document': {'date_created': timestamp.isoformat()}}, content.unsorted) - self.assertEqual(timestamp, content.date_created) - - def test_properties_can_be_stored_in_separate_columns_and_still_read_as_dav(self): - FEATURE_TOGGLES.set('write_metadata_columns') - timestamp = pendulum.datetime(1980, 1, 1) isoformat = timestamp.isoformat() - res = self.add_resource( - 'foo', - properties={ - ('date_created', f'{NS}document'): isoformat, - ('date_last_checkout', f'{NS}document'): timestamp, - }, - ) + res = self.add_resource('foo', properties={('date_created', f'{NS}document'): isoformat}) self.assertEqual(isoformat, res.properties[('date_created', f'{NS}document')]) - self.assertEqual(isoformat, res.properties[('date_last_checkout', f'{NS}document')]) content = self.connector._get_content(res.id) - self.assertEqual( - {'document': {'date_created': isoformat, 'date_last_checkout': isoformat}}, - content.unsorted, - ) + self.assertEqual({'document': {'date_created': isoformat}}, content.unsorted) self.assertEqual(timestamp, content.date_created) - self.assertEqual(timestamp, content.date_last_checkout) def test_properties_can_be_stored_in_separate_columns(self): FEATURE_TOGGLES.set('write_metadata_columns_strict') FEATURE_TOGGLES.set('read_metadata_columns') timestamp = pendulum.datetime(1980, 1, 1) - res = self.add_resource('foo', properties={('date_created', f'{NS}document'): timestamp}) - self.assertEqual(timestamp, res.properties[('date_created', f'{NS}document')]) + isoformat = timestamp.isoformat() + res = self.add_resource('foo', properties={('date_created', f'{NS}document'): isoformat}) + self.assertEqual(isoformat, res.properties[('date_created', f'{NS}document')]) content = self.connector._get_content(res.id) self.assertEqual({}, content.unsorted) self.assertEqual(timestamp, content.date_created) @@ -394,3 +379,33 @@ def test_search_looks_in_columns_or_unsorted_depending_on_toggle(self): result = self.connector.search([var], var == 'Wissen') unique_id, uuid = next(result) self.assertEqual(res.id, unique_id) + + def test_revoke_write_toggle_must_not_break_checkin(self): + FEATURE_TOGGLES.set('write_metadata_columns') + self.repository['testcontent'] = ExampleContentType() + example_date = pendulum.datetime(2024, 10, 1) + with checked_out(self.repository['testcontent']) as co: + IModified(co).date_created = example_date + FEATURE_TOGGLES.unset('write_metadata_columns') + + def test_delete_property_from_column(self): + FEATURE_TOGGLES.set('read_metadata_columns') + FEATURE_TOGGLES.set('write_metadata_columns') + id = 'http://xml.zeit.de/testcontent' + self.repository['testcontent'] = ExampleContentType() + example_date = pendulum.datetime(2024, 1, 1).isoformat() + prop = ('date_created', 'http://namespaces.zeit.de/CMS/document') + self.connector.changeProperties(id, {prop: example_date}) + transaction.commit() + self.connector.changeProperties(id, {prop: DeleteProperty}) + transaction.commit() + res = self.connector[id] + self.assertNotIn(prop, res.properties) + + def test_unsorted_properties_must_be_strings(self): + date = pendulum.datetime(2024, 1, 1) + with raises(ValueError): + self.add_resource('foo', properties={('date_created', f'{NS}document'): date}) + FEATURE_TOGGLES.set('write_metadata_columns') + with raises(ValueError): + self.add_resource('bar', properties={('date_created', f'{NS}document'): date})