Lambda api and unification #64

pdyba · 2022-08-25T09:46:44Z

No description provided.

CHANGELOG.md

lbz/misc.py

lbz/resource.py

lbz/exceptions.py

lbz/api_base.py

lbz/lambda_to_lambda/invoke.py

…lambdalizator into lambda_api � Conflicts: � CHANGELOG.md � lbz/exceptions.py

redlickigrzegorz

I haven't read the entire pull request yet, but the modules I commented on can be considered checked 😉

CHANGELOG.md

lbz/consts.py

lbz/dev/test.py

lbz/events/broker.py

lbz/handler_base.py

redlickigrzegorz

Ok, it looks like I wrote pretty a lot of comments for this PR 😬

I cannot force you but definitely, there are parts of the code that could be implemented in separate smaller pull requests which could speed up the work on this, especially reviewing.

lbz/exceptions.py

redlickigrzegorz · 2022-09-08T12:12:06Z

lbz/misc.py

+
+    def decorator(func: Callable) -> Callable:
+        warnings.simplefilter("always", DeprecationWarning)  # turn off filter
+        warnings.warn(


I think you put this warning at the wrong level which makes it printed no matter if the function was used or not 🤔

This being done, I think tests are required 😉

lbz/misc.py

lbz/lambdas/broker.py

tests/test_lambdas/test_broker.py

redlickigrzegorz · 2022-09-22T06:29:36Z

CHANGELOG.md

+
+
+### Version 0.5.16
+Released 2022-09-09


You should adjust this date before merging 😉

CHANGELOG.md

lbz/handlers.py

tests/test_handlers.py

tests/test_lambdas/test_enums.py

lbz/lambdas/client.py

lbz/lambdas/broker.py

redlickigrzegorz · 2022-09-22T11:04:10Z

lbz/lambdas/broker.py

+        if (op := self.event.get(self.op_key)) is None:
+            return lambda_error_response(
+                result=LambdaResult.BAD_REQUEST,
+                error="Lambda execution error: Missing 'op' field in the event.",
+            )
+        try:
+            return self.mapper[op]
+        except KeyError:
+            return lambda_error_response(
+                result=LambdaResult.BAD_REQUEST,
+                error=f"Lambda execution error: No handler implemented for operation: {op}",
+            )


The exception is more than welcome in these two situations, otherwise, they can be silently skipped by the code that uses this broker. The final decision on what to do should not be made by lbz 😉

Co-authored-by: Grzegorz Redlicki <[email protected]>

sonarqubecloud · 2022-09-27T09:54:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

No Coverage information
0.0% Duplication

redlickigrzegorz

Still, there are some comments from the past that have not been resolved yet, and now, I added new ones. Unfortunately, I'm slowly getting lost here. Too many lines of code, too many comments, and too much time spent on re-reviewing changes 😕

Can you follow my previous suggestion and split this PR? Trust me, slicing a big thing into smaller pieces always speeds up the work even if you don't see it at the first glance (#elephant-carpaccio).

We can start with three pull requests:

Adding the BaseHandler class and the tests.
Adding the deprecated decorated and the tests.
Using the above ones to improve the resource and events.

Then, we will return to the whole lambdas package (client and broker separately). Thanks to that, we will close it faster focusing on things that matter without decreasing my motivation to read your code 😉

redlickigrzegorz · 2022-09-27T15:25:35Z

lbz/handlers.py

+    def _pre_handle(self) -> None:
+        self.pre_handle()


This new method does not make sense or just I don't see that 🤔

Did you do it to stay consistent with the _post_handle() method?

redlickigrzegorz · 2022-09-27T16:12:00Z

lbz/lambdas/broker.py

+            return lambda_error_response(
+                result=LambdaResult.BAD_REQUEST,
+                error_message="Lambda execution error: Missing 'op' field in the event.",
+            )


This is the only place where lambda_error_response is used which is inconsistent with the rest of the code here and also with the comment I wrote before: #977515342 🤔

I think we need to talk about this Broker class - without good examples of what you want to achieve (tests), my review may be unfortunately inappropriate...

redlickigrzegorz · 2022-09-27T16:15:19Z

lbz/lambdas/client.py

+from lbz.lambdas.enums import LambdaResult
+from lbz.misc import get_logger
+
+from .exceptions import LambdaError


As far as I remember, we don't use relative imports in lambdalizator 🤔

Maybe we should but this is not a good pull request to start doing it in one place.

redlickigrzegorz · 2022-09-27T16:15:52Z

lbz/lambdas/__init__.py

+from .broker import LambdaBroker
+from .client import LambdaClient
+from .enums import LambdaResult, LambdaSource
+from .exceptions import LambdaError
+from .response import LambdaResponse, lambda_error_response, lambda_ok_response


The same here, you should use absolute imports instead.

redlickigrzegorz · 2022-09-27T16:18:35Z

lbz/lambdas/client.py

+        if lambda_result in allowed_error_results:
+            return response
+        if raise_if_error_resp:
+            raise LambdaError(function_name, op, lambda_result, response)  # type: ignore


You should resolve the problem instead of disabling type checking (rel. #965942904) 😉

redlickigrzegorz · 2022-09-27T16:32:10Z

lbz/lambdas/response.py

+class LambdaResponse(TypedDict, total=False):
+    result: str
+    data: Optional[Any]
+    message: Optional[str]


Optional here does not refer to the presence of a specific key - it just means that the value may be None.

Do we want to allow passing None as the message? 🤔

redlickigrzegorz · 2022-09-27T16:33:37Z

lbz/lambdas/response.py

+
+class LambdaResponse(TypedDict, total=False):
+    result: str
+    data: Optional[Any]


Also here, Any means anything (including None) 😉

Suggested change

data: Optional[Any]

data: Any

redlickigrzegorz · 2022-09-27T16:48:44Z

lbz/resource.py

-        That will be used when Authorization Header is not in place.
-        """
-        return {}
+    def _pre_handle(self) -> None:


We shouldn't overwrite the protected superclass method - things like that add to the complexity of the entire flow 😐

I know what problem this code is solving but generally speaking, you want to change the default behavior of the react() method, so, you should overwrite the react() method itself.

redlickigrzegorz · 2022-09-27T16:53:24Z

lbz/resource.py

-                raise NotFound
-            if self.method not in self._router[self.path]:
-                raise UnsupportedMethod(method=self.method)
+    def validate_path_and_method(self) -> None:


Helpers that are not needed outside the class should be protected and placed at the bottom.

However, I am not sure why you did this unnecessary refactoring now (there are already almost 1k of added lines in this PR).

redlickigrzegorz · 2022-09-27T17:11:22Z

tests/test_lambdas/test_broker.py

+        assert resp == "some_data"
+        func_1.assert_called_once_with({"y": 1})
+
+    def test_broker_responds_raises_implemented_when_event_type_is_not_recognized(self) -> None:


To make this sentence more correct:

Suggested change

def test_broker_responds_raises_implemented_when_event_type_is_not_recognized(self) -> None:

def test_broker_raises_error_when_event_type_is_not_recognized(self) -> None:

pdyba added 2 commits August 25, 2022 11:35

Lambda to Lambda API

823b30c

Extends changlog

634162b

redlickigrzegorz reviewed Sep 2, 2022

View reviewed changes

pdyba added 3 commits September 8, 2022 07:39

changes from review and discussion

0d39957

fixes

f588f0f

Merge branches 'lambda_api' and 'master' of https://github.com/pdyba/…

198ab03

…lambdalizator into lambda_api � Conflicts: � CHANGELOG.md � lbz/exceptions.py

redlickigrzegorz reviewed Sep 8, 2022

View reviewed changes

redlickigrzegorz requested changes Sep 8, 2022

View reviewed changes

changes from code review

7f2f8b6

redlickigrzegorz requested changes Sep 22, 2022

View reviewed changes

pdyba and others added 2 commits September 23, 2022 14:06

Apply suggestions from code review

f7499fa

Co-authored-by: Grzegorz Redlicki <[email protected]>

changes from code review

ef2eed8

redlickigrzegorz requested changes Sep 27, 2022

View reviewed changes

This was referenced Sep 28, 2022

adds Lambda Source enum #66

Merged

adds deprecated decorator #67

Merged

adds base handler and lambda api #68

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lambda api and unification #64

Lambda api and unification #64

pdyba commented Aug 25, 2022

redlickigrzegorz left a comment

redlickigrzegorz left a comment •

edited

Loading

redlickigrzegorz Sep 8, 2022

redlickigrzegorz Sep 8, 2022

redlickigrzegorz Sep 22, 2022

redlickigrzegorz Sep 22, 2022

sonarqubecloud bot commented Sep 27, 2022

redlickigrzegorz left a comment

redlickigrzegorz Sep 27, 2022

redlickigrzegorz Sep 27, 2022

redlickigrzegorz Sep 27, 2022

redlickigrzegorz Sep 27, 2022

redlickigrzegorz Sep 27, 2022

redlickigrzegorz Sep 27, 2022

redlickigrzegorz Sep 27, 2022

redlickigrzegorz Sep 27, 2022

redlickigrzegorz Sep 27, 2022

redlickigrzegorz Sep 27, 2022

	def test_broker_responds_raises_implemented_when_event_type_is_not_recognized(self) -> None:
	def test_broker_raises_error_when_event_type_is_not_recognized(self) -> None:

Lambda api and unification #64

Are you sure you want to change the base?

Lambda api and unification #64

Conversation

pdyba commented Aug 25, 2022

redlickigrzegorz left a comment

Choose a reason for hiding this comment

redlickigrzegorz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 27, 2022

redlickigrzegorz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redlickigrzegorz left a comment •

edited

Loading