diff --git a/mycroft/skills/intent_service_interface.py b/mycroft/skills/intent_service_interface.py index 89a2f292be..99b7116f34 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 @@ class IntentServiceInterface: """ 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 @@ class IntentServiceInterface: 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 @@ class IntentServiceInterface: 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 4f331e27b6..a0c8f89ac8 100644 --- a/mycroft/skills/mycroft_skill/mycroft_skill.py +++ b/mycroft/skills/mycroft_skill/mycroft_skill.py @@ -22,7 +22,7 @@ from itertools import chain 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 @@ class MycroftSkill: # 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 @@ class MycroftSkill: 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 @@ class MycroftSkill: 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 @@ class MycroftSkill: 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 @@ class MycroftSkill: 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 @@ class MycroftSkill: 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 @@ class MycroftSkill: 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 @@ class MycroftSkill: 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 @@ class MycroftSkill: 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 @@ class MycroftSkill: 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 @@ class MycroftSkill: 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 @@ class MycroftSkill: 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 bc60ca5c90..22f491b74d 100644 --- a/test/unittests/skills/test_mycroft_skill.py +++ b/test/unittests/skills/test_mycroft_skill.py @@ -27,7 +27,8 @@ from mycroft.configuration import Configuration 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 @@ class TestMycroftSkill(unittest.TestCase): 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 @@ class SimpleSkill6(_TestSkill): 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