cleanup architecture/code structure

[Panaetius]

Collection issue for tasks to make code more consistent and cleaner.

• [x] database repositories should not know about apispec classes, only domain objects
• [x] business logic should be in core.py in each module, not in the database repositories or blueprints. blueprints should only contain validation logic and transformations from/to apispec. DB repos should only contain logic for storing/loading/filtering the db
• [x] ORM objects should not be used outside of the database layer
• [x] ORM classes should not contain business logic
• [ ] We should refactor our some common apispec schemas, e.g. ULID, into a common spec and refer to them by path ($ref: ../../common/apispec.yaml#/Ulid or similar)
• [x] cleanup/consolidate modules. E.g. user and user_preferences should be in a single module, as user_preferences are not top-level entities that make sense on their own
• [x] we should validate input in all blueprint methods (including path parameters) and validate output schema as well https://github.com/SwissDataScienceCenter/renku-data-services/issues/266
• [x] switch to type state kinda pattern for all relevant entities (definitely APIUser, not sure what else) https://github.com/SwissDataScienceCenter/renku-data-services/issues/184
• [x] Consolidate User and UserWithNamespace classes

[leafty]

Some comments:

• business logic should be in core.py in each module, not in the database repositories or blueprints. blueprints should only contain validation logic and transformations from/to apispec. DB repos should only contain logic for storing/loading/filtering the db
• ORM objects should not be used outside of the database layer
• ORM classes should not contain business logic

What if the core logic needs to perform database manipulations? Do we leak the database transaction into the core logic layer?

• We should refactor our some common apispec schemas, e.g. ULID, into a common spec and refer to them by path ($ref: ../../common/apispec.yaml#/Ulid or similar)

I think this probably makes editing in the swagger editor painful (we should try). If it doesn't work, I think we should simply merge all specs into a single file.

[Panaetius]

What if the core logic needs to perform database manipulations? Do we leak the database transaction into the core logic layer?

It's "should" not "must", in general these are guidelines and can be broken, but there needs to be a really good reason to break them.

I think this probably makes editing in the swagger editor painful (we should try). If it doesn't work, I think we should simply merge all specs into a single file.

Agreed, it'd be nice if we can keep them split but have some reuse, but if it turns out to be cumbersome, we can go with another approach. But the current approach of having 10 definitions of Ulid isn't good.

[leafty]

The swagger editor works with URLs which can be accessed online, e.g.

$ref: "https://raw.githubusercontent.com/SwissDataScienceCenter/renku-data-services/main/components/renku_data_services/session/api.spec.yaml#/components/schemas/EnvironmentPatch"

[Panaetius]

I don't think absolute URLs can work for us. E.g. if I wanted to change the ULID definition in a PR I'd have a chicken&egg problem.

[Panaetius]

for reference, https://herbertograca.com/2017/11/16/explicit-architecture-01-ddd-hexagonal-onion-clean-cqrs-how-i-put-it-all-together/ is the architecture that worked well for me in the past, and I'm suggesting a light version of this (domain and application layer are rolled into one for instance)