added simple project crud

[KonradUdoHannes]

Still drafting, but I implemented the basic crud functionality.

@jandix @DatenBergwerker There are mainly two points worth discussing.

1. Shall we create database sessions in the requests via dependency injections (current implementation) or shall we maintain a central connection as part of the app life cycle. In terms of load the second option should be fine since the session is async but I'm not sure whether flushes and commits from different requests could potentially interfere.
2. Any column suggestions that we should absolutely include? A few things where omitted on purpose so far
   • enum representation of status will be done as part of Implement Project Life cycle functionality scaffolding #23
   • no relations such as organizations or people where defined yet because I wanted to wait a little bit until we have data models and example data in place for other entities.

Before finishing the draft I'll still implement a few tests and try to create a first version of a script to populate projects from our cms.

[jandix]

@KonradUdoHannes Thank you for providing the draft.

1. I would prefer using a dependency because we can use FastAPI's dependency overwrite for testing. Additionally, we can implicitly commit, rollback and handle exceptions related to the database. This setup would also provide us with implicit transactions. However, we should probably exhaustively manage this behaviour then. Please find a snippet below:

async def get_db_session() -> AsyncGenerator[AsyncSession, None]:
    async with LocalSession() as session:
        try:
            yield session
            await session.commit()
        except Exception as e:
            logger.exception(e)
            await session.rollback()
            raise
        finally:
            await session.close()

2. I think for the basic setup this should be fine. Two considerations.

• Should we already add the tags (e.g.: Python, Welfare, etc.) that will be defined in the taxonomy or should we create a separate issue for this?
• I have some bad experience with enums in SQLAlchemy, Alembic, and Postgres. The enum will be created but not torn down when the revision is reverted. Maybe we can normalise the schema and create a separate table for the stages linked via a foreign key? What do you think?

4. Should we include the database changes in the initial revision? Maybe we can create a single revision when merging?
5. We should be consistent with model and table names. Do we always use singular notation for database models and database tables? What is the notation for Pydantic schemas? That is something that we can probably discuss in our next call?

[KonradUdoHannes]

@jandix Thanks for the feedback

Regarding 1

I like the suggested dependency. But is the session.close() needed in the finally block? Shouldn't the async with clause handle the session closing automatically?

Regarding 2

I think for enums in general we can can probably start by storing String arrays in the database and then implement the enum on the pydantic side. That way we favor flexibility over stability a bit on the DB and still have validation of the data in the API. This should also avoid enum issues related specifically to the DB.
One the project is further and we need more stability we can then look into normalizing the arrays into separate tables

Regarding 4

I don't see an issue with keeping the revisions separate. Do you have any concerns here? As long as we don't have a continuously running DB with actual we have of course the flexibility to modify revisions without much trouble.

Regarding 5

I also noticed some slight inconsistencies here. I think its indeed a good point for the next call.

[KonradUdoHannes]

@jandix I added some tests and set the PR to review.

Regarding the database transactions I refactored the current version to use the session makers .begin method to create a context with both a session and a transaction at the same time. Did you mean that by implicit transactions. Either way I find it quite nice to use that way.

I also create a script to populate projects from our cms via both their APIs. I didn't commit this script so far because I think we could maybe discuss where to put this and similar scripts, because they are then very CorrelAid specific. At the same time this currently only uses a public endpointof our cms, so everybody could actually run it.

I think we should also finish the discussion regarding migrations (merged vs. separate) before merging.

[jandix]

@KonradUdoHannes I added some proposals regarding style. Regarding the revisions, let's keep it for now. We can discuss this in our next meeting.

[jandix]

This function could be turned into a dependency:

Suggested change

	async def get_project(project_id: uuid.UUID, session: AsyncSession) -> Project:
	async def get_project(project_id: Annotated[uuid.UUID, Path()], session: Annotated[AsyncSession, SessionWithTransactionContext]) -> Project:

[KonradUdoHannes]

I actually had issues with that for when using unit of work patterns for ORM update and delete. The issue was that the return value then belonged to the injected session and I needed access to that session in the function that I inject get_project into. Otherwise I could not continue with the unit of work pattern.
Do you know a solution for this or is the unit of work pattern just not suitable here?

Either way I refactored the dependency injections independently to allow for better testing so the issue is not relevant anymore right now. But I would still be interested in how to solve it (Or whether it does not work with the unit of work pattern)

[KonradUdoHannes]

@jandix Thanks for the review. I actually refactored a little and since the approval stays valid I just wanted to let you know. Basically I encapsulated the database interactions into a separate ProjectStore class to be used as a dependency injection. This allows for easier testing by overwriting dependency ProjectStore itself as opposed to low level Session dependencies. In that context I also added the first test that works without a database. This will make it a little more meaningfull to set up CI if there is actually something to test.

I also included alembic migrations in the test setup itself. This makes testing a tiny bit more convenient now, and it will allow us to more easily build db tests for CI with sqllite in the future if we want to.