Create task 4 by SofaD8 · Pull Request #260 · mate-academy/py-fastapi-homework-4-task

SofaD8 · 2026-02-12T16:36:45Z

No description provided.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are several critical issues that will prevent the profile feature and tests from working correctly. First, ProfileCreateSchema in src/schemas/profiles.py does not define an avatar field, does not accept it in as_form, and does not validate it with validate_image; this means the endpoint cannot properly receive or validate the uploaded image file as required. Second, in src/routes/profiles.py the token-related exceptions are imported from the wrong module (exceptions), which is likely to cause an ImportError at runtime and break token handling. Third, the S3/MinIO upload logic is fragile: the extension extraction is not robust, the file stream is passed without ensuring correct type/position, and it’s unclear whether upload_file should be awaited or receive bytes, so uploads may fail or return no URL. Finally, there is no explicit check that current_user exists and is active, so a token referencing a deleted user could incorrectly result in a 403 instead of the required 401 User not found or not active.

To move this forward: (1) add avatar: UploadFile (or Optional[UploadFile]) to ProfileCreateSchema, accept it in as_form with avatar: UploadFile = File(...), and add a @field_validator('avatar') that calls validate_image; (2) update the import to the correct exceptions module (for example from exceptions.security import TokenExpiredError, InvalidTokenError); (3) adjust the S3 client usage so you normalize the extension with rsplit('.', 1)[-1].lower(), reset/read the file appropriately, and await the call if the client is async, making sure it returns the final URL; and (4) explicitly check current_user and its is_active flag and return 401 User not found or not active. when the token’s user does not exist or is inactive. As smaller improvements, avoid raising a bare Exception when upload returns no URL—either log and map a storage-specific error to the existing 500 response—and clean up unused imports or types. You’ve implemented many of the core checks and structure correctly; once these blocking issues are fixed, your solution should be much closer to meeting all the task requirements.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2026-02-12T16:42:39Z

src/routes/profiles.py

+from config.dependencies import get_s3_storage_client, get_jwt_auth_manager
+from security.interfaces import JWTAuthManagerInterface
+from security.http import get_token
+from exceptions import TokenExpiredError, InvalidTokenError


The import path for token-related exceptions looks incorrect. The project defines security-related exceptions under the exceptions package (e.g. exceptions/security.py). Importing directly from exceptions will likely raise ImportError at runtime. Update this to the module where the exceptions are implemented (for example from exceptions.security import TokenExpiredError, InvalidTokenError or the correct path in this codebase). See the project exceptions layout for reference.

mateacademy-ai-mentor · 2026-02-12T16:42:39Z

src/routes/profiles.py

+    result = await db.execute(query)
+    users = {u.id: u for u in result.scalars().all()}
+
+    current_user = users.get(current_user_id)


current_user can be None if the JWT subject corresponds to a deleted/inactive user. Right now you only check the target_user for existence/active status. Consider adding an explicit check of current_user and its is_active and return 401 User not found or not active. when appropriate — otherwise you may return 403 for a token that references a non-existent user. This will make behavior consistent and clearer for tests.

mateacademy-ai-mentor · 2026-02-12T16:42:39Z

src/routes/profiles.py

+        from validation import validate_image
+        validate_image(avatar)
+
+        file_extension = avatar.filename.split(".")[-1]


Extracting file extension with avatar.filename.split(".")[-1] is fragile: if the filename has no extension this returns the full filename; casing is not normalized. Use rsplit('.', 1) and lowercase the extension (e.g. ext = avatar.filename.rsplit('.', 1)[-1].lower()), and ensure validate_image covers missing-extension cases. This improves robustness of S3 path naming.

mateacademy-ai-mentor · 2026-02-12T16:42:39Z

src/routes/profiles.py

+
+        file_extension = avatar.filename.split(".")[-1]
+        s3_path = f"avatars/{user_id}_avatar.{file_extension}"
+        avatar_url = s3_client.upload_file(avatar.file, s3_path)


Ensure the S3 upload call matches the storage client interface. Common problems to address here:

If upload_file is async you must await it.

Many storage clients expect bytes or a stream starting at position 0, so do avatar.file.seek(0) or data = await avatar.read() (or avatar.file.read() for sync) before upload.

Confirm upload_file returns the final public URL expected by the response. If it raises storage-specific exceptions, catch them explicitly and map to the 500 error. The current code calls it synchronously and may fail if the client requires await or different input types.

mateacademy-ai-mentor · 2026-02-12T16:42:39Z

src/routes/profiles.py

+        s3_path = f"avatars/{user_id}_avatar.{file_extension}"
+        avatar_url = s3_client.upload_file(avatar.file, s3_path)
+        if not avatar_url:
+             raise Exception("S3 upload returned no URL")


Raising a bare Exception("S3 upload returned no URL") hides the original storage error and makes debugging harder. Prefer catching and mapping specific storage exceptions (there's a storage exceptions module in the project) or at least log the original exception before returning the 500 message required by the spec. This will help diagnose real upload issues during tests and runtime.

mateacademy-ai-mentor · 2026-02-12T16:42:39Z

src/schemas/profiles.py


-from fastapi import UploadFile, Form, File, HTTPException
-from pydantic import BaseModel, field_validator, HttpUrl
+from fastapi import UploadFile, Form, File


UploadFile and File are imported but never used in the schema. The schema must accept an avatar file (UploadFile) and as_form must use File(...) so multipart uploads work. Consider adding avatar: UploadFile to the model and avatar: UploadFile = File(...) to as_form so the endpoint can receive files.

mateacademy-ai-mentor · 2026-02-12T16:42:39Z

src/schemas/profiles.py


-# Write your code here
+
+class ProfileCreateSchema(BaseModel):


ProfileCreateSchema is missing the avatar field. Requirement: the schema must allow submission of avatar (JPG/JPEG/PNG, max 1MB). Add an avatar field (e.g. avatar: UploadFile or avatar: Optional[UploadFile]) to this class.

mateacademy-ai-mentor · 2026-02-12T16:42:39Z

src/schemas/profiles.py

+    info: str
+
+    @classmethod
+    def as_form(


as_form helper does not accept an avatar parameter. To support file uploads via multipart/form-data you must add avatar: UploadFile = File(...) to the method signature and include it appropriately when constructing/handling the model.

mateacademy-ai-mentor · 2026-02-12T16:42:39Z

src/schemas/profiles.py

+            raise ValueError("Info cannot be empty or consist only of spaces.")
+        return v
+
+


There is no validator for avatar. You imported validate_image but never used it. Add a @field_validator('avatar') that calls validate_image to enforce allowed types (JPG/JPEG/PNG) and the max size (1MB), raising ValueError on invalid files as required by the task.

mateacademy-ai-mentor · 2026-02-12T16:42:39Z

src/schemas/profiles.py

@@ -1,7 +1,8 @@
 from datetime import date
+from typing import Optional


Optional is imported but not used. If you intend avatar to be optional, use Optional[UploadFile]; otherwise remove the unused import to keep imports accurate.

mateacademy-ai-mentor

Review Summary

I am requesting changes because several core requirements are not yet met and they will cause tests and functionality to fail. First, the account routes in src/routes/accounts.py do not inject the email notificator (get_accounts_email_notificator) or use BackgroundTasks to send activation and password-reset emails in register_user, activate_account, request_password_reset_token, and reset_password, which is explicitly required and is why the email E2E tests are failing. Second, the profile flow is incomplete: ProfileCreateSchema in src/schemas/profiles.py is missing the avatar: UploadFile field, as_form does not accept avatar, and there are no @field_validator methods calling the provided validation helpers for names, gender, birth date, info, and avatar image; right now this logic is incorrectly placed inside the route in src/routes/profiles.py. Third, the avatar upload currently stores or returns the raw S3 key instead of the public URL, and the unused avatar_url plus long line in profiles.py are breaking lint and the storage E2E test; you should obtain avatar_url = await s3_client.get_file_url(s3_path) and ensure the response’s avatar field exposes that URL. Finally, src/security/passwords.py incorrectly encodes strings to bytes before passing them to pwd_context.hash/verify, and truncation should be applied on the same str (or consistently on UTF-8 bytes) to avoid runtime errors and mismatched bcrypt length handling. Once you move validation into the schema, wire up email sending with BackgroundTasks, return the correct avatar URL, and fix the password hashing/verification types, your solution will align with the task requirements and should pass the tests.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2026-02-12T19:02:20Z

src/routes/accounts.py

 from sqlalchemy.orm import joinedload

-from config import get_jwt_auth_manager, get_settings, BaseAppSettings, get_accounts_email_notificator
+from config import get_jwt_auth_manager, get_settings, BaseAppSettings


The project requires account controllers to send email notifications using the existing EmailSender via dependency injection and BackgroundTasks. Line 10 currently imports DI helpers from config and does not provide the get_accounts_email_notificator dependency. Add/import the notificator provider and use it plus BackgroundTasks in register_user, activate_account, request_password_reset_token, and reset_password to schedule emails (activation request, activation complete, password reset request, password reset complete). Also ensure you generate valid URLs for activation and reset and pass them to background_tasks.add_task(...). Example: background_tasks.add_task(email_sender.send_activation_request_email, str(new_user.email), activation_link).

mateacademy-ai-mentor · 2026-02-12T19:02:20Z

src/routes/profiles.py

+async def create_profile(
+    user_id: int,
+    token: str = Depends(get_token),
+    request: Request = None,


The request: Request = None parameter is not used anywhere in the function. Remove it to avoid unused parameter noise and potential confusion.

mateacademy-ai-mentor · 2026-02-12T19:02:20Z

src/routes/profiles.py

+    profile_data: ProfileCreateSchema = Depends(ProfileCreateSchema.as_form),
+    avatar: UploadFile = File(...),


You depend on ProfileCreateSchema.as_form and also accept avatar: UploadFile separately. The task requires avatar to be part of the schema and validated there (using validate_image). Either include avatar in the schema as_form or explicitly document why it is separate; best is to move avatar into the schema so validation lives in the schema layer per requirements.

mateacademy-ai-mentor · 2026-02-12T19:02:20Z

src/routes/profiles.py

+    # 3. Validate input data using custom validators (return 400 on failure)
+    try:
+        from validation import validate_name, validate_birth_date, validate_gender, validate_image
+
+        validate_name(profile_data.first_name)
+        validate_name(profile_data.last_name)
+        validate_birth_date(profile_data.date_of_birth)
+        gender_value = profile_data.gender.value if hasattr(profile_data.gender, 'value') else profile_data.gender
+        validate_gender(gender_value)
+        if not profile_data.info or profile_data.info.isspace():
+            raise ValueError("Info field cannot be empty or contain only spaces.")
+        # validate avatar
+        avatar.file.seek(0)
+        validate_image(avatar)
+    except ValueError as e:
+        raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e))


This block performs all input validation inside the route using validation helpers. Per the Code Style / Implementation requirements, those validations must be implemented in ProfileCreateSchema via field_validator methods (use validate_name, validate_gender, validate_birth_date, validate_image). Move these checks into the schema to satisfy the spec and keep routes thin.

mateacademy-ai-mentor · 2026-02-12T19:02:20Z

src/routes/profiles.py

+        # validate avatar
+        avatar.file.seek(0)
+        validate_image(avatar)


Calling avatar.file.seek(0) and validate_image(avatar) here validates the uploaded file at route level. The spec requires image validation in the schema using validate_image (including extension and size checks). Move avatar validation into ProfileCreateSchema and keep route-level checks minimal.

mateacademy-ai-mentor · 2026-02-12T19:02:20Z

src/routes/profiles.py

+
+        file_extension = avatar.filename.rsplit('.', 1)[-1].lower()
+        s3_path = f"avatars/{user_id}_avatar.{file_extension}"
+
+        await s3_client.upload_file(s3_path, file_data)


File extension extraction uses rsplit('.', 1)[-1].lower() which is robust — good. Ensure s3_client.upload_file signature expects (path, bytes) and is async; if the storage interface differs, adjust accordingly. If the client is sync, don't await it.

mateacademy-ai-mentor · 2026-02-12T19:02:20Z

src/routes/profiles.py

+        # Optionally get a public URL (not required by tests but useful for response)
+        try:
+            avatar_url = await s3_client.get_file_url(s3_path)
+        except Exception:
+            avatar_url = None
+
+    except Exception as e:
+        import logging
+
+        logging.error(f"S3 upload error: {e}")
+        from exceptions import S3FileUploadError
+        raise HTTPException(
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            detail="Failed to upload avatar. Please try again later."
+        )
+
+    # 6. Profile Creation and Storage
+    new_profile = UserProfileModel(
+        user_id=user_id,
+        first_name=profile_data.first_name.lower(),
+        last_name=profile_data.last_name.lower(),
+        gender=gender_value,
+        date_of_birth=profile_data.date_of_birth,
+        info=profile_data.info,
+        # store the S3 object key in DB as required by tests
+        avatar=s3_path


You attempt to get a public file URL (avatar_url = await s3_client.get_file_url(s3_path)) but do not use avatar_url when creating the DB record or in the final response (you store s3_path in DB). The requirement states the response must include the final avatar URL. Ensure the final response's avatar field contains the public URL (or convert the stored key to URL during serialization).

mateacademy-ai-mentor · 2026-02-12T19:02:20Z

src/routes/profiles.py

+
+    except Exception as e:
+        import logging
+
+        logging.error(f"S3 upload error: {e}")
+        from exceptions import S3FileUploadError
+        raise HTTPException(
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            detail="Failed to upload avatar. Please try again later."
+        )


The except Exception block around the upload is too broad and hides storage-specific errors. Also import logging and from exceptions import S3FileUploadError are done inside the except and S3FileUploadError is unused. Prefer catching explicit storage-related exceptions, import logging at module top, and remove unused imports. Keep the HTTPException message as defined but avoid catching bare Exception if possible.

mateacademy-ai-mentor · 2026-02-12T19:02:20Z

src/routes/profiles.py

+    db.add(new_profile)
+    await db.commit()
+    await db.refresh(new_profile)
+
+    return new_profile


You return new_profile ORM instance directly which will be serialized by the response model. Make sure the response serialization produces the public avatar URL (not just the S3 key) as required. If the tests expect a URL, convert the stored key to URL before returning or store the URL in the DB field you return.

mateacademy-ai-mentor · 2026-02-12T19:02:20Z

src/security/passwords.py

+    if isinstance(password, str):
+        password = password.encode('utf-8')


Encoding password (a Python str) to bytes before passing to pwd_context.hash is incorrect — passlib expects a str. Instead, handle bytes by decoding to a str (e.g. if isinstance(password, bytes): password = password.decode('utf-8')), and do not encode str to bytes. Update this line so password passed into pwd_context.hash is a str.

mateacademy-ai-mentor · 2026-02-12T19:02:24Z

src/security/passwords.py

+    if len(password) > 72:
+        password = password[:72]


Truncating by character count (72) may not match bcrypt's 72-byte limit if non-ASCII characters are present. If you must enforce bcrypt's limit, consider truncating the UTF-8 encoded bytes to 72 bytes (and then decode safely) or document that truncation is done by characters. At minimum ensure truncation runs on the same type (str) used by passlib.

mateacademy-ai-mentor · 2026-02-12T19:02:25Z

src/security/passwords.py

+    if isinstance(plain_password, str):
+        plain_password = plain_password.encode('utf-8')


Same issue as above: do not encode plain_password (a str) to bytes before verification. If you receive bytes, decode them; otherwise pass plain_password as str to pwd_context.verify.

mateacademy-ai-mentor · 2026-02-12T19:02:25Z

src/security/passwords.py

+    if len(plain_password) > 72:
+        plain_password = plain_password[:72]


As with hashing, ensure truncation here targets the correct length metric (bytes vs characters). Truncating plain_password by characters may not correctly enforce bcrypt byte-length limit. Consider consistent handling as suggested for the hash function.

mateacademy-ai-mentor · 2026-02-12T19:02:25Z

src/security/passwords.py

+        plain_password = plain_password.encode('utf-8')
+    if len(plain_password) > 72:
+        plain_password = plain_password[:72]
    return pwd_context.verify(plain_password, hashed_password)


pwd_context.verify expects the plain password as a str (not bytes). Ensure plain_password is a str here. After fixes above this return should work correctly.

mateacademy-ai-mentor · 2026-02-12T19:02:25Z

src/schemas/profiles.py


-from fastapi import UploadFile, Form, File, HTTPException
-from pydantic import BaseModel, field_validator, HttpUrl
+from fastapi import UploadFile, Form, File


You imported UploadFile, Form, and File but the schema doesn't define or use an avatar field. Per the requirements, add avatar: UploadFile to the model and accept it in as_form (e.g. avatar: UploadFile = File(...)) so multipart uploads are handled.

mateacademy-ai-mentor · 2026-02-12T19:02:25Z

src/schemas/profiles.py

-from fastapi import UploadFile, Form, File, HTTPException
-from pydantic import BaseModel, field_validator, HttpUrl
+from fastapi import UploadFile, Form, File
+from pydantic import BaseModel, field_validator, ConfigDict


field_validator is imported but no validators are implemented. The schema must include @field_validator methods that call the validation helpers (e.g. validate_name for first_name/last_name, validate_gender for gender, validate_birth_date for date_of_birth, and a validator for info to ensure it isn't empty or only spaces).

mateacademy-ai-mentor · 2026-02-12T19:02:25Z

src/schemas/profiles.py

+class ProfileCreateSchema(BaseModel):
+    first_name: str
+    last_name: str
+    gender: str
+    date_of_birth: date
+    info: str | None


Current model fields (first_name, last_name, gender, date_of_birth, info) are declared but avatar is missing. Add avatar: UploadFile to the class fields so the Pydantic model represents all required inputs.

mateacademy-ai-mentor · 2026-02-12T19:02:25Z

src/schemas/profiles.py

+    @classmethod
+    def as_form(
+        cls,
+        first_name: str = Form(...),
+        last_name: str = Form(...),
+        gender: str = Form(...),
+        date_of_birth: date = Form(...),
+        info: str | None = Form(None),


as_form does not accept avatar. Update as_form signature to include avatar: UploadFile = File(...) and return cls(..., avatar=avatar) so FastAPI can bind the uploaded file via Depends(ProfileCreateSchema.as_form).

mateacademy-ai-mentor · 2026-02-12T19:02:25Z

src/schemas/profiles.py

+        return cls(
+            first_name=first_name,
+            last_name=last_name,
+            gender=gender,
+            date_of_birth=date_of_birth,
+            info=info,
+        )


When returning cls(...) you must include the avatar argument after adding it to as_form. Also ensure avatar is preserved as an UploadFile type on the model so the @field_validator('avatar') can call validate_image(avatar) and enforce extension/size rules.

mateacademy-ai-mentor · 2026-02-12T19:02:25Z

src/schemas/profiles.py

+            info=info,
+        )
+
+


Consider adding explicit @field_validator implementations in this file: - first_name/last_name: call validate_name (only English letters). - gender: call validate_gender. - date_of_birth: call validate_birth_date. - info: check not empty / not only spaces. - avatar: call validate_image (JPG/JPEG/PNG and <=1MB).

mateacademy-ai-mentor · 2026-02-12T19:02:30Z

src/schemas/profiles.py

+class ProfileResponseSchema(BaseModel):
+    id: int
+    user_id: int
+    first_name: str
+    last_name: str
+    gender: str
+    date_of_birth: date
+    info: str
+    avatar: str
+
+    model_config = ConfigDict(from_attributes=True)


Right now ProfileResponseSchema is fine for responses, but ensure the create schema produces/returns an avatar value matching tests (either DB-stored path or a public URL). If tests expect a URL, the route should resolve the storage key to a URL before returning; keep the response schema consistent with that behavior.

Create task 4

99703da

mateacademy-ai-mentor suggested changes Feb 12, 2026

View reviewed changes

Check task 4

216412b

SofaD8 requested a review from mateacademy-ai-mentor February 12, 2026 18:56

mateacademy-ai-mentor suggested changes Feb 12, 2026

View reviewed changes

mateacademy-ai-mentor reviewed Feb 12, 2026

View reviewed changes

SofaD8 force-pushed the develop branch from a8ebbc3 to 216412b Compare February 13, 2026 19:15

		raise ValueError("Info cannot be empty or consist only of spaces.")
		return v

		@@ -1,7 +1,8 @@
		from datetime import date
		from typing import Optional

		profile_data: ProfileCreateSchema = Depends(ProfileCreateSchema.as_form),
		avatar: UploadFile = File(...),

		if isinstance(password, str):
		password = password.encode('utf-8')

		if isinstance(plain_password, str):
		plain_password = plain_password.encode('utf-8')

		if len(plain_password) > 72:
		plain_password = plain_password[:72]

Conversation

SofaD8 commented Feb 12, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone