refactor: Code-Review-Fixes (Reuse/Simplification/Efficiency) aus dieser Session
4 parallele Review-Agenten (Reuse/Simplification/Efficiency/Altitude) gegen den Diff dieser Session (c7dd47f...HEAD) laufen lassen, echte Funde angewendet: - lager.py: 5x wiederholtes "db.get(...) or 404 raisen" durch _get_or_404()- Helper ersetzt - akte.py: zwei Queries für Geräte-Instanzen (erst Positions-IDs, dann Geräte) zu einer Query mit Subquery zusammengefasst - akte.py: manuelle Feld-für-Feld-Rekonstruktion von ObjektRead/HistorieRead durch model_validate()+model_copy() ersetzt (HistorieRead.benutzer_name bekommt dafür einen Default, harmlos für den bestehenden Endpunkt) - vorlagen.py: doppelte "Positionen löschen + flush"-Logik (Vorlage-Löschen und Positionen-Ersetzen) in loesche_alle_positionen() zusammengeführt Bewusst nicht angewendet: - Zentrale Session-Rollback-Vereinheitlichung für die drei Pre-Check-Stellen (objekte.py Selbstbezug, geraet_instanz.py SN-Duplikat, lager.py Eltern- Selbstbezug) - der Altitude-Review schlug das vor, aber get_db() rollt in Produktion bei jeder Exception bereits korrekt zurück (jede Anfrage hat eine eigene Session); das PendingRollbackError-Problem trat nur in der geteilten Test-Session auf und wurde bereits gezielt per Pre-Check vermieden - eine zusätzliche zentrale Rollback-Schicht würde nichts mehr reparieren, was nicht schon repariert ist. - Postgres-Cluster-Encoding (template1) auf beiden Hosts fixen, nicht nur in CI/für die eine migrierte DB - echter Infra-Eingriff, braucht Rücksprache. - LagerSection.tsx "inkonsistente" Formular-Resets - beim genaueren Hinsehen gewolltes UX (Ort/Typ bleiben ausgewählt für mehrere Anlagen hintereinander). 140 Tests weiterhin grün. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CVgbozhYmuEhiEJHffRXCV
This commit is contained in:
@@ -1,3 +1,5 @@
|
|||||||
|
from typing import TypeVar
|
||||||
|
|
||||||
from fastapi import APIRouter, Depends, HTTPException, status
|
from fastapi import APIRouter, Depends, HTTPException, status
|
||||||
from sqlalchemy import select
|
from sqlalchemy import select
|
||||||
from sqlalchemy.ext.asyncio import AsyncSession
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
@@ -23,6 +25,15 @@ router = APIRouter()
|
|||||||
_admin_only = require_roles(RolleTyp.administration)
|
_admin_only = require_roles(RolleTyp.administration)
|
||||||
_materialverantwortliche = require_roles(RolleTyp.administration, RolleTyp.materialverantwortlicher)
|
_materialverantwortliche = require_roles(RolleTyp.administration, RolleTyp.materialverantwortlicher)
|
||||||
|
|
||||||
|
_T = TypeVar("_T")
|
||||||
|
|
||||||
|
|
||||||
|
async def _get_or_404(db: AsyncSession, model: type[_T], id_: int, meldung: str) -> _T:
|
||||||
|
objekt = await db.get(model, id_)
|
||||||
|
if objekt is None:
|
||||||
|
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=meldung)
|
||||||
|
return objekt
|
||||||
|
|
||||||
|
|
||||||
# -- Lagerort (WH-001) ---------------------------------------------------------
|
# -- Lagerort (WH-001) ---------------------------------------------------------
|
||||||
|
|
||||||
@@ -36,8 +47,8 @@ async def liste_lagerorte(db: AsyncSession = Depends(get_db), _=Depends(get_curr
|
|||||||
async def erstelle_lagerort(
|
async def erstelle_lagerort(
|
||||||
payload: LagerortCreate, db: AsyncSession = Depends(get_db), _=Depends(_admin_only)
|
payload: LagerortCreate, db: AsyncSession = Depends(get_db), _=Depends(_admin_only)
|
||||||
) -> Lagerort:
|
) -> Lagerort:
|
||||||
if payload.parent_id is not None and await db.get(Lagerort, payload.parent_id) is None:
|
if payload.parent_id is not None:
|
||||||
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Übergeordneter Lagerort nicht gefunden")
|
await _get_or_404(db, Lagerort, payload.parent_id, "Übergeordneter Lagerort nicht gefunden")
|
||||||
lagerort = Lagerort(**payload.model_dump())
|
lagerort = Lagerort(**payload.model_dump())
|
||||||
db.add(lagerort)
|
db.add(lagerort)
|
||||||
await db.flush()
|
await db.flush()
|
||||||
@@ -48,9 +59,7 @@ async def erstelle_lagerort(
|
|||||||
async def aendere_lagerort(
|
async def aendere_lagerort(
|
||||||
lagerort_id: int, payload: LagerortUpdate, db: AsyncSession = Depends(get_db), _=Depends(_admin_only)
|
lagerort_id: int, payload: LagerortUpdate, db: AsyncSession = Depends(get_db), _=Depends(_admin_only)
|
||||||
) -> Lagerort:
|
) -> Lagerort:
|
||||||
lagerort = await db.get(Lagerort, lagerort_id)
|
lagerort = await _get_or_404(db, Lagerort, lagerort_id, "Lagerort nicht gefunden")
|
||||||
if lagerort is None:
|
|
||||||
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Lagerort nicht gefunden")
|
|
||||||
daten = payload.model_dump(exclude_unset=True)
|
daten = payload.model_dump(exclude_unset=True)
|
||||||
if daten.get("parent_id") == lagerort_id:
|
if daten.get("parent_id") == lagerort_id:
|
||||||
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail="Lagerort kann nicht sein eigener Elternknoten sein")
|
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail="Lagerort kann nicht sein eigener Elternknoten sein")
|
||||||
@@ -77,8 +86,7 @@ async def liste_lagerplaetze(
|
|||||||
async def erstelle_lagerplatz(
|
async def erstelle_lagerplatz(
|
||||||
payload: LagerplatzCreate, db: AsyncSession = Depends(get_db), _=Depends(_materialverantwortliche)
|
payload: LagerplatzCreate, db: AsyncSession = Depends(get_db), _=Depends(_materialverantwortliche)
|
||||||
) -> Lagerplatz:
|
) -> Lagerplatz:
|
||||||
if await db.get(Lagerort, payload.lagerort_id) is None:
|
await _get_or_404(db, Lagerort, payload.lagerort_id, "Lagerort nicht gefunden")
|
||||||
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Lagerort nicht gefunden")
|
|
||||||
vorhanden = await db.execute(select(Lagerplatz).where(Lagerplatz.code == payload.code))
|
vorhanden = await db.execute(select(Lagerplatz).where(Lagerplatz.code == payload.code))
|
||||||
if vorhanden.scalar_one_or_none() is not None:
|
if vorhanden.scalar_one_or_none() is not None:
|
||||||
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail="Code bereits vergeben")
|
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail="Code bereits vergeben")
|
||||||
@@ -94,8 +102,7 @@ async def erstelle_lagerplatz(
|
|||||||
async def hole_bestand(
|
async def hole_bestand(
|
||||||
lagerplatz_id: int, db: AsyncSession = Depends(get_db), _=Depends(get_current_user)
|
lagerplatz_id: int, db: AsyncSession = Depends(get_db), _=Depends(get_current_user)
|
||||||
) -> list[Bestand]:
|
) -> list[Bestand]:
|
||||||
if await db.get(Lagerplatz, lagerplatz_id) is None:
|
await _get_or_404(db, Lagerplatz, lagerplatz_id, "Lagerplatz nicht gefunden")
|
||||||
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Lagerplatz nicht gefunden")
|
|
||||||
result = await db.execute(select(Bestand).where(Bestand.lagerplatz_id == lagerplatz_id))
|
result = await db.execute(select(Bestand).where(Bestand.lagerplatz_id == lagerplatz_id))
|
||||||
return list(result.scalars().all())
|
return list(result.scalars().all())
|
||||||
|
|
||||||
@@ -111,8 +118,7 @@ async def route_einlagern(
|
|||||||
db: AsyncSession = Depends(get_db),
|
db: AsyncSession = Depends(get_db),
|
||||||
current_user=Depends(get_current_user),
|
current_user=Depends(get_current_user),
|
||||||
) -> Materialbewegung:
|
) -> Materialbewegung:
|
||||||
if await db.get(Lagerplatz, lagerplatz_id) is None:
|
await _get_or_404(db, Lagerplatz, lagerplatz_id, "Lagerplatz nicht gefunden")
|
||||||
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Lagerplatz nicht gefunden")
|
|
||||||
return await einlagern(
|
return await einlagern(
|
||||||
db,
|
db,
|
||||||
lagerplatz_id=lagerplatz_id,
|
lagerplatz_id=lagerplatz_id,
|
||||||
@@ -134,8 +140,7 @@ async def route_auslagern(
|
|||||||
db: AsyncSession = Depends(get_db),
|
db: AsyncSession = Depends(get_db),
|
||||||
current_user=Depends(get_current_user),
|
current_user=Depends(get_current_user),
|
||||||
) -> Materialbewegung:
|
) -> Materialbewegung:
|
||||||
if await db.get(Lagerplatz, lagerplatz_id) is None:
|
await _get_or_404(db, Lagerplatz, lagerplatz_id, "Lagerplatz nicht gefunden")
|
||||||
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Lagerplatz nicht gefunden")
|
|
||||||
try:
|
try:
|
||||||
return await auslagern(
|
return await auslagern(
|
||||||
db,
|
db,
|
||||||
|
|||||||
@@ -6,9 +6,15 @@ from app.api.deps import get_current_user, require_roles
|
|||||||
from app.db.session import get_db
|
from app.db.session import get_db
|
||||||
from app.models.auth import RolleTyp
|
from app.models.auth import RolleTyp
|
||||||
from app.models.objekt import Objekt
|
from app.models.objekt import Objekt
|
||||||
from app.models.vorlage import Beladungsvorlage, Vorlagenposition
|
from app.models.vorlage import Beladungsvorlage
|
||||||
from app.schemas.vorlage import BeladungsvorlageCreate, BeladungsvorlageRead, VorlagenAenderung
|
from app.schemas.vorlage import BeladungsvorlageCreate, BeladungsvorlageRead, VorlagenAenderung
|
||||||
from app.services.vorlagen import aktualisiere_positionen, erstelle_vorlage, hole_positionen, neue_version
|
from app.services.vorlagen import (
|
||||||
|
aktualisiere_positionen,
|
||||||
|
erstelle_vorlage,
|
||||||
|
hole_positionen,
|
||||||
|
loesche_alle_positionen,
|
||||||
|
neue_version,
|
||||||
|
)
|
||||||
|
|
||||||
router = APIRouter()
|
router = APIRouter()
|
||||||
|
|
||||||
@@ -117,12 +123,5 @@ async def loesche_vorlage(
|
|||||||
detail="Vorlage wird von mindestens einem Objekt verwendet und kann nicht gelöscht werden",
|
detail="Vorlage wird von mindestens einem Objekt verwendet und kann nicht gelöscht werden",
|
||||||
)
|
)
|
||||||
|
|
||||||
positionen = await db.execute(select(Vorlagenposition).where(Vorlagenposition.vorlage_id == vorlage_id))
|
await loesche_alle_positionen(db, vorlage_id)
|
||||||
for position in positionen.scalars().all():
|
|
||||||
await db.delete(position)
|
|
||||||
# Flush zwischen den Deletes nötig: Beladungsvorlage/Vorlagenposition haben
|
|
||||||
# keine ORM-relationship() (nur rohe FK-Spalten), SQLAlchemy kennt die
|
|
||||||
# Abhängigkeit also nicht automatisch und könnte beide DELETEs in falscher
|
|
||||||
# Reihenfolge absetzen (FK-Verletzung, beim echten Löschversuch gefunden).
|
|
||||||
await db.flush()
|
|
||||||
await db.delete(vorlage)
|
await db.delete(vorlage)
|
||||||
|
|||||||
@@ -9,7 +9,7 @@ class HistorieRead(BaseModel):
|
|||||||
id: uuid.UUID
|
id: uuid.UUID
|
||||||
zeitpunkt: datetime
|
zeitpunkt: datetime
|
||||||
benutzer_id: int | None
|
benutzer_id: int | None
|
||||||
benutzer_name: str | None
|
benutzer_name: str | None = None
|
||||||
ereignistyp: str
|
ereignistyp: str
|
||||||
entitaet_typ: str
|
entitaet_typ: str
|
||||||
entitaet_id: str
|
entitaet_id: str
|
||||||
|
|||||||
@@ -78,29 +78,19 @@ async def akte_fuer_objekt(db: AsyncSession, objekt_id: int) -> AkteRead | None:
|
|||||||
select(Dokument).where(Dokument.entitaet_typ == "objekt", Dokument.entitaet_id == str(objekt_id))
|
select(Dokument).where(Dokument.entitaet_typ == "objekt", Dokument.entitaet_id == str(objekt_id))
|
||||||
)
|
)
|
||||||
|
|
||||||
positions_result = await db.execute(
|
geraete_result = await db.execute(
|
||||||
select(Objektposition.id).where(Objektposition.objekt_id == objekt_id)
|
select(GeraetInstanz).where(
|
||||||
)
|
GeraetInstanz.objektposition_id.in_(
|
||||||
position_ids = [row[0] for row in positions_result.all()]
|
select(Objektposition.id).where(Objektposition.objekt_id == objekt_id)
|
||||||
geraete: list[GeraetInstanz] = []
|
)
|
||||||
if position_ids:
|
|
||||||
geraete_result = await db.execute(
|
|
||||||
select(GeraetInstanz).where(GeraetInstanz.objektposition_id.in_(position_ids))
|
|
||||||
)
|
)
|
||||||
geraete = list(geraete_result.scalars().all())
|
)
|
||||||
|
geraete = list(geraete_result.scalars().all())
|
||||||
|
|
||||||
maengel_result = await db.execute(select(Mangel).where(Mangel.objekt_id == objekt_id))
|
maengel_result = await db.execute(select(Mangel).where(Mangel.objekt_id == objekt_id))
|
||||||
|
|
||||||
objekt_read = ObjektRead(
|
objekt_read = ObjektRead.model_validate(objekt).model_copy(
|
||||||
id=objekt.id,
|
update={"letzte_kontrolle_am": letzte_map.get(objekt_id)}
|
||||||
code=objekt.code,
|
|
||||||
name=objekt.name,
|
|
||||||
objekttyp_id=objekt.objekttyp_id,
|
|
||||||
vorlage_id=objekt.vorlage_id,
|
|
||||||
standort_id=objekt.standort_id,
|
|
||||||
status=objekt.status,
|
|
||||||
fahrzeug_id=objekt.fahrzeug_id,
|
|
||||||
letzte_kontrolle_am=letzte_map.get(objekt_id),
|
|
||||||
)
|
)
|
||||||
|
|
||||||
return AkteRead(
|
return AkteRead(
|
||||||
@@ -114,18 +104,7 @@ async def akte_fuer_objekt(db: AsyncSession, objekt_id: int) -> AkteRead | None:
|
|||||||
],
|
],
|
||||||
letzte_kontrolle_am=letzte_map.get(objekt_id),
|
letzte_kontrolle_am=letzte_map.get(objekt_id),
|
||||||
historie=[
|
historie=[
|
||||||
HistorieRead(
|
HistorieRead.model_validate(eintrag).model_copy(update={"benutzer_name": benutzer_name})
|
||||||
id=eintrag.id,
|
|
||||||
zeitpunkt=eintrag.zeitpunkt,
|
|
||||||
benutzer_id=eintrag.benutzer_id,
|
|
||||||
benutzer_name=benutzer_name,
|
|
||||||
ereignistyp=eintrag.ereignistyp,
|
|
||||||
entitaet_typ=eintrag.entitaet_typ,
|
|
||||||
entitaet_id=eintrag.entitaet_id,
|
|
||||||
alter_wert=eintrag.alter_wert,
|
|
||||||
neuer_wert=eintrag.neuer_wert,
|
|
||||||
begruendung=eintrag.begruendung,
|
|
||||||
)
|
|
||||||
for eintrag, benutzer_name in historie_result.all()
|
for eintrag, benutzer_name in historie_result.all()
|
||||||
],
|
],
|
||||||
dokumente=[DokumentRead.model_validate(d) for d in dokumente_result.scalars().all()],
|
dokumente=[DokumentRead.model_validate(d) for d in dokumente_result.scalars().all()],
|
||||||
|
|||||||
@@ -41,6 +41,18 @@ async def neue_version(
|
|||||||
return neue
|
return neue
|
||||||
|
|
||||||
|
|
||||||
|
async def loesche_alle_positionen(db: AsyncSession, vorlage_id: int) -> None:
|
||||||
|
"""Gemeinsame Vorstufe für Vorlage-Löschen und Positionen-Ersetzen: Flush
|
||||||
|
zwischen Löschen und dem nächsten Schritt ist Pflicht - Beladungsvorlage/
|
||||||
|
Vorlagenposition haben keine ORM-relationship() (nur rohe FK-Spalten),
|
||||||
|
SQLAlchemy kennt die Abhängigkeit also nicht automatisch und könnte DELETEs
|
||||||
|
in falscher Reihenfolge absetzen (FK-Verletzung, beim echten Löschversuch
|
||||||
|
gefunden)."""
|
||||||
|
for position in await hole_positionen(db, vorlage_id):
|
||||||
|
await db.delete(position)
|
||||||
|
await db.flush()
|
||||||
|
|
||||||
|
|
||||||
async def aktualisiere_positionen(
|
async def aktualisiere_positionen(
|
||||||
db: AsyncSession, *, vorlage: Beladungsvorlage, positionen: list[VorlagenpositionCreate]
|
db: AsyncSession, *, vorlage: Beladungsvorlage, positionen: list[VorlagenpositionCreate]
|
||||||
) -> Beladungsvorlage:
|
) -> Beladungsvorlage:
|
||||||
@@ -49,13 +61,7 @@ async def aktualisiere_positionen(
|
|||||||
sich NICHT auf bereits angelegte Objekte aus, da deren Sollmenge fest in
|
sich NICHT auf bereits angelegte Objekte aus, da deren Sollmenge fest in
|
||||||
`objektposition.sollmenge_vorlage` kopiert ist (siehe services/objekte.py),
|
`objektposition.sollmenge_vorlage` kopiert ist (siehe services/objekte.py),
|
||||||
nicht mehr live aus der Vorlage gelesen wird."""
|
nicht mehr live aus der Vorlage gelesen wird."""
|
||||||
bisherige = await hole_positionen(db, vorlage.id)
|
await loesche_alle_positionen(db, vorlage.id)
|
||||||
for position in bisherige:
|
|
||||||
await db.delete(position)
|
|
||||||
# Flush zwischen Löschen und Neuanlegen: gleiches Muster wie beim Löschen
|
|
||||||
# einer ganzen Vorlage (DELETE /vorlagen/{id}) - keine relationship(), also
|
|
||||||
# keine automatische Abhängigkeits-Reihenfolge durch SQLAlchemy.
|
|
||||||
await db.flush()
|
|
||||||
for pos in positionen:
|
for pos in positionen:
|
||||||
db.add(Vorlagenposition(vorlage_id=vorlage.id, **pos.model_dump()))
|
db.add(Vorlagenposition(vorlage_id=vorlage.id, **pos.model_dump()))
|
||||||
await db.flush()
|
await db.flush()
|
||||||
|
|||||||
Reference in New Issue
Block a user