Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed trunk issues in conftest #44

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions app/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,10 @@ def user_mocked_info(db_mocked_app_client):
"password": "pass_mock",
}
response = db_mocked_app_client.post(url="user/register", json=user_info)
assert 201 == response.status_code

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't change the behavior of the code.

The assert statement does exactly the same as you made - check it: https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement

Copy link
Author

@abdur-n-tr abdur-n-tr May 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I have drafted this PR.

This change was made to fix trunk issues so it should not do any change in the behaviour of the code as discussed here: #34

But if we want to keep assert as it is currently being used, we can configure the trunk to ignore assert statements. What do you say?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, do we really to fix all trunk issues with current trunk default configuration as it require a lot of changes to do in the code, For example, if I fix trunk issue in app/core/db/migrations/models.py, then I have to remove the below both imports in this file as they are not being used anywhere

from app.core.models.record import Record
from app.services.user.models import RevokedToken, User

trunk raise issues in this file are the following,

app/core/db/migrations/models.py:1:1
 1:1   high  'app.core.models.record.Record' imported but unused          flake8/F401
 1:36  high  `app.core.models.record.Record` imported but unused          ruff/F401  
 2:1   high  'app.services.user.models.User' imported but unused          flake8/F401
 2:1   high  'app.services.user.models.RevokedToken' imported but unused  flake8/F401
 2:38  high  `app.services.user.models.RevokedToken` imported but unused  ruff/F401  
 2:52  high  `app.services.user.models.User` imported but unused          ruff/F401  

Please let me know your thoughts on how we want to go for these issues fix? fix all as per truck default recommendations or we need to modify trunk?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, I got it now, sorry!! so, never mind what I said

I'm in trouble with trunk 😭

Screenshot 2024-05-03 at 10 17 54 AM

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my guess is, that we definitely need to fix most of them. but of course, some of them don't make any sense to be fixed, so we can ignore using the #noqa notation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will try to resolve as much as possible and then submit PR for review

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In app/core/admin/models.py, on line 56: model: object = await self.get_object_for_edit(pk), can we replace the model type from object to ModelCore from app/core/models/base.py

This is a high severity issue in the trunk.

I am confused because I don't fully understand the codebase at this point but I am trying to understand it gradually.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroimpulcetto I commit the trunk issues fix, looking forward to review.

if response.status_code != 201:
raise AssertionError(
f"Expected status code 201, but got {response.status_code}"
)

# Login
response = db_mocked_app_client.post(
Expand All @@ -88,7 +91,10 @@ def user_mocked_info(db_mocked_app_client):
},
)

assert 200 == response.status_code
if response.status_code != 200:
raise AssertionError(
f"Expected status code 200, but got {response.status_code}"
)

user_info["access_token"] = response.json()["access_token"]
user_info["refresh_token"] = response.json()["refresh_token"]
Expand Down
2 changes: 2 additions & 0 deletions app/core/admin/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,5 @@ async def authenticate(self, request: Request) -> RedirectResponse | None:
)

request.session.update({"username": username})

return None
3 changes: 2 additions & 1 deletion app/core/admin/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from sqladmin._queries import Query
from starlette.requests import Request

from app.core.models.base import ModelCore
from app.core.models.record import Record

source = "ADMIN"
Expand Down Expand Up @@ -52,7 +53,7 @@ async def update_model(self, request: Request, pk: str, data: dict) -> Any:
async def delete_model(self, request: Request, pk: Any) -> None:
"""Overwrite default delete method for
safe_delete/reverse_delete methods"""
model: object = await self.get_object_for_edit(pk)
model: ModelCore = await self.get_object_for_edit(pk)
if model.deleted:
deleted = False
action_description = "REVERSE_DELETE"
Expand Down
1 change: 0 additions & 1 deletion app/core/db/migrations/env.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

# add your model's MetaData object here
# for 'autogenerate' support
from app.core.db.migrations.models import *

meta = MetaData()
for table in SQLModel.metadata.tables.values():
Expand Down
26 changes: 17 additions & 9 deletions app/core/db/session.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from contextvars import ContextVar
from typing import Dict, Optional, Union
from typing import Any, Dict, Optional, Union

from sqlalchemy.engine import Engine
from sqlalchemy.engine.url import URL
Expand All @@ -13,6 +13,8 @@
from starlette.requests import Request
from starlette.types import ASGIApp

from app.core.models.base import ModelCore


class sessionmaker(sessionmaker_):
def __init__(self, *args, **kwargs):
Expand All @@ -31,8 +33,8 @@ def __init__(
app: ASGIApp,
db_url: Optional[Union[str, URL]] = None,
custom_engine: Optional[Engine] = None,
engine_args: Dict = None,
session_args: Dict = None,
engine_args: Optional[Dict[Any, Any]] = None,
session_args: Optional[Dict[Any, Any]] = None,
commit_on_exit: bool = False,
):
super().__init__(app)
Expand Down Expand Up @@ -106,7 +108,9 @@ def session(self) -> Session:
raise MissingSessionError
return session

def get_one(self, model: any, key: any, value: any) -> object:
def get_one(
self, model: ModelCore, key: Any, value: Any
) -> Optional[ModelCore]:
try:
statement = select(model).where(
key == value, model.deleted == False
Expand All @@ -116,7 +120,9 @@ def get_one(self, model: any, key: any, value: any) -> object:
except NoResultFound:
return None

def get_all(self, model: any, offset: int, limit: int, order_by: any):
def get_all(
self, model: ModelCore, offset: int, limit: int, order_by: Any
):
"""Get all items excluding delete ones"""
statement = (
select(model)
Expand All @@ -128,20 +134,20 @@ def get_all(self, model: any, offset: int, limit: int, order_by: any):
result = self.session.exec(statement).all()
return result

def update(self, item: any) -> object:
def update(self, item: Any) -> object:
"""Create or modify"""
self.session.add(item)
self.session.commit()
self.session.refresh(item)
return item

def delete(self, item: any) -> bool:
def delete(self, item: Any) -> bool:
"""TODO: Add return False when exceptions are observed"""
self.session.delete(item)
self.session.commit()
return True

def count(self, model: any) -> int:
def count(self, model: Any) -> int:
result = len(
self.session.exec(
select(model).where(model.deleted == False)
Expand All @@ -152,7 +158,9 @@ def count(self, model: any) -> int:

class DBSession(metaclass=DBSessionMeta):
def __init__(
self, session_args: Dict = None, commit_on_exit: bool = False
self,
session_args: Optional[Dict[Any, Any]] = None,
commit_on_exit: bool = False,
):
self.token = None
self.session_args = session_args or {}
Expand Down
5 changes: 3 additions & 2 deletions app/core/models/base.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from datetime import datetime
from typing import Any, Optional

from sqlmodel import Field, SQLModel

Expand Down Expand Up @@ -64,7 +65,7 @@ def delete(
return True

@classmethod
def get_one(cls, value: any, key: any = None) -> any:
def get_one(cls, value: Any, key: Any = None) -> Optional["ModelCore"]:
"""
Get one item based in key/value
- value: the value of the field
Expand All @@ -75,7 +76,7 @@ def get_one(cls, value: any, key: any = None) -> any:
return db.get_one(cls, key=key, value=value)

@classmethod
def get_all(cls, offset: int = 0, limit: int = 100, order_by: any = None):
def get_all(cls, offset: int = 0, limit: int = 100, order_by: Any = None):
"""
Return a list of items
Params:
Expand Down
21 changes: 15 additions & 6 deletions app/core/test/admin/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ async def test_admin_auth_login_successful(admin_auth):

result = await admin_auth.login(mock_request)

assert result is True
if result is not True:
raise AssertionError("Expected result to be True, but it was not.")


@pytest.mark.asyncio
Expand All @@ -53,7 +54,8 @@ async def test_admin_auth_login_failed(admin_auth):

result = await admin_auth.login(mock_request)

assert result is False
if result is not True:
raise AssertionError("Expected result to be True, but it was not.")


@pytest.mark.asyncio
Expand All @@ -62,7 +64,8 @@ async def test_admin_auth_logout(admin_auth):

result = await admin_auth.logout(mock_request)

assert result is True
if result is not True:
raise AssertionError("Expected result to be True, but it was not.")


@pytest.mark.asyncio
Expand All @@ -71,7 +74,8 @@ async def test_admin_auth_authenticate_valid_token(admin_auth, valid_token):

result = await admin_auth.authenticate(mock_request)

assert result is None
if result is not None:
raise AssertionError("Expected result to be None, but it was not.")


@pytest.mark.asyncio
Expand All @@ -80,5 +84,10 @@ async def test_admin_auth_authenticate_invalid_token(admin_auth):

result = await admin_auth.authenticate(mock_request)

assert isinstance(result, RedirectResponse)
assert result.status_code == 302
if not isinstance(result, RedirectResponse):
raise AssertionError(
"Expected result to be an instance of RedirectResponse."
)

if result.status_code != 302:
raise AssertionError("Expected status code to be 302, but it was not.")
42 changes: 30 additions & 12 deletions app/core/test/auth/test_functions.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,28 @@ def test_verify_password_ok(self):
password_plain = "test"
password_hashed = hash_password(password_plain)

assert verify_password(password_plain, password_hashed)
if not verify_password(password_plain, password_hashed):
raise AssertionError("Password verification failed.")

def test_verify_password_ko(self):
password_plain = "test"
password_hashed = hash_password(password_plain)

assert not verify_password("incorrect_password", password_hashed)
if verify_password("incorrect_password", password_hashed):
raise AssertionError("Password verification should fail.")


class TestToken(TestCase):
def test_create_token(self):
access_token = create_access_token(username="test")
assert str == type(access_token)
if type(access_token) != str:
raise AssertionError("Access token should be of type str.")

def test_get_current_user_ok(self):
access_token = create_access_token(username="test")
username = get_current_user(access_token)
assert "test" == username
if username != "test":
raise AssertionError("Username should be 'test'.")

def test_get_current_user_no_username(self):
access_token = create_jwt_token(
Expand All @@ -59,46 +63,60 @@ def test_get_current_user_expired_signature(self):
def test_get_current_admin_ok(self):
access_token = create_access_token(username="test")
username = get_current_admin(access_token)
assert "test" == username
if username != "test":
raise AssertionError("Username should be 'test'.")

def test_get_current_admin_no_username(self):
access_token = create_jwt_token(
data={}, expiration_delta=timedelta(minutes=30)
)
username = get_current_admin(access_token)
assert username is None
if username is not None:
raise AssertionError("Username should be None.")

def test_get_current_admin_invalid_signature(self):
username = get_current_admin("incorrect_token")
assert username is None
if username is not None:
raise AssertionError("Username should be None.")

def test_get_current_admin_expired_signature(self):
access_token = create_jwt_token(
data={}, expiration_delta=timedelta(minutes=-30)
)
username = get_current_admin(access_token)
assert username is None
if username is not None:
raise AssertionError("Username should be None.")

def test_verify_refresh_token_ok(self):
payload = {"scopes": "refresh_token"}
token = create_jwt_token(
data=payload, expiration_delta=timedelta(minutes=30)
)
assert payload == verify_refresh_token(token)
if payload != verify_refresh_token(token):
raise AssertionError(
"Payload should match the result of verify_refresh_token."
)

def test_verify_refresh_token_no_scopes(self):
payload = {}
token = create_jwt_token(
data=payload, expiration_delta=timedelta(minutes=30)
)
assert verify_refresh_token(token) is None
if payload != verify_refresh_token(token):
raise AssertionError(
"Payload should match the result of verify_refresh_token."
)

def test_verify_refresh_token_invalid_signature(self):
assert verify_refresh_token("invalid_token") is None
if verify_refresh_token("invalid_token") is not None:
raise AssertionError("Invalid token should return None.")

def test_verify_refresh_token_expired_signature(self):
payload = {"scopes": "refresh_token"}
token = create_jwt_token(
data=payload, expiration_delta=timedelta(minutes=-30)
)
assert verify_refresh_token(token) is None
if verify_refresh_token(token) is not None:
raise AssertionError(
"Refresh token verification should return None."
)
3 changes: 2 additions & 1 deletion app/core/test/db/test_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@

def test_get_engine():
engine = get_engine()
assert settings.database_url == str(engine.url)
if settings.database_url != str(engine.url):
raise AssertionError("Database URL does not match engine URL.")
12 changes: 7 additions & 5 deletions app/core/test/db/test_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,10 @@ def test_db_session_constructor(self):
self.assertIsInstance(db_session, DBSessionMeta)

# Exit rollback
with self.assertRaises(Exception):
with self.assertRaises(RuntimeError):
with DBSession() as db_session:
self.assertIsNotNone(db_session)
raise Exception("Simulated error")
raise RuntimeError("Simulated error")

# Exit commit
with DBSession(commit_on_exit=True) as db_session:
Expand All @@ -91,15 +91,17 @@ def _db_mocked(self, db_mocked):
def test_session_ok(self):
db_test = DBSession
session = db_test.session
assert session is not None
if session is None:
raise AssertionError("Session should not be None.")

def test_session_missing(self):
with patch(
"app.core.db.session._session", MagicMock()
) as mock_session, self.assertRaises(MissingSessionError):
) as mock_session:
mock_session.get.return_value = None
db_test = DBSession
db_test.session
session = db_test.session
self.assertRaises(MissingSessionError, session)

@patch("app.core.db.session._Session", None)
def test_session_no_initialised(self):
Expand Down
Loading