Repository: jina-ai/serve
Pull request: fix: add check that jaml parses to correct class
Fixes silent error when invalid YAML is passed as an Executor


Problem statement:
Parsing invalid Executor YAML does not fail properly
If a `.yml` that does not define an Executor is passed to `uses=`, it will be parsed into a dict instead of an actual Executor. This dict is then treated like an Executor in the worker runtime ~~, which then fails of course, leading to a cryptic error message.~~

The behaviour has changed (I believe as a consequence of exec endpoint discovery), now no error is given at all.

**To Reproduce:**

Define invalid YAML:

`invalid.yml`:
```yaml
invalidkey: hello
```

Then try to use it:

```python
f = Flow().add(
        uses='invalid.yml',
    )

with f:
  f.index(inputs=[Document()])
```

Hints from issue discussion:
Please put less priority to this ticket. Priority on reliability and robustness should go in the `communication layer`

Commit message: fix: add check that jaml parses to correct class

Patch to review:
diff --git a/jina/jaml/__init__.py b/jina/jaml/__init__.py
index 0188323f26e82..124bba634719b 100644
--- a/jina/jaml/__init__.py
+++ b/jina/jaml/__init__.py
@@ -749,7 +749,12 @@ def _delitem(
                 # revert yaml's tag and load again, this time with substitution
                 tag_yml = JAML.unescape(JAML.dump(no_tag_yml))
             # load into object, no more substitute
-            return JAML.load(tag_yml, substitute=False, runtime_args=runtime_args)
+            obj = JAML.load(tag_yml, substitute=False, runtime_args=runtime_args)
+            if not isinstance(obj, cls):
+                raise BadConfigSource(
+                    f'Can not construct {cls} object from {source}. Source might be an invalid configuration.'
+                )
+            return obj
 
     @classmethod
     def _override_yml_params(cls, raw_yaml, field_name, override_field):
diff --git a/tests/unit/jaml/invalid.yml b/tests/unit/jaml/invalid.yml
new file mode 100644
index 0000000000000..58f4979c547d8
--- /dev/null
+++ b/tests/unit/jaml/invalid.yml
@@ -0,0 +1 @@
+invalidkey: hello
\ No newline at end of file
diff --git a/tests/unit/jaml/test_type_parse.py b/tests/unit/jaml/test_type_parse.py
index cc7378843a970..1433f38c55e6d 100644
--- a/tests/unit/jaml/test_type_parse.py
+++ b/tests/unit/jaml/test_type_parse.py
@@ -2,9 +2,10 @@
 
 import pytest
 
-from jina.serve.executors import BaseExecutor
+from jina import Flow, __default_executor__, requests
+from jina.excepts import BadConfigSource
 from jina.jaml import JAML, JAMLCompatible
-from jina import __default_executor__, requests, Flow
+from jina.serve.executors import BaseExecutor
 
 
 class MyExecutor(BaseExecutor):
@@ -179,3 +180,11 @@ def test_parsing_brackets_in_envvar():
         assert b['executors'][0]['env']['var2'] == 'a'
         assert b['executors'][0]['env']['var3'] == '{"1": "2"}-a'
         assert b['executors'][0]['env']['var4'] == '-{"1": "2"}'
+
+
+def test_exception_invalid_yaml():
+    with pytest.raises(BadConfigSource):
+        BaseExecutor.load_config('./invalid.yml')
+
+    with pytest.raises(BadConfigSource):
+        Flow.load_config('./invalid.yml')


Code review comments:
diff_hunk: @@ -179,3 +180,11 @@ def test_parsing_brackets_in_envvar():
         assert b['executors'][0]['env']['var2'] == 'a'
         assert b['executors'][0]['env']['var3'] == '{"1": "2"}-a'
         assert b['executors'][0]['env']['var4'] == '-{"1": "2"}'
+
+
+def test_exception_invalid_yaml():
+    with pytest.raises(BadConfigSource):
+        BaseExecutor.load_config('./invalid.yml')
path: tests/unit/jaml/test_type_parse.py
text: @user1:
do not use `./invalid.yml`, use some `cur_dir` to make sure test is runnable from anywhere