From ab242a2c821553e6ecde0fa656f3be0331aef738 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke?= Date: Wed, 23 Nov 2022 07:22:56 +0100 Subject: [PATCH] Handle multiple intents with the same name (#2921) * Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one. * Add tests for intent collisions * Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. * Add move logic to find if intent is detached MycroftSkill.enable_intent() will now check if the intent is detached before trying to re-enable it. * Lock updates of intents This should avoid some race conditions that may occur if multiple threads tries to enable / disable intents --- mycroft/skills/intent_service_interface.py | 37 +++++- mycroft/skills/mycroft_skill/mycroft_skill.py | 106 ++++++++++++------ test/unittests/skills/test_mycroft_skill.py | 44 +++++++- 3 files changed, 147 insertions(+), 40 deletions(-) diff --git a/mycroft/skills/intent_service_interface.py b/mycroft/skills/intent_service_interface.py index 89a2f292be93..99b7116f341f 100644 --- a/mycroft/skills/intent_service_interface.py +++ b/mycroft/skills/intent_service_interface.py @@ -35,6 +35,7 @@ class IntentServiceInterface: def __init__(self, bus=None): self.bus = bus self.registered_intents = [] + self.detached_intents = [] def set_bus(self, bus): self.bus = bus @@ -83,14 +84,40 @@ def register_adapt_intent(self, name, intent_parser): """ self.bus.emit(Message("register_intent", intent_parser.__dict__)) self.registered_intents.append((name, intent_parser)) + self.detached_intents = [detached for detached in self.detached_intents + if detached[0] != name] def detach_intent(self, intent_name): """Remove an intent from the intent service. + The intent is saved in the list of detached intents for use when + re-enabling an intent. + Args: intent_name(str): Intent reference """ - self.bus.emit(Message("detach_intent", {"intent_name": intent_name})) + name = intent_name.split(':')[1] + if name in self: + self.bus.emit(Message("detach_intent", + {"intent_name": intent_name})) + self.detached_intents.append((name, self.get_intent(name))) + self.registered_intents = [pair for pair in self.registered_intents + if pair[0] != name] + + def intent_is_detached(self, intent_name): + """Determine if an intent is detached. + + Args: + intent_name(str): Intent reference + + Returns: + (bool) True if intent is found, else False. + """ + for (name, _) in self.detached_intents: + if name == intent_name: + return True + + return False def set_adapt_context(self, context, word, origin): """Set an Adapt context. @@ -161,6 +188,8 @@ def __contains__(self, val): def get_intent(self, intent_name): """Get intent from intent_name. + This will find both enabled and disabled intents. + Args: intent_name (str): name to find. @@ -170,8 +199,10 @@ def get_intent(self, intent_name): for name, intent in self: if name == intent_name: return intent - else: - return None + for name, intent in self.detached_intents: + if name == intent_name: + return intent + return None class IntentQueryApi: diff --git a/mycroft/skills/mycroft_skill/mycroft_skill.py b/mycroft/skills/mycroft_skill/mycroft_skill.py index 4f331e27b6a5..a0c8f89ac8fd 100644 --- a/mycroft/skills/mycroft_skill/mycroft_skill.py +++ b/mycroft/skills/mycroft_skill/mycroft_skill.py @@ -22,7 +22,7 @@ from os import walk from os.path import join, abspath, dirname, basename, exists from pathlib import Path -from threading import Event, Timer +from threading import Event, Timer, Lock from xdg import BaseDirectory @@ -161,6 +161,7 @@ def __init__(self, name=None, bus=None, use_settings=True): # Delegator classes self.event_scheduler = EventSchedulerInterface(self.name) self.intent_service = IntentServiceInterface() + self.intent_service_lock = Lock() # Skill Public API self.public_api = {} @@ -364,9 +365,10 @@ def handle_settings_change(self, message): self.settings_change_callback() def detach(self): - for (name, _) in self.intent_service: - name = '{}:{}'.format(self.skill_id, name) - self.intent_service.detach_intent(name) + with self.intent_service_lock: + for (name, _) in self.intent_service: + name = '{}:{}'.format(self.skill_id, name) + self.intent_service.detach_intent(name) def initialize(self): """Perform any final setup needed for the skill. @@ -958,14 +960,28 @@ def remove_event(self, name): def _register_adapt_intent(self, intent_parser, handler): """Register an adapt intent. + Will handle registration of anonymous Args: intent_parser: Intent object to parse utterance for the handler. handler (func): function to register with intent """ # Default to the handler's function name if none given + is_anonymous = not intent_parser.name name = intent_parser.name or handler.__name__ + if is_anonymous: + # Find a good name + original_name = name + nbr = 0 + while name in self.intent_service: + nbr += 1 + name = f'{original_name}{nbr}' + else: + if name in self.intent_service: + raise ValueError(f'The intent name {name} is already taken') + munge_intent_parser(intent_parser, name, self.skill_id) self.intent_service.register_adapt_intent(name, intent_parser) + if handler: self.add_event(intent_parser.name, handler, 'mycroft.skill.handler') @@ -973,6 +989,17 @@ def _register_adapt_intent(self, intent_parser, handler): def register_intent(self, intent_parser, handler): """Register an Intent with the intent service. + Args: + intent_parser: Intent, IntentBuilder object or padatious intent + file to parse utterance for the handler. + handler (func): function to register with intent + """ + with self.intent_service_lock: + self._register_intent(intent_parser, handler) + + def _register_intent(self, intent_parser, handler): + """Register an Intent with the intent service. + Args: intent_parser: Intent, IntentBuilder object or padatious intent file to parse utterance for the handler. @@ -1019,6 +1046,7 @@ def register_intent_file(self, intent_file, handler): filename = self.find_resource(intent_file, 'vocab') if not filename: raise FileNotFoundError('Unable to find "{}"'.format(intent_file)) + self.intent_service.register_padatious_intent(name, filename) if handler: self.add_event(name, handler, 'mycroft.skill.handler') @@ -1047,23 +1075,20 @@ def register_entity_file(self, entity_file): raise FileNotFoundError('Unable to find "{}"'.format(entity_file)) name = '{}:{}'.format(self.skill_id, entity_file) - self.intent_service.register_padatious_entity(name, filename) + with self.intent_service_lock: + self.intent_service.register_padatious_entity(name, filename) def handle_enable_intent(self, message): """Listener to enable a registered intent if it belongs to this skill. """ intent_name = message.data['intent_name'] - for (name, _) in self.intent_service: - if name == intent_name: - return self.enable_intent(intent_name) + return self.enable_intent(intent_name) def handle_disable_intent(self, message): """Listener to disable a registered intent if it belongs to this skill. """ intent_name = message.data['intent_name'] - for (name, _) in self.intent_service: - if name == intent_name: - return self.disable_intent(intent_name) + self.disable_intent(intent_name) def disable_intent(self, intent_name): """Disable a registered intent if it belongs to this skill. @@ -1074,15 +1099,16 @@ def disable_intent(self, intent_name): Returns: bool: True if disabled, False if it wasn't registered """ - if intent_name in self.intent_service: - LOG.debug('Disabling intent ' + intent_name) - name = '{}:{}'.format(self.skill_id, intent_name) - self.intent_service.detach_intent(name) - return True - else: - LOG.error('Could not disable ' - '{}, it hasn\'t been registered.'.format(intent_name)) - return False + with self.intent_service_lock: + if intent_name in self.intent_service: + LOG.info('Disabling intent ' + intent_name) + name = '{}:{}'.format(self.skill_id, intent_name) + self.intent_service.detach_intent(name) + return True + else: + LOG.error('Could not disable ' + f'{intent_name}, it hasn\'t been registered.') + return False def enable_intent(self, intent_name): """(Re)Enable a registered intent if it belongs to this skill. @@ -1094,17 +1120,21 @@ def enable_intent(self, intent_name): bool: True if enabled, False if it wasn't registered """ intent = self.intent_service.get_intent(intent_name) - if intent: - if ".intent" in intent_name: - self.register_intent_file(intent_name, None) + with self.intent_service_lock: + if intent and self.intent_service.intent_is_detached(intent_name): + if ".intent" in intent_name: + self.register_intent_file(intent_name, None) + else: + intent.name = intent_name + self._register_intent(intent, None) + LOG.debug('Enabling intent {}'.format(intent_name)) + return True + elif intent: + LOG.error(f'Could not enable {intent_name}, ' + 'it\'s not detached') else: - intent.name = intent_name - self.register_intent(intent, None) - LOG.debug('Enabling intent {}'.format(intent_name)) - return True - else: - LOG.error('Could not enable ' - '{}, it hasn\'t been registered.'.format(intent_name)) + LOG.error('Could not enable ' + f'{intent_name}, it hasn\'t been registered.') return False def set_context(self, context, word='', origin=''): @@ -1169,7 +1199,8 @@ def register_vocabulary(self, entity, entity_type): entity_type: Intent handler entity to tie the word to """ keyword_type = to_alnum(self.skill_id) + entity_type - self.intent_service.register_adapt_keyword(keyword_type, entity) + with self.intent_service_lock: + self.intent_service.register_adapt_keyword(keyword_type, entity) def register_regex(self, regex_str): """Register a new regex. @@ -1179,7 +1210,8 @@ def register_regex(self, regex_str): self.log.debug('registering regex string: ' + regex_str) regex = munge_regex(regex_str, self.skill_id) re.compile(regex) # validate regex - self.intent_service.register_adapt_regex(regex) + with self.intent_service_lock: + self.intent_service.register_adapt_regex(regex) def speak(self, utterance, expect_response=False, wait=False, meta=None): """Speak a sentence. @@ -1295,9 +1327,10 @@ def load_vocab_files(self, root_directory): for line in keywords[vocab_type]: entity = line[0] aliases = line[1:] - self.intent_service.register_adapt_keyword(vocab_type, - entity, - aliases) + with self.intent_service_lock: + self.intent_service.register_adapt_keyword(vocab_type, + entity, + aliases) def load_regex_files(self, root_directory): """ Load regex files found under the skill directory. @@ -1314,7 +1347,8 @@ def load_regex_files(self, root_directory): regexes = load_regex(locale_dir, self.skill_id) for regex in regexes: - self.intent_service.register_adapt_regex(regex) + with self.intent_service_lock: + self.intent_service.register_adapt_regex(regex) def __handle_stop(self, _): """Handler for the "mycroft.stop" signal. Runs the user defined diff --git a/test/unittests/skills/test_mycroft_skill.py b/test/unittests/skills/test_mycroft_skill.py index bc60ca5c90a7..22f491b74d1d 100644 --- a/test/unittests/skills/test_mycroft_skill.py +++ b/test/unittests/skills/test_mycroft_skill.py @@ -27,7 +27,8 @@ from mycroft.messagebus.message import Message from mycroft.skills.skill_data import (load_regex_from_file, load_regex, load_vocabulary, read_vocab_file) -from mycroft.skills.core import MycroftSkill, resting_screen_handler +from mycroft.skills import (MycroftSkill, resting_screen_handler, + intent_handler) from mycroft.skills.intent_service import open_intent_envelope from test.util import base_config @@ -628,6 +629,23 @@ def test_speak_dialog_render_not_initialized(self): s.speak_dialog(key='key') +class TestIntentCollisions(unittest.TestCase): + def test_two_intents_with_same_name(self): + emitter = MockEmitter() + skill = SameIntentNameSkill() + skill.bind(emitter) + with self.assertRaises(ValueError): + skill.initialize() + + def test_two_anonymous_intent_decorators(self): + """Two anonymous intent handlers should be ok.""" + emitter = MockEmitter() + skill = SameAnonymousIntentDecoratorsSkill() + skill.bind(emitter) + skill._register_decorated() + self.assertEqual(len(skill.intent_service.registered_intents), 2) + + class _TestSkill(MycroftSkill): def __init__(self): super().__init__() @@ -705,3 +723,27 @@ def initialize(self): def handler(self, message): pass + + +class SameIntentNameSkill(_TestSkill): + """Test skill for duplicate intent namesr.""" + skill_id = 'A' + + def initialize(self): + intent = IntentBuilder('TheName').require('Keyword') + intent2 = IntentBuilder('TheName').require('Keyword') + self.register_intent(intent, self.handler) + self.register_intent(intent2, self.handler) + + def handler(self, message): + pass + + +class SameAnonymousIntentDecoratorsSkill(_TestSkill): + """Test skill for duplicate anonymous intent handlers.""" + skill_id = 'A' + + @intent_handler(IntentBuilder('').require('Keyword')) + @intent_handler(IntentBuilder('').require('OtherKeyword')) + def handler(self, message): + pass