From 1042d5bef0d6b827ea1f2425fe168e8cb3d33d19 Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Tue, 26 Sep 2023 14:41:58 +0100 Subject: [PATCH] disallow list slots in flows, add log, amend exception messages --- rasa/validator.py | 43 ++++++++++++++++++------ tests/test_validator.py | 74 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 99 insertions(+), 18 deletions(-) diff --git a/rasa/validator.py b/rasa/validator.py index b2d85256bf70..cd2a49c4ed58 100644 --- a/rasa/validator.py +++ b/rasa/validator.py @@ -34,6 +34,7 @@ from rasa.shared.core.domain import Domain from rasa.shared.core.generator import TrainingDataGenerator from rasa.shared.core.constants import SlotMappingType, MAPPING_TYPE +from rasa.shared.core.slots import ListSlot from rasa.shared.core.training_data.structures import StoryGraph from rasa.shared.exceptions import RasaException from rasa.shared.importers.importer import TrainingDataImporter @@ -495,34 +496,52 @@ def warn_if_config_mandatory_keys_are_not_set(self) -> None: def verify_flows_steps_against_domain(self, user_flows: List[Flow]) -> bool: """Checks flows steps' references against the domain file.""" all_good = True - domain_slot_names = [slot.name for slot in self.domain.slots] + domain_slots = {slot.name: slot for slot in self.domain.slots} for flow in user_flows: for step in flow.steps: if isinstance(step, CollectInformationFlowStep): - if step.collect_information not in domain_slot_names: + if step.collect_information not in domain_slots: raise RasaException( f"The slot '{step.collect_information}' is used in the " - f"step '{step.id}' of flow '{flow.name}', but it " + f"step '{step.id}' of flow id '{flow.id}', but it " f"is not listed in the domain slots. " f"You should add it to your domain file!", ) + current_slot = domain_slots[step.collect_information] + if isinstance(current_slot, ListSlot): + raise RasaException( + f"The slot '{step.collect_information}' is used in the " + f"step '{step.id}' of flow id '{flow.id}', but it " + f"is a list slot. List slots are currently not " + f"supported in flows. You should change it to a " + f"text, boolean or float slot in your domain file!", + ) elif isinstance(step, SetSlotsFlowStep): for slot in step.slots: slot_name = slot["key"] - if slot_name not in domain_slot_names: + if slot_name not in domain_slots: raise RasaException( f"The slot '{slot_name}' is used in the step " - f"'{step.id}' of flow '{flow.name}', but it " + f"'{step.id}' of flow id '{flow.id}', but it " f"is not listed in the domain slots. " f"You should add it to your domain file!", ) + current_slot = domain_slots[slot_name] + if isinstance(current_slot, ListSlot): + raise RasaException( + f"The slot '{slot_name}' is used in the " + f"step '{step.id}' of flow id '{flow.id}', but it " + f"is a list slot. List slots are currently not " + f"supported in flows. You should change it to a " + f"text, boolean or float slot in your domain file!", + ) elif isinstance(step, ActionFlowStep): if step.action not in self.domain.action_names_or_texts: raise RasaException( f"The action '{step.action}' is used in the step " - f"'{step.id}' of flow '{flow.name}', but it " + f"'{step.id}' of flow id '{flow.id}', but it " f"is not listed in the domain file. " f"You should add it to your domain file!", ) @@ -541,15 +560,15 @@ def verify_unique_flows(user_flows: List[Flow]) -> bool: cleaned_description = flow_description.translate(punctuation_table) # type: ignore[union-attr] # noqa: E501 if cleaned_description in flows_mapping.values(): raise RasaException( - f"Detected duplicate flow description for flow '{flow.name}'. " + f"Detected duplicate flow description for flow id '{flow.id}'. " f"Flow descriptions must be unique. " f"Please make sure that all flows have different descriptions." ) if flow.name in flows_mapping: raise RasaException( - f"Detected duplicate flow name '{flow.name}'. " - f"Flow names must be unique. " + f"Detected duplicate flow name '{flow.name}' for flow " + f"id '{flow.id}'. Flow names must be unique. " f"Please make sure that all flows have different names." ) @@ -579,7 +598,7 @@ def verify_predicates(user_flows: List[Flow]) -> bool: if not is_valid: raise RasaException( f"Detected invalid condition '{link.condition}' " - f"at step '{step.id}' for flow '{flow.name}'. " + f"at step '{step.id}' for flow id '{flow.id}'. " f"Please make sure that all conditions are valid." ) elif isinstance(step, CollectInformationFlowStep): @@ -599,13 +618,15 @@ def verify_predicates(user_flows: List[Flow]) -> bool: raise RasaException( f"Detected invalid rejection '{predicate}' " f"at `collect_information` step '{step.id}' " - f"for flow '{flow.name}'. " + f"for flow id '{flow.id}'. " f"Please make sure that all conditions are valid." ) return all_good def verify_flows(self) -> bool: """Checks for inconsistencies across flows.""" + logger.info("Validating flows...") + if self.flows.is_empty(): logger.warning( "No flows were found in the data files. " diff --git a/tests/test_validator.py b/tests/test_validator.py index da7462612789..a193416a0557 100644 --- a/tests/test_validator.py +++ b/tests/test_validator.py @@ -917,7 +917,7 @@ def test_verify_flow_steps_against_domain_missing_slot_in_domain( assert ( f"The slot '{missing_slot_in_domain}' is used in the step 'ask_amount' of " - f"flow 'transfer money', but it is not listed in the domain slots." + f"flow id 'transfer_money', but it is not listed in the domain slots." ) in str(e.value) @@ -980,7 +980,7 @@ def test_verify_flow_steps_against_domain_missing_action_in_domain( assert ( f"The action '{missing_action_in_domain}' is used in the step " - f"'execute_transfer' of flow 'transfer money', but it " + f"'execute_transfer' of flow id 'transfer_money', but it " f"is not listed in the domain file." ) in str(e.value) @@ -1050,11 +1050,71 @@ def test_verify_flow_steps_against_domain_missing_slot_from_set_slot_step( assert ( f"The slot '{missing_slot_in_domain}' is used in the step " - f"'set_account_type' of flow 'transfer money', " + f"'set_account_type' of flow id 'transfer_money', " f"but it is not listed in the domain slots." ) in str(e.value) +def test_verify_flow_steps_against_domain_disallowed_list_slot( + tmp_path: Path, + nlu_data_path: Path, +) -> None: + flows_file = tmp_path / "flows.yml" + with open(flows_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + flows: + order_pizza: + description: This flow lets users order their favourite pizza. + name: order pizza + steps: + - id: "ask_pizza_toppings" + collect_information: pizza_toppings + next: "ask_address" + - id: "ask_address" + collect_information: address + """ + ) + domain_file = tmp_path / "domain.yml" + with open(domain_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + intents: + - greet + slots: + pizza_toppings: + type: list + mappings: [] + address: + type: text + mappings: [] + """ + ) + importer = RasaFileImporter( + config_file="data/test_moodbot/config.yml", + domain_path=str(domain_file), + training_data_paths=[str(flows_file), str(nlu_data_path)], + ) + + validator = Validator.from_importer(importer) + user_flows = [ + flow + for flow in validator.flows.underlying_flows + if not flow.id.startswith("pattern_") + ] + + with pytest.raises(RasaException) as e: + validator.verify_flows_steps_against_domain(user_flows) + + assert ( + "The slot 'pizza_toppings' is used in the step 'ask_pizza_toppings' " + "of flow id 'order_pizza', but it is a list slot. List slots are " + "currently not supported in flows." in str(e.value) + ) + + def test_verify_unique_flows_duplicate_names( tmp_path: Path, nlu_data_path: Path, @@ -1122,8 +1182,8 @@ def test_verify_unique_flows_duplicate_names( validator.verify_unique_flows(user_flows) assert ( - f"Detected duplicate flow name '{duplicate_flow_name}'. " - f"Flow names must be unique. " + f"Detected duplicate flow name '{duplicate_flow_name}' for " + f"flow id 'recurrent_payment'. Flow names must be unique. " f"Please make sure that all flows have different names." ) in str(e.value) @@ -1196,7 +1256,7 @@ def test_verify_unique_flows_duplicate_descriptions( validator.verify_unique_flows(user_flows) assert ( - "Detected duplicate flow description for flow 'setup recurrent payment'. " + "Detected duplicate flow description for flow id 'recurrent_payment'. " "Flow descriptions must be unique. " "Please make sure that all flows have different descriptions." ) in str(e.value) @@ -1210,7 +1270,7 @@ def test_verify_predicates_invalid_rejection_if( expected_exception = ( f"Detected invalid rejection '{predicate}' " f"at `collect_information` step 'ask_account_type' " - f"for flow 'transfer money'. " + f"for flow id 'transfer_money'. " f"Please make sure that all conditions are valid." ) flows_file = tmp_path / "flows.yml"