From 9e316182d3f4b22a071684c77e2db7ab28229934 Mon Sep 17 00:00:00 2001 From: Jannis Jahr Date: Thu, 16 Jan 2025 11:49:04 +0100 Subject: [PATCH] WCM-575: remove feature flag contentquery_custom_as_sql (#977) --- core/docs/changelog/WCM-575.change | 1 + .../zeit/content/cp/tests/test_automatic.py | 312 +----------------- core/src/zeit/contentquery/configuration.py | 17 +- core/src/zeit/contentquery/query.py | 107 ------ core/src/zeit/contentquery/tests/__init__.py | 1 - .../src/zeit/contentquery/tests/test_query.py | 10 - 6 files changed, 9 insertions(+), 439 deletions(-) create mode 100644 core/docs/changelog/WCM-575.change delete mode 100644 core/src/zeit/contentquery/tests/__init__.py delete mode 100644 core/src/zeit/contentquery/tests/test_query.py diff --git a/core/docs/changelog/WCM-575.change b/core/docs/changelog/WCM-575.change new file mode 100644 index 0000000000..064e553b1e --- /dev/null +++ b/core/docs/changelog/WCM-575.change @@ -0,0 +1 @@ +WCM-575: remove feature flag contentquery_custom_as_sql \ No newline at end of file diff --git a/core/src/zeit/content/cp/tests/test_automatic.py b/core/src/zeit/content/cp/tests/test_automatic.py index b6f542d7e5..1800393006 100644 --- a/core/src/zeit/content/cp/tests/test_automatic.py +++ b/core/src/zeit/content/cp/tests/test_automatic.py @@ -9,7 +9,6 @@ import transaction import zope.component -from zeit.cms.content.sources import FEATURE_TOGGLES from zeit.cms.testcontenttype.testcontenttype import ExampleContentType from zeit.content.cp.interfaces import IRenderedArea import zeit.cms.content.interfaces @@ -144,260 +143,6 @@ def test_it_returns_content_objects_provided_by_elasticsearch(self): self.elasticsearch.search.call_args, ) - def test_builds_query_from_conditions(self): - lead = self.repository['cp'].body['lead'] - lead.count = 1 - source = zeit.cms.content.interfaces.ICommonMetadata['serie'].source(None) - autotest = source.find('Autotest') - lead.query = (('serie', 'eq', autotest),) - lead.automatic = True - lead.automatic_type = 'custom' - IRenderedArea(lead).values() - self.assertEqual( - { - 'query': { - 'bool': { - 'filter': [ - { - 'bool': { - 'filter': [ - {'term': {'payload.document.serie': 'Autotest'}}, - ] - } - }, - {'term': {'payload.workflow.published': True}}, - ], - 'must_not': [ - {'term': {'payload.zeit__DOT__content__DOT__gallery.type': 'inline'}} - ], - } - }, - 'sort': [{'payload.workflow.date_last_published_semantic': 'desc'}], - }, - self.elasticsearch.search.call_args[0][0], - ) - - def test_builds_query_with_elasticsearch_fieldname_exceptions(self): - lead = self.repository['cp'].body['lead'] - lead.count = 1 - lead.query = ( - ('channels', 'eq', 'International', 'Nahost'), - ('channels', 'eq', 'Wissen', None), - ) - lead.automatic = True - lead.automatic_type = 'custom' - IRenderedArea(lead).values() - self.assertEqual( - { - 'query': { - 'bool': { - 'filter': [ - { - 'bool': { - 'filter': [ - { - 'bool': { - 'should': [ - { - 'term': { - 'payload.document.channels.hierarchy': 'International Nahost' - } - }, - { - 'term': { - 'payload.document.channels.hierarchy': 'Wissen' - } - }, - ] - } - }, - ] - } - }, - {'term': {'payload.workflow.published': True}}, - ], - 'must_not': [ - {'term': {'payload.zeit__DOT__content__DOT__gallery.type': 'inline'}} - ], - } - }, - 'sort': [{'payload.workflow.date_last_published_semantic': 'desc'}], - }, - self.elasticsearch.search.call_args[0][0], - ) - - def test_builds_query_with_condition_exception(self): - lead = self.repository['cp'].body['lead'] - lead.count = 1 - lead.query = ( - ('ressort', 'eq', 'International', 'Nahost'), - ('ressort', 'eq', 'Wissen', None), - ) - lead.automatic = True - lead.automatic_type = 'custom' - IRenderedArea(lead).values() - self.assertEqual( - { - 'query': { - 'bool': { - 'filter': [ - { - 'bool': { - 'filter': [ - { - 'bool': { - 'should': [ - { - 'bool': { - 'must': [ - { - 'term': { - 'payload.document.ressort': 'International' - } - }, - { - 'term': { - 'payload.document.sub_ressort': 'Nahost' - } - }, - ] - } - }, - { - 'term': { - 'payload.document.ressort': 'Wissen' - } - }, - ] - } - }, - ] - } - }, - {'term': {'payload.workflow.published': True}}, - ], - 'must_not': [ - {'term': {'payload.zeit__DOT__content__DOT__gallery.type': 'inline'}} - ], - } - }, - 'sort': [{'payload.workflow.date_last_published_semantic': 'desc'}], - }, - self.elasticsearch.search.call_args[0][0], - ) - - def test_joins_different_fields_with_AND_but_same_fields_with_OR(self): - lead = self.repository['cp'].body['lead'] - lead.count = 1 - source = zeit.cms.content.interfaces.ICommonMetadata['serie'].source(None) - autotest = source.find('Autotest') - lead.query = ( - ('channels', 'eq', 'International', 'Nahost'), - ('channels', 'eq', 'Wissen', None), - ('serie', 'eq', autotest), - ('ressort', 'eq', 'Wissen', None), - ) - lead.automatic = True - lead.automatic_type = 'custom' - IRenderedArea(lead).values() - self.assertEqual( - { - 'query': { - 'bool': { - 'filter': [ - { - 'bool': { - 'filter': [ - { - 'bool': { - 'should': [ - { - 'term': { - 'payload.document.channels.hierarchy': 'International Nahost' - } - }, - { - 'term': { - 'payload.document.channels.hierarchy': 'Wissen' - } - }, - ] - } - }, - {'term': {'payload.document.serie': 'Autotest'}}, - {'term': {'payload.document.ressort': 'Wissen'}}, - ] - } - }, - {'term': {'payload.workflow.published': True}}, - ], - 'must_not': [ - {'term': {'payload.zeit__DOT__content__DOT__gallery.type': 'inline'}} - ], - } - }, - 'sort': [{'payload.workflow.date_last_published_semantic': 'desc'}], - }, - self.elasticsearch.search.call_args[0][0], - ) - - def test_puts_fields_into_bool_according_to_operator(self): - lead = self.repository['cp'].body['lead'] - lead.count = 1 - source = zeit.cms.content.interfaces.ICommonMetadata['serie'].source(None) - autotest = source.find('Autotest') - lead.query = ( - ('channels', 'eq', 'International', 'Nahost'), - ('channels', 'eq', 'Wissen', None), - ('serie', 'eq', autotest), - ('ressort', 'neq', 'Wissen', None), - ) - lead.automatic = True - lead.automatic_type = 'custom' - IRenderedArea(lead).values() - self.assertEqual( - { - 'query': { - 'bool': { - 'filter': [ - { - 'bool': { - 'filter': [ - { - 'bool': { - 'should': [ - { - 'term': { - 'payload.document.channels.hierarchy': 'International Nahost' - } - }, - { - 'term': { - 'payload.document.channels.hierarchy': 'Wissen' - } - }, - ] - } - }, - {'term': {'payload.document.serie': 'Autotest'}}, - ], - 'must_not': [ - {'term': {'payload.document.ressort': 'Wissen'}}, - ], - } - }, - {'term': {'payload.workflow.published': True}}, - ], - 'must_not': [ - {'term': {'payload.zeit__DOT__content__DOT__gallery.type': 'inline'}} - ], - } - }, - 'sort': [{'payload.workflow.date_last_published_semantic': 'desc'}], - }, - self.elasticsearch.search.call_args[0][0], - ) - def test_can_take_over_whole_query_body(self): lead = self.repository['cp'].body['lead'] lead.count = 1 @@ -433,61 +178,12 @@ def test_adds_hide_dupes_clause_to_whole_query_body(self): self.elasticsearch.search.call_args[0][0]['query'], ) - def test_custom_query_order_defaults_to_semantic_publish(self): - self.area.automatic_type = 'custom' - IRenderedArea(self.area).values() - self.assertEqual( - [{'payload.workflow.date_last_published_semantic': 'desc'}], - self.elasticsearch.search.call_args[0][0]['sort'], - ) - - def test_custom_query_can_be_forced_to_elastic(self): - FEATURE_TOGGLES.set('contentquery_custom_as_sql') - self.area.automatic_type = 'custom' - self.area.query = (('ressort', 'eq', 'International', 'Nahost'),) - self.area.xml.find('query').set('type', 'elastic') - IRenderedArea(self.area).values() - self.assertTrue(self.elasticsearch.search.called) - def test_query_order_can_be_set(self): self.area.elasticsearch_raw_order = 'order:desc' IRenderedArea(self.area).values() query = self.elasticsearch.search.call_args[0][0] self.assertEqual([{'order': 'desc'}], query['sort']) - def test_bbb_converts_automatic_type_channel_to_custom(self): - lead = self.repository['cp'].body['lead'] - lead.count = 1 - source = zeit.cms.content.interfaces.ICommonMetadata['serie'].source(None) - autotest = source.find('Autotest') - lead.query = (('serie', 'eq', autotest),) - lead.automatic = True - lead.xml.set('automatic_type', 'channel') - IRenderedArea(lead).values() - self.assertEqual( - { - 'query': { - 'bool': { - 'filter': [ - { - 'bool': { - 'filter': [ - {'term': {'payload.document.serie': 'Autotest'}}, - ] - } - }, - {'term': {'payload.workflow.published': True}}, - ], - 'must_not': [ - {'term': {'payload.zeit__DOT__content__DOT__gallery.type': 'inline'}} - ], - } - }, - 'sort': [{'payload.workflow.date_last_published_semantic': 'desc'}], - }, - self.elasticsearch.search.call_args[0][0], - ) - def test_valid_query_despite_missing_order(self): self.area.elasticsearch_raw_query = '{"query": {}}' self.area.elasticsearch_raw_order = '' @@ -1205,7 +901,6 @@ def setUp(self): self.area.automatic_type = 'custom' self.repository['cp'] = self.cp self.connector = zope.component.getUtility(zeit.connector.interfaces.IConnector) - FEATURE_TOGGLES.set('contentquery_custom_as_sql') def test_builds_query_from_conditions(self): source = zeit.cms.content.interfaces.ICommonMetadata['serie'].source(None) @@ -1305,3 +1000,10 @@ def test_print_queries(self): ORDER BY print_page asc... """ self.assertEllipsis(query, self.connector.search_args[0]) + + def test_custom_query_order_defaults_to_semantic_publish(self): + self.area.automatic_type = 'custom' + self.area.query = (('ressort', 'eq', 'International', 'Nahost'),) + IRenderedArea(self.area).values() + query = '...ORDER BY date_last_published_semantic desc...' + self.assertEllipsis(query, self.connector.search_args[0]) diff --git a/core/src/zeit/contentquery/configuration.py b/core/src/zeit/contentquery/configuration.py index b10e11bf76..e5872eb706 100644 --- a/core/src/zeit/contentquery/configuration.py +++ b/core/src/zeit/contentquery/configuration.py @@ -120,22 +120,6 @@ def referenced_cp(self): def referenced_cp(self, value): self._referenced_cp = value - # For automatic_type=custom - _query_order = zeit.cms.content.property.ObjectPathProperty( - '.query_order', IConfiguration['query_order'], use_default=True - ) - - @property - def query_order(self): - from zeit.contentquery.query import CustomContentQuery # break cycle - - value = self._query_order - return CustomContentQuery.ES_ORDER_BWCOMPAT.get(value, value) - - @query_order.setter - def query_order(self, value): - self._query_order = value - for name, default in { # For automatic_type=topicpage 'referenced_topicpage': False, @@ -155,6 +139,7 @@ def query_order(self, value): 'reach_access': False, 'reach_age': False, # For automatic_type=custom + 'query_order': True, 'query_restrict_time': True, 'query_force_queryplan': False, # For automatic_type=sql-query diff --git a/core/src/zeit/contentquery/query.py b/core/src/zeit/contentquery/query.py index 2de78f4648..f5b74f9a35 100644 --- a/core/src/zeit/contentquery/query.py +++ b/core/src/zeit/contentquery/query.py @@ -20,7 +20,6 @@ from zeit.cms.content.cache import content_cache from zeit.cms.content.interfaces import IUUID -from zeit.cms.content.sources import FEATURE_TOGGLES from zeit.cms.interfaces import ICMSContent from zeit.connector.models import TIMESTAMP from zeit.connector.models import Content as ConnectorModel @@ -191,7 +190,6 @@ def _force_queryplan_enabled(self): class SQLCustomContentQuery(SQLContentQuery): grok.name('custom') - grok.baseclass() # See dispatch_custom_query @property def order(self): @@ -285,20 +283,6 @@ def _make_ressort_condition(self, item): return condition -@grok.adapter(zeit.contentquery.interfaces.IConfiguration, name='custom') -@grok.implementer(zeit.contentquery.interfaces.IContentQuery) -def dispatch_custom_query(context): - """Helper for switching during transition phase""" - query = context.xml.find('query') - if query is not None and query.get('type') == 'elastic': - return CustomContentQuery(context) - if FEATURE_TOGGLES.find('contentquery_custom_as_sql'): - return SQLCustomContentQuery(context) - if query is not None and query.get('type') == 'sql': - return SQLCustomContentQuery(context) - return CustomContentQuery(context) - - @grok.adapter(zeit.contentquery.interfaces.IContentQuery) @grok.implementer(ICMSContent) def query_to_content(context): @@ -419,97 +403,6 @@ def hide_dupes_clause(self): return {'ids': {'values': ids}} -class CustomContentQuery(ElasticsearchContentQuery): - grok.name('custom') - grok.baseclass() # See dispatch_custom_query - - ES_FIELD_NAMES = { - 'channels': 'payload.document.channels.hierarchy', - 'content_type': 'doc_type', - } - - ES_ORDER = { - 'date_last_published_semantic': 'payload.workflow.date_last_published_semantic:desc', - 'date_first_released': 'payload.document.date_first_released:desc', - 'date_last_published': 'payload.workflow.date_last_published:desc', - 'print_page': 'payload.document.page:asc', - } - ES_ORDER_BWCOMPAT = {v: k for k, v in ES_ORDER.items()} - - serialize = CustomQueryProperty()._serialize_query_item - - def __init__(self, context): - # Skip direct superclass, as we set `query` and `order` differently. - super(ElasticsearchContentQuery, self).__init__(context) - self.query = self._make_custom_query() - self.order_default = self.ES_ORDER[self.context.query_order] - - def _make_custom_query(self): - fields = {} - for item in self.context.query: - typ = item[0] - fields.setdefault(typ, []).append(item) - - must = [] - must_not = [] - for typ in fields: - positive = [] - for item in fields[typ]: - if item[1] == 'neq': - must_not.append(self._make_clause(typ, item)) - else: - positive.append(item) - if len(positive) > 1: - must.append({'bool': {'should': [self._make_clause(typ, x) for x in positive]}}) - elif len(positive) == 1: - must.append(self._make_clause(typ, positive[0])) - # We rely on _build_query() putting this inside a bool/filter context - query = {'query': {'bool': {}}} - if must: - query['query']['bool']['filter'] = must - if must_not: - query['query']['bool']['must_not'] = must_not - return query - - def _make_clause(self, typ, item): - try: - func = getattr(self, '_make_{}_condition'.format(typ)) - return func(item) - except AttributeError: - return self._make_condition(item) - - def _make_condition(self, item): - typ, operator, value = self.serialize(self.context, item) - return {'term': {self._fieldname(typ): value}} - - @classmethod - def _fieldname(cls, typ): - fieldname = cls.ES_FIELD_NAMES.get(typ) - if fieldname: - return fieldname - - # XXX Generalize the class? - prop = getattr(zeit.content.article.article.Article, typ) - if not isinstance(prop, zeit.cms.content.dav.DAVProperty): - raise ValueError('Cannot determine field name for %s', typ) - return 'payload.%s.%s' % ( - zeit.retresco.content.davproperty_to_es(prop.namespace, prop.name) - ) - - def _make_ressort_condition(self, item): - if item[3]: - return { - 'bool': { - 'must': [ - {'term': {self._fieldname('ressort'): item[2]}}, - {'term': {self._fieldname('sub_ressort'): item[3]}}, - ] - } - } - else: - return {'term': {self._fieldname('ressort'): item[2]}} - - class TMSContentQuery(ContentQuery): grok.name('topicpage') grok.baseclass() diff --git a/core/src/zeit/contentquery/tests/__init__.py b/core/src/zeit/contentquery/tests/__init__.py deleted file mode 100644 index fbaa9a7442..0000000000 --- a/core/src/zeit/contentquery/tests/__init__.py +++ /dev/null @@ -1 +0,0 @@ -# python package diff --git a/core/src/zeit/contentquery/tests/test_query.py b/core/src/zeit/contentquery/tests/test_query.py deleted file mode 100644 index a910b621d5..0000000000 --- a/core/src/zeit/contentquery/tests/test_query.py +++ /dev/null @@ -1,10 +0,0 @@ -from zeit.contentquery.interfaces import QueryTypeSource -from zeit.contentquery.query import CustomContentQuery -import zeit.cms.testing - - -class QueryTest(zeit.cms.testing.ZeitCmsTestCase): - def test_conditions_retrieve_identifier_from_descriptor(self): - for name in QueryTypeSource(): - with self.assertNothingRaised(): - CustomContentQuery._fieldname(name)