diff --git a/mycroft/skills/intent_service_interface.py b/mycroft/skills/intent_service_interface.py index ac1c671643a8..20d347c5c109 100644 --- a/mycroft/skills/intent_service_interface.py +++ b/mycroft/skills/intent_service_interface.py @@ -36,6 +36,11 @@ def __init__(self, bus=None): self.bus = bus self.skill_id = self.__class__.__name__ self.registered_intents = [] + self.detached_intents = [] + + @property + def intent_names(self): + return [a[0] for a in self.registered_intents + self.detached_intents] def set_bus(self, bus): self.bus = bus @@ -97,18 +102,56 @@ def register_adapt_intent(self, name, intent_parser): msg.context["skill_id"] = self.skill_id self.bus.emit(msg.forward("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. + DEPRECATED: please use remove_intent instead, all other methods from this class expect intent_name, + this was the weird one expecting the internal munged intent_name with skill_id + + The intent is saved in the list of detached intents for use when + re-enabling an intent. + + Args: + intent_name(str): internal munged intent name (skill_id:name) + """ + name = intent_name.split(':')[1] + self.remove_intent(name) + + def remove_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 """ msg = dig_for_message() or Message("") if "skill_id" not in msg.context: msg.context["skill_id"] = self.skill_id + if intent_name in self.intent_names: + self.detached_intents.append((intent_name, self.get_intent(intent_name))) + self.registered_intents = [pair for pair in self.registered_intents + if pair[0] != intent_name] self.bus.emit(msg.forward("detach_intent", - {"intent_name": intent_name})) + {"intent_name": f"{self.skill_id}.{intent_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. @@ -191,17 +234,21 @@ 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. Returns: Found intent or None if none were found. """ - for name, intent in self: + for name, intent in self.registered_intents: 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 57b41fa6ae3a..a5b0a2875957 100644 --- a/mycroft/skills/mycroft_skill/mycroft_skill.py +++ b/mycroft/skills/mycroft_skill/mycroft_skill.py @@ -21,7 +21,7 @@ from inspect import signature from itertools import chain from os.path import join, abspath, dirname, basename, exists, isfile -from threading import Event +from threading import Event, Lock from hashlib import md5 from ovos_utils.intents import Intent, IntentBuilder @@ -151,6 +151,7 @@ def __init__(self, name=None, bus=None, use_settings=True): # Delegator classes self.event_scheduler = EventSchedulerInterface() self.intent_service = IntentServiceInterface() + self.intent_service_lock = Lock() # Skill Public API self.public_api = {} @@ -587,9 +588,9 @@ def handle_settings_change(self, message): self._start_filewatcher() def detach(self): - for (name, _) in self.intent_service: - name = f'{self.skill_id}:{name}' - self.intent_service.detach_intent(name) + with self.intent_service_lock: + for name in self.intent_service.intent_names: + self.intent_service.remove_intent(name) def initialize(self): """Perform any final setup needed for the skill. @@ -1184,21 +1185,45 @@ 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.intent_names: + nbr += 1 + name = f'{original_name}{nbr}' + elif name in self.intent_service.intent_names and \ + not self.intent_service.intent_is_detached(name): + 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') + self.add_event(intent_parser.name, handler, 'mycroft.skill.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. @@ -1274,23 +1299,20 @@ def register_entity_file(self, entity_file): continue filename = str(entity.file_path) name = f"{self.skill_id}:{basename(entity_file)}_{md5(entity_file.encode('utf-8')).hexdigest()}" - self.intent_service.register_padatious_entity(name, filename, lang) + with self.intent_service_lock: + self.intent_service.register_padatious_entity(name, filename, lang) 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) + return self.disable_intent(intent_name) def disable_intent(self, intent_name): """Disable a registered intent if it belongs to this skill. @@ -1301,19 +1323,14 @@ 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.info('Disabling intent ' + intent_name) - name = f'{self.skill_id}:{intent_name}' - self.intent_service.detach_intent(name) - - langs = [self._core_lang] + self._secondary_langs - for lang in langs: - lang_intent_name = f'{name}_{lang}' - self.intent_service.detach_intent(lang_intent_name) - return True - else: - LOG.error(f'Could not disable {intent_name}, it hasn\'t been registered.') - return False + with self.intent_service_lock: + if intent_name in self.intent_service.intent_names: + LOG.info('Disabling intent ' + intent_name) + self.intent_service.remove_intent(intent_name) + return True + else: + LOG.error(f'Could not disable {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. @@ -1324,15 +1341,20 @@ def enable_intent(self, intent_name): Returns: 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) + if intent_name in self.intent_service.intent_names: + if not self.intent_service.intent_is_detached(intent_name): + LOG.error(f'Could not enable {intent_name}, ' + 'it\'s not detached') + return False else: - intent.name = intent_name - self.register_intent(intent, None) - LOG.debug(f'Enabling intent {intent_name}') - return True + if ".intent" in intent_name: + self.register_intent_file(intent_name, None) + else: + intent = self.intent_service.get_intent(intent_name) + intent.name = intent_name + self._register_intent(intent, None) + LOG.debug(f'Enabling intent {intent_name}') + return True else: LOG.error(f'Could not enable {intent_name}, it hasn\'t been registered.') return False @@ -1406,7 +1428,8 @@ def register_vocabulary(self, entity, entity_type, lang=None): """ keyword_type = self._alphanumeric_skill_id + entity_type lang = lang or self.lang - self.intent_service.register_adapt_keyword(keyword_type, entity, lang=lang) + with self.intent_service_lock: + self.intent_service.register_adapt_keyword(keyword_type, entity, lang=lang) def register_regex(self, regex_str, lang=None): """Register a new regex. @@ -1416,7 +1439,8 @@ def register_regex(self, regex_str, lang=None): 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, lang=lang or self.lang) + with self.intent_service_lock: + self.intent_service.register_adapt_regex(regex, lang=lang or self.lang) def speak(self, utterance, expect_response=False, wait=False, meta=None): """Speak a sentence. @@ -1517,8 +1541,9 @@ def load_vocab_files(self, root_directory=None): for line in skill_vocabulary[vocab_type]: entity = line[0] aliases = line[1:] - self.intent_service.register_adapt_keyword( - vocab_type, entity, aliases, lang) + with self.intent_service_lock: + self.intent_service.register_adapt_keyword( + vocab_type, entity, aliases, lang) def load_regex_files(self, root_directory=None): """ Load regex files found under the skill directory.""" @@ -1528,7 +1553,8 @@ def load_regex_files(self, root_directory=None): if resources.types.regex.base_directory is not None: regexes = resources.load_skill_regex(self._alphanumeric_skill_id) for regex in regexes: - self.intent_service.register_adapt_regex(regex, lang) + with self.intent_service_lock: + self.intent_service.register_adapt_regex(regex, lang) def __handle_stop(self, message): """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 291a32f20997..5f7157a7fcfb 100644 --- a/test/unittests/skills/test_mycroft_skill.py +++ b/test/unittests/skills/test_mycroft_skill.py @@ -26,7 +26,7 @@ from copy import deepcopy from mycroft.configuration import Configuration from mycroft.messagebus.message import Message -from mycroft.skills.core import MycroftSkill, resting_screen_handler +from mycroft.skills.core import MycroftSkill, resting_screen_handler, intent_handler from mycroft.skills.intent_service import open_intent_envelope from mycroft.skills.skill_data import (load_regex_from_file, load_regex, load_vocabulary, read_vocab_file) @@ -619,6 +619,23 @@ def test_native_langs(self): s.config_core['secondary_langs'] = secondary +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__() @@ -697,3 +714,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