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

feature: MongoDB support via beanie #187

Merged
merged 21 commits into from
Dec 6, 2023

Conversation

usefulalgorithm
Copy link
Contributor

Adds support for MongoDB and Beanie. Probably fixes #119 ?

I did a simple crud app with virtually the same setup, seems to be working fine.

Demos

Todos / caveats / things to note:

  • MongoDB requires additional setup if you're not writing to the admin database
  • Some docstring and comments are probably off, and flake is likely to complain.
  • I have no idea how cookiecutter works, so I basically did it like how I use Jinja templates and tried to fill in as many gaps as possible. I'm guessing I probably missed a lot of places...
  • The only setup I tested is the one in the clip.

Copy link
Owner

@s3rius s3rius left a comment

Choose a reason for hiding this comment

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

Hi and thanks for your AWESOME WORK! But I have some questions and suggestions regarding the PR.

Few things to add:

  • Services for Github actions and Gitlab-Ci. It's used by generate CI option.
  • Fix dummy for GraphQL in web/gql folder. Since it's the same as for REST right now.
  • Can you please add test in fastapi_template/tests/test_generator.py? Because we need to be sure that after the project generation, all generated tests are passing.

Thank you very much. I really appreciate the work you did.

@s3rius s3rius changed the base branch from master to develop August 23, 2023 21:08
@s3rius
Copy link
Owner

s3rius commented Aug 23, 2023

Actually, you can easily test the project generations by creating one command and running it everytime you make changes. For example.

fastapi_template --name myproject --db mongo --api-type rest --orm beanie --dummy --routers --quite --force

This command will be overriding all files within myproject folder. It helps me fixing all flake errors.

@s3rius
Copy link
Owner

s3rius commented Aug 23, 2023

Also, you should not start DB separately from the project. The main point is that the up command will bring all services app, including your application.

docker compose up --build

Signed-off-by: Tsung-Ju Lii <[email protected]>
Signed-off-by: Tsung-Ju Lii <[email protected]>
@usefulalgorithm
Copy link
Contributor Author

Hi @s3rius , I think I addressed all the things you suggested:

  • Services for Github actions and Gitlab-Ci. It's used by generate CI option.
    • Added variables in test.yml and .gitlab-ci.yml.
  • Fix dummy for GraphQL in web/gql folder. Since it's the same as for REST right now.
    • Done.
  • Can you please add test in fastapi_template/tests/test_generator.py?
    • Added several tests in that file.

I also made sure all flake8 warnings are gone, mypy does not complain, and that I was able to create & read dummy models with the containers started by docker compose.

However I wasn't able to get pytest to run successfully (both in fastapi_template and in the generated myproject). In fastapi_template running poetry run pytest breaks:

FAILED fastapi_template/tests/test_generator.py::test_default_with_db[sqlalchemy-mysql] - AssertionError
FAILED fastapi_template/tests/test_generator.py::test_default_with_db[tortoise-mysql] - AssertionError
FAILED fastapi_template/tests/test_generator.py::test_default_with_db[ormar-mysql] - AssertionError
FAILED fastapi_template/tests/test_generator.py::test_default_with_nosql_db[beanie-mongodb] - AssertionError

and running tests in myproject would always end up with an unauthenticated client. Are the tests meant to be run only when triggered by github / gitlab pipelines?

Thanks!

@usefulalgorithm
Copy link
Contributor Author

usefulalgorithm commented Sep 6, 2023

Hi @s3rius, I modified the PR per your suggestions, please check out the changes and see if they make sense when you have time, thanks a bunch!

@usefulalgorithm
Copy link
Contributor Author

usefulalgorithm commented Oct 20, 2023

Hi @s3rius , sorry for the lack of response. I've addressed all your suggestions in the latest commit, please take a look when you have time.

There's something I don't get in the generated test_dummy.py file:

async def test_creation(
    fastapi_app: FastAPI,
    client: AsyncClient,
) -> None:
    """Tests dummy instance creation."""
    url = fastapi_app.url_path_for("create_dummy_model")
    test_name = uuid.uuid4().hex
    response = await client.put(
        url,
        json={
            "name": test_name,
        },
    )   # Inserts a Dummy into DB
# ... snipped ...
@pytest.mark.anyio
async def test_getting(
    fastapi_app: FastAPI,
    client: AsyncClient,
) -> None:
    """Tests dummy instance retrieval."""
    dao = DummyDAO()
    test_name = uuid.uuid4().hex
    await dao.create_dummy_model(name=test_name)  # XXX Inserts another Dummy into DB??
    url = fastapi_app.url_path_for("get_dummy_models")
    response = await client.get(url)
    dummies = response.json()

    assert response.status_code == status.HTTP_200_OK
    assert len(dummies) == 1   # XXX Should be 2?

This the only test that's failing. What I don't understand is that how come in test_getting it is testing that the object we inserted is indeed the only object in the database, while the object inserted in test_creation is never cleaned up?

Thanks in advance for taking a look and answering the question!

I decided to remove the inserted object in test_creation, so that in test_getting it's going to report only one object in the database. Now all tests are passing on my machine.

Thanks!

@usefulalgorithm
Copy link
Contributor Author

Hi @s3rius , I believe this is complete now. Thanks!

@usefulalgorithm
Copy link
Contributor Author

@s3rius ping

Copy link
Owner

@s3rius s3rius left a comment

Choose a reason for hiding this comment

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

Sorry. I couldn't find time to review. It takes a lot of time to review every request to this project.

But thank you very much for implementing it. I know how much effort it takes to implement something like this.

@s3rius
Copy link
Owner

s3rius commented Nov 21, 2023

Okay. I don't know what is happening, but now mypy seems to complain about some types. Can you fix it?

Just add an [Any] to every AsyncConnectionPool like AsyncConnectionPool[Any]. Don't forget to add any in imports if it doesn't exists yet in the file.

@usefulalgorithm
Copy link
Contributor Author

Sorry I missed a AsyncConnectionPool... can you trigger the test again? Thanks

@usefulalgorithm
Copy link
Contributor Author

I don't get it...

@s3rius
Copy link
Owner

s3rius commented Nov 25, 2023

@usefulalgorithm, that's fine. Happens sometimes. Will rerun tests.

@s3rius s3rius merged commit 8d73952 into s3rius:develop Dec 6, 2023
1 of 2 checks passed
@s3rius
Copy link
Owner

s3rius commented Dec 6, 2023

I will try to fix the problem on develop branch. Thanks for your outstanding contribution.

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.

Add support for db/orm like mongodb and oss like minio
2 participants