DB and API Refactors #99

OdyAsh · 2024-12-08T12:05:07Z

feat: loguru, cors_ok & DB Code Refactors

Also: Zrok Notes, Meta API Notes

Regarding ansari_db.py's code refactors:

the execute_query() might initially seem complicated, but it becomes obvious once you see how it's called in different functions.
- However, if it's still too complicated, I'll revert it 👍
- I've added this function to avoid code duplications, for example when adding debug logs (which facilitates the process of understanding the DB logic flow).
- Maybe in the future, I'll add try/except wrapper as well to this function, since this repetitive error-handling blocks take up code size in the rest of ansari_db.py's functions.

Regarding cors_ok logic in main_api.py:

Small refactors to unnest code by one level :]

Regarding Zrok:

I've added these comments to freeze the callback URL used in the testing app of Meta's developer page. Now, any future WhatsApp tester needs only to know the ZROK_SHARE_TOKEN, instead of changing the callback URL each time they want to debug/test new WhatsApp features.

Regarding Loguru:

Benefits over standard logging and migration notes are mentioned here.

Also: Zrok Notes, Meta API Notes

waleedkadous

Once we've addressed the model breakage good to go.

There are some fuzzy bits of feedback here; don't feel I know Python well enough to be sure of myself.

waleedkadous · 2024-12-16T22:13:43Z

.env.example

+#   a new one will have to be generated, then added to Meta's callback URL of the *testing* app
+#   (Noting that the *production* app's callback URL will be different anyway, so the 3rd party won't be able to access that app)
+#   (but we still don't want random calls to be made to our testing app, so that's why we'll still have to change an exposed token :])
+


Love the extensive comments.

Thank you! I was thinking of other ways to document these comments externally, but until we figure something out, will stick to inline commenting as I do think these should help newcomers .

waleedkadous · 2024-12-16T22:14:34Z

docs/structure_of_api_responses/ansari_website_api_structure_of_a_request_sent_using_zrok.json

+		},
+		"http_version": "1.1",
+		"server": ["127.0.0.1", 8000], // When running locally
+		"client": ["127.0.0.1", 11563], // The port here changes dynamically


Is there a better way of handling this?

If "this" refers to the "port changes dynamically" comment, then I don't know...

To be honest, I don't even know why this port changes ... even though I've bounded Ansari frontend with port 3000 ... unless "client" here refers to something else, since origin does indeed have port 3000 ...

waleedkadous · 2024-12-16T22:20:35Z

src/ansari/ansari_db.py

+                or only after all of them are executed.
+
+        Returns:
+            Union[Optional[List], List[Optional[List]]]:


What if we simplified this a bit and made this always return a list (even if there is only one element)?

I am not sure so this is only a suggestion, but having multiple return types feels slightly clumsy to me.

Good point, will adjust it 👍

Also, I love the attached screenshot; any attempts to explain reasoning/advantages/disadvantages are always appreciated :]

waleedkadous · 2024-12-16T22:22:27Z

src/ansari/ansari_db.py

+                which_fetch = [which_fetch] * len(query)
+
+        caller_function_name = inspect.stack()[1].function
+        logger.debug(f"Function {caller_function_name}() \nis running queries: \n{query} \nwith params: \n{params}")


What happens if this is called from an object method? Just curious.

I don't quite get your question... if I understand correctly, an "object's method" is like account_exists() method in Ansari class (object) ... all other functions that are in this file are also methods which I call in it the `_execute_query() method.

If you're asking what this code snippet does in general... then the answer is this: it fetches the name of the function/method that called this _execute_query() method (e.g., "account_exists", etc.)

Hopefully I understood your question correctly 🙌

waleedkadous · 2024-12-16T22:24:43Z

src/ansari/ansari_db.py

@@ -109,13 +112,95 @@ def _get_token_from_request(self, request: Request) -> str:
                detail="Authorization header is malformed",
            )

+    def _execute_query(


You realize we are taking baby steps towards building an ORM, right ? :-)

Wonder if we should just fully commit to an ORM.

Totally slipped my mind, I don't know how!

I don't mind later changing this code to support ORM ... my main motivation on why I created this method, was that I found duplications in other methods, so I wanted to refactor, + add logs ... but if we're headed towards ORM, no problems at all 👍

waleedkadous · 2024-12-16T22:40:50Z

src/ansari/ansari_db.py

+                        result = cur.fetchall()
+
+                    # Remove possible SQL comments at the start of the q variable
+                    q = re.sub(r"^\s*--.*\n", "", q, flags=re.MULTILINE)


This feels a bit weird to me. Why is this necessary? Why is it two dashes instead of the normal 3? Does pysql not handle comments?

I can adjust the regex to activate on 2 or 3 ISA ... actually, this regex isn't that necessary 😅 , as we don't have any SQL statements starting with dashes in our code, but I will adjust the regex nonetheless 🙌

waleedkadous · 2024-12-16T22:42:12Z

src/ansari/config.py

 from functools import lru_cache
 from pathlib import Path
 from typing import Literal

 from pydantic import DirectoryPath, Field, PostgresDsn, SecretStr, field_validator
 from pydantic_settings import BaseSettings, SettingsConfigDict

-# Can't use get_logger() here due to circular import
-logger = logging.getLogger(__name__)
+# # Can't use get_logger() here due to circular import


Unnecessary #?

Nice catch! Will replace "# #" with "#" 👍

waleedkadous · 2024-12-16T22:42:42Z

src/ansari/util/fastapi_helpers.py

+
+
+# Defined in a separate file to avoid circular imports between main_*.py files
+def validate_cors(request: Request, settings: Settings = Depends(get_settings)) -> bool:


Like this! Are there othetr helpers that should be moved in here?

Thanks!

Whenever I find code that is used in multiple files (e.g., validate_cors is used in main_api and main_whatsapp), I'll move them here, but now I can't think of any.

waleedkadous · 2024-12-16T22:43:02Z

src/ansari/util/fastapi_helpers.py

+# Defined in a separate file to avoid circular imports between main_*.py files
+def validate_cors(request: Request, settings: Settings = Depends(get_settings)) -> bool:
+    try:
+        logger.info(f"Headers of raw request are: {request.headers}")


All headers feel more like a "debug" level log.

Makes sense, will adjust ISA 👍

waleedkadous · 2024-12-17T03:11:35Z

Oh, you're right. Sorry. False alarm.

…

On Tue, Dec 17, 2024 at 2:09 PM Ashraf Haress ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In docs/structure_of_api_responses/ansari_website_api_structure_of_a_request_sent_using_zrok.json <#99 (comment)> : > @@ -0,0 +1,76 @@ +{ + "scope": { + "type": "http", + "asgi": { + "version": "3.0", + "spec_version": "2.4" + }, + "http_version": "1.1", + "server": ["127.0.0.1", 8000], // When running locally + "client": ["127.0.0.1", 11563], // The port here changes dynamically If "this" refers to the "port changes dynamically" comment, then I don't know... To be honest, I don't even know why this port changes ... even though I've bounded Ansari frontend with port 3000 ... unless "client" here refers to something else, since origin does indeed have <https://github.com/ansari-project/ansari-backend/blob/2f9eeb223bb6ad74ce0411128a20c47aecd77368/docs/structure_of_api_responses/ansari_website_api_structure_of_a_request_sent_using_zrok.json#L28> port 3000 ... — Reply to this email directly, view it on GitHub <#99 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAUXGUERVVAOIL2OBSYI4ID2F6IVXAVCNFSM6AAAAABTHFFRVCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMBXG42TMOJTGI> . You are receiving this because you commented.Message ID: ***@***.***>

Based on this PR: ansari-project#99

OdyAsh added 4 commits December 8, 2024 13:25

feat: loguru, cors_ok & DB Code Refactors

bd417e7

Also: Zrok Notes, Meta API Notes

refactor(ansari_db.py): _execute_query()

041a75a

style(main_api.py): Ruff comment formatting

2d30e2a

fix(fastapi_helpers.py): log request headers only

2f9eeb2

waleedkadous approved these changes Dec 16, 2024

View reviewed changes

_execute_query & Loguru Updates based on PR

268618d

Based on this PR: ansari-project#99

OdyAsh merged commit 2c4c3d8 into ansari-project:main Dec 19, 2024
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DB and API Refactors #99

DB and API Refactors #99

OdyAsh commented Dec 8, 2024

waleedkadous left a comment

waleedkadous Dec 16, 2024

OdyAsh Dec 17, 2024

waleedkadous Dec 16, 2024

OdyAsh Dec 17, 2024

waleedkadous Dec 16, 2024

OdyAsh Dec 17, 2024

waleedkadous Dec 16, 2024

OdyAsh Dec 17, 2024

waleedkadous Dec 16, 2024

OdyAsh Dec 17, 2024

waleedkadous Dec 16, 2024

OdyAsh Dec 17, 2024

waleedkadous Dec 16, 2024

OdyAsh Dec 17, 2024 •

edited

Loading

waleedkadous Dec 16, 2024

OdyAsh Dec 17, 2024

waleedkadous Dec 16, 2024

OdyAsh Dec 17, 2024

waleedkadous commented Dec 17, 2024 via email



		# Defined in a separate file to avoid circular imports between main_*.py files
		def validate_cors(request: Request, settings: Settings = Depends(get_settings)) -> bool:

DB and API Refactors #99

DB and API Refactors #99

Conversation

OdyAsh commented Dec 8, 2024

waleedkadous 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OdyAsh Dec 17, 2024 • 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

waleedkadous commented Dec 17, 2024 via email

OdyAsh Dec 17, 2024 •

edited

Loading