Skip to content

Conversation

@silicoflare
Copy link
Contributor

@silicoflare silicoflare commented Jul 9, 2025

Changelog

  • Migrated app and all dependencies from Flask + Flasgger to FastAPI
  • Updated corresponding tests and utilities
  • Added stricter type checking with updated request, response and profile models
  • Added new health check endpoint at /health
  • Migrated from flake8 to ruff linter
  • Updated README and CONTRIBUTING files

- Replaced Flask and Flasgger with FastAPI for improved performance and features.
- Updated dependencies in `pyproject.toml` and `requirements.txt` to include FastAPI and Uvicorn.
- Refactored API endpoints to use FastAPI's async capabilities and response models.
- Removed unused imports and cleaned up the code structure.
…ting

- Changed the endpoint for authentication requests from `/authenticate` to `/`.
- Adjusted formatting for better readability in the Docker image build instructions and request parameters section.
…sertion formatting

- Updated the test client initialization to use FastAPI's TestClient instead of Flask's test client.
- Enhanced assertion formatting for better readability and consistency in error messages.
- Changed variable handling for campus code to ensure proper type conversion.
@silicoflare
Copy link
Contributor Author

One major change that I made is the auth endpoint. Instead of POST /authenticate, you can just do POST /. I felt that there was no need to make a separate endpoint for authentication if the only purpose of the app is authentication.

Let me know if I have to keep it or revert it.

silicoflare and others added 2 commits July 9, 2025 23:40
- Introduced a new POST endpoint `/authenticate` to maintain support for legacy authentication using PESU credentials.
- Implemented input validation and error handling for the new endpoint.
- Enhanced logging for better traceability during the authentication process.
@achyu-dev achyu-dev requested a review from aditeyabaral July 9, 2025 18:51
@silicoflare
Copy link
Contributor Author

One more thing, pesu-auth is now also available on https://pesu-auth.hy-pr.link. It has no downtime, and its response time is as fast as the Render endpoint. I am yet to setup CI/CD on that, but can I also add this link to the README?

@aditeyabaral
Copy link
Member

aditeyabaral commented Jul 10, 2025

No. Render will be our officially recognised deployment. You are free to deploy the API on your personal websites, but the Render endpoint is the only one being maintained and monitored actively by the devs. And point to note: Render does not have any downtimes anymore, we have mitigated that issue. We can remove the latency warning from the README but I would suggest still keeping it there.

@aditeyabaral
Copy link
Member

I have reverted the authentication endpoint to /authenticate. The root route must never be an API endpoint, it should always redirect to the docs. And not to mention, the other potential services that change could break. I have also taken the liberty to clean up some files and logic in app.py, added some documentation, and also fixed/updated the tests and pre-commit files.

@aditeyabaral aditeyabaral requested a review from Copilot July 10, 2025 05:00

This comment was marked as outdated.

@aditeyabaral aditeyabaral requested a review from Copilot July 10, 2025 05:10

This comment was marked as outdated.

@aditeyabaral aditeyabaral requested a review from Copilot July 10, 2025 05:40

This comment was marked as outdated.

Copy link
Contributor

@achyu-dev achyu-dev left a comment

Choose a reason for hiding this comment

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

This Field is giving me a depreceation warning when I am running the tests.


======================================================================== warnings summary ========================================================================= 
..\..\..\.conda\envs\test2\Lib\site-packages\pydantic\fields.py:1093: 19 warnings
  C:\Users\Achyuth S S\.conda\envs\test2\Lib\site-packages\pydantic\fields.py:1093: PydanticDeprecatedSince20: Using extra keyword arguments on `Field` is deprecated and will be removed. Use `json_schema_extra` instead. (Extra keys: 'example'). Deprecated in Pydantic V2.0 to be removed in V3.0. See Pydantic V2 Migration Guide at https://errors.pydantic.dev/2.11/migration/
    warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================ 31 passed, 19 warnings in 20.11s ================================================================= 

although all tests are passing, this is just a warning.

One way to overcome, if we want to, is add a json_schema parameter wherever this Field is used

@achyu-dev
Copy link
Contributor

The /authenticate is working on postman, just tested

image

@achyu-dev
Copy link
Contributor

achyu-dev commented Jul 10, 2025

I still get this warning. Can we safely ignore this? As @ndigvijay had pointed before its just a VS Code extension issue 👍

  fields: list[Literal[*PESUAcademyConstants.DEFAULT_FIELDS]] | None = Field(

on this line

Unpacked arguments cannot be used in this contextPylance[reportInvalidTypeForm](https://github.com/microsoft/pylance-release/blob/main/docs/diagnostics/reportInvalidTypeForm.md)

Thus, when i run

pre-commit run --all-files

I get below error - >

          File "app/models/request.py", line 27
            fields: list[Literal[*PESUAcademyConstants.DEFAULT_FIELDS]] | None = Field(
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        SyntaxError: invalid syntax. Perhaps you forgot a comma?

Is there a workaround to fix this? @aditeyabaral

@achyu-dev
Copy link
Contributor

I still get this warning. Can we safely ignore this? As @ndigvijay had pointed before its just a VS Code extension issue 👍

  fields: list[Literal[*PESUAcademyConstants.DEFAULT_FIELDS]] | None = Field(

on this line

Unpacked arguments cannot be used in this contextPylance[reportInvalidTypeForm](https://github.com/microsoft/pylance-release/blob/main/docs/diagnostics/reportInvalidTypeForm.md)

Thus, when i run

pre-commit run --all-files

I get below error - >

          File "app/models/request.py", line 27
            fields: list[Literal[*PESUAcademyConstants.DEFAULT_FIELDS]] | None = Field(
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        SyntaxError: invalid syntax. Perhaps you forgot a comma?

Is there a workaround to fix this? @aditeyabaral

For this, I did a temp fix, if u dont want this, please revert @aditeyabaral

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates the authentication API from Flask/Flasgger to FastAPI, updating the application to use modern async patterns and automatic API documentation generation. The migration includes comprehensive updates to the API structure, error handling, and testing infrastructure.

  • Updated from Flask/Flasgger to FastAPI with automatic OpenAPI documentation
  • Migrated synchronous route handlers to async functions with proper error handling
  • Updated all test files to use FastAPI's TestClient instead of Flask's test client
  • Enhanced Pydantic models with detailed field descriptions and validation

Reviewed Changes

Copilot reviewed 20 out of 22 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
app/app.py Complete rewrite to use FastAPI with async handlers, structured error handling, and automatic OpenAPI docs
app/models/*.py Enhanced Pydantic models with detailed field descriptions and modern type annotations
app/util.py Removed Flask-specific validation function as FastAPI handles this automatically
app/pesu.py Updated type hints to use modern union syntax and improved docstrings
tests/integration/test_*.py Updated all integration tests to use FastAPI TestClient
tests/unit/test_*.py Modified unit tests for async compatibility and updated assertions
pyproject.toml Updated dependencies and added FastAPI, uvicorn, and testing packages

aditeyabaral
aditeyabaral previously approved these changes Jul 11, 2025
Copy link
Member

@aditeyabaral aditeyabaral left a comment

Choose a reason for hiding this comment

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

LGTM. Please pull the changes at your end and test.

@aditeyabaral
Copy link
Member

This PR will resolve #45 and complete the entire migration to PESUAuth 2.0 #41

Copy link
Member

@aditeyabaral aditeyabaral left a comment

Choose a reason for hiding this comment

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

Updated README. Good to merge now.

Copy link
Contributor

@achyu-dev achyu-dev left a comment

Choose a reason for hiding this comment

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

Looks Good to me. Please go ahead and merge

@aditeyabaral aditeyabaral merged commit 8568fb6 into pesu-dev:main Jul 11, 2025
7 checks passed
@silicoflare silicoflare deleted the fastapi branch July 14, 2025 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants