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 8b232121ade5..11c2f1f27172 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 hashlib import md5 from xdg import BaseDirectory @@ -162,6 +162,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 = {} @@ -365,9 +366,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. @@ -959,14 +961,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') @@ -974,6 +990,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. @@ -1020,6 +1047,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') @@ -1050,23 +1078,20 @@ def register_entity_file(self, entity_file): name = '{}:{}_{}'.format(self.skill_id, basename(entity_file), str(md5(entity_file.encode('utf-8')) .hexdigest())) - 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. @@ -1077,15 +1102,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. @@ -1097,17 +1123,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=''): @@ -1172,7 +1202,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. @@ -1182,7 +1213,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. @@ -1298,9 +1330,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. @@ -1317,7 +1350,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 254265b1219c..9f121ba33e8a 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 @@ -629,6 +630,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__() @@ -706,3 +724,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