fix/anonymous+detached-intents (mycroft-core/pull/2921)

cherry pick of MycroftAI#2921

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 tests for intent collisions

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.

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 = []
         self.skill_id = self.__class__.__name__
 
     def set_bus(self, bus):
@@ -78,15 +79,26 @@ def register_adapt_intent(self, name, intent_parser):
         self.bus.emit(Message("register_intent", intent_parser.__dict__,
                       context={"skill_id": self.skill_id}))
         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},
-                              context={"skill_id": self.skill_id}))
+        name = intent_name.split(':')[1]
+        if name in self:
+            self.bus.emit(Message("detach_intent",
+                                  {"intent_name": intent_name},
+                                  context={"skill_id": self.skill_id}))
+            self.detached_intents.append((name, self.get_intent(name)))
+            self.registered_intents = [pair for pair in self.registered_intents
+                                       if pair[0] != name]
 
     def set_adapt_context(self, context, word, origin):
         """Set an Adapt context.
@@ -160,6 +172,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.
 
@@ -169,8 +183,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:

mycroft/skills/mycroft_skill/mycroft_skill.py

@@ -962,12 +962,25 @@ 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:
@@ -1057,7 +1070,7 @@ 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:
+        for (name, _) in self.intent_service.detached_intents:
             if name == intent_name:
                 return self.enable_intent(intent_name)
 
@@ -1079,7 +1092,7 @@ def disable_intent(self, intent_name):
                 bool: True if disabled, False if it wasn't registered
         """
         if intent_name in self.intent_service:
-            LOG.debug('Disabling intent ' + intent_name)
+            LOG.info('Disabling intent ' + intent_name)
             name = '{}:{}'.format(self.skill_id, intent_name)
             self.intent_service.detach_intent(name)
             return True

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
@@ -621,6 +622,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__()
@@ -698,3 +716,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