Handle multiple intents with the same name (#2921)

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:

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

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