From c590f2716963257e4c1859fde7ef59fd34694ac2 Mon Sep 17 00:00:00 2001
From: Kyle Hornberg <kyle.hornberg@gmail.com>
Date: Thu, 24 Jan 2019 08:55:40 -0600
Subject: [PATCH] Bug Fix: Properly verify and check required nested parameters

---
 src/octokit/__init__.py | 25 +++++++++++++++++++------
 src/octokit/utils.py    | 12 +++++++++++-
 tests/test_methods.py   | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+), 7 deletions(-)

diff --git a/src/octokit/__init__.py b/src/octokit/__init__.py
index dc7fd9c..4eb56f5 100644
--- a/src/octokit/__init__.py
+++ b/src/octokit/__init__.py
@@ -24,22 +24,35 @@ class Base(object):
     def _get_headers(self, method_headers):
         return dict(ChainMap(method_headers, self.headers))
 
+    def _get_required_params(self, params, cached_kwargs):
+        all_required = [k for k, v in params.items() if v.get('required')]
+        required_params = []
+        for p in all_required:
+            param = p.replace('[]', '')
+            if utils.verify_path(cached_kwargs, param.split('.')):
+                required_params.append(param)
+        return required_params
+
     def _validate(self, kwargs, params):
         cached_kwargs = dict(ChainMap(kwargs, self._attribute_cache['url']))
-        required_params = [k for k, v in params.items() if v.get('required')]
+        required_params = self._get_required_params(params, cached_kwargs)
         self._validate_required_params(required_params, cached_kwargs)
         for kwarg, value in kwargs.items():
             param_value = params.get(kwarg)
             self._validate_params(param_value, kwarg, value, required_params)
 
+    def _raise_required_parameter(self, param):
+        message = '{} is a required parameter'.format(param)
+        raise errors.OctokitParameterError(message)
+
     def _validate_required_params(self, required_params, cached_kwargs):
         for p in required_params:
-            if '.' in p:
+            try:
                 utils.walk_path(cached_kwargs, p.split('.'))
-                continue
-            if p not in cached_kwargs:  # has all required
-                message = '{} is a required parameter'.format(p)
-                raise errors.OctokitParameterError(message)
+            except KeyError:
+                self._raise_required_parameter(p)
+            except AssertionError:
+                self._raise_required_parameter(p)
 
     def _validate_params(self, param_value, kwarg, value, required_params):
         if not param_value:  # is a valid param but not necessarily required
diff --git a/src/octokit/utils.py b/src/octokit/utils.py
index b75044b..90d9243 100644
--- a/src/octokit/utils.py
+++ b/src/octokit/utils.py
@@ -19,6 +19,16 @@ def parameter_transform(params):
 
 def walk_path(obj, path):
     if len(path) == 1:
-        assert obj or obj[path[0]]
+        assert path[0] in obj
     else:
         walk_path(obj[path[0]], path[1:])
+
+
+def verify_path(obj, path):
+    if not obj and len(path) == 1:
+        return True
+    if len(path) <= 2:
+        return path[0] in obj
+    if path[0] in obj:
+        verify_path(obj[path[0]], path[1:])
+    return False
diff --git a/tests/test_methods.py b/tests/test_methods.py
index 5d21480..c3ca974 100644
--- a/tests/test_methods.py
+++ b/tests/test_methods.py
@@ -66,6 +66,44 @@ class TestClientMethods(object):
             Octokit().oauth_authorizations.create_authorization(**data)
         assert 'None is not a valid parameter for gist_id' == str(e.value)
 
+    def test_must_include_required_dictionary_sub_parameters_when_used(self, mocker):
+        mocker.patch('requests.get')
+        data = {
+            'owner': 'owner',
+            'repo': 'repo',
+            'name': 'name',
+            'head_sha': 'master',
+            'output': {}
+        }
+        with pytest.raises(errors.OctokitParameterError) as e:
+            Octokit().checks.create(**data)
+        assert 'output.title is a required parameter' == str(e.value)
+        data.update({'output': {'title': 'blah'}})
+        with pytest.raises(errors.OctokitParameterError) as e:
+            Octokit().checks.create(**data)
+        assert 'output.summary is a required parameter' == str(e.value)
+        data.update({'output': {'title': 'blah', 'summary': 'that'}})
+        Octokit().checks.create(**data)
+
+    def test_must_include_required_list_sub_parameters_when_used(self, mocker):
+        mocker.patch('requests.get')
+        data = {
+            'owner': 'owner',
+            'repo': 'repo',
+            'name': 'name',
+            'head_sha': 'master',
+            'actions': []
+        }
+        with pytest.raises(errors.OctokitParameterError) as e:
+            Octokit().checks.create(**data)
+        assert 'actions.label is a required parameter' == str(e.value)
+        data.update({'actions': {'label': 'blah'}})
+        with pytest.raises(errors.OctokitParameterError) as e:
+            Octokit().checks.create(**data)
+        assert 'actions.description is a required parameter' == str(e.value)
+        data.update({'actions': {'label': 'blah', 'description': 'x', 'identifier': 'a'}})
+        Octokit().checks.create(**data)
+
     def test_use_default_parameter_values(self, mocker):
         mocker.patch('requests.get')
         headers = {'Content-Type': 'application/json', 'accept': 'application/vnd.github.v3+json'}