fix: update room mgmt api - #66
Conversation
Janne6565
left a comment
There was a problem hiding this comment.
Went through this against the room-management backend rather than just reading it in isolation — worth noting for anyone else looking: this file is byte-identical to room-management.yaml at HEAD of team01-backend-room-management, and that repo generates its Spring interfaces from it (openApiGenerate { inputSpec.set("$rootDir/room-management.yaml") }). So it's the implementation's source of truth, and every rename here checks out against real code. The sau-portal.de/ase-1/room-mgmt server URL also matches the ingress and SERVER_SERVLET_CONTEXT_PATH; the old api.provadis.com/v1 was fictional.
Despite the age of the PR it hasn't gone stale — main's copy has exactly one commit ever (Sept 2025), so there's no drift to rebase around.
Content-wise I'm happy with the direction. A few things I'd like to see before this goes in:
1. RoomInquiry.groupId is now dead and contradicts the PR's own model change (~L642-673). This PR migrates bookings from group UUIDs to group names, but the inquiry endpoint still requires a groupId UUID. I grepped the backend — groupId isn't referenced anywhere in src/main/java, so it's never read. This is the one finding a consumer would actually get burned by: they're required to send a UUID that means nothing. Drop it, or rename to groupName for consistency with studentGroupNames.
2. Building.name's example fails its own new pattern (~L494-496). The added pattern: '^[A-Za-z0-9\-\.:]+$' rejects spaces, but example: "Main Campus" has one. Redocly flags it (no-invalid-schema-examples) and it's newly introduced here — main lints clean on that rule. Scalar renders the example as the suggested payload, so people will copy a value the API rejects. Either "Main-Campus" or widen the pattern.
3. Version bump. info.version stays 0.2.0, but removing state/BuildingState, renaming studentGroupIds → studentGroupNames, and adding chemSymbol/composedOf to required on both Room and RoomCreateRequest are all breaking — every create call now has to send composedOf even when empty. Should be 0.3.0.
Smaller stuff, take or leave:
composableparam description is in the wrong place (~L163-170): it's nested insideschemarather than on the parameter object. Theforceparam added in this same PR does it correctly. Renderers read parameter descriptions from the parameter object, so as written it won't show up in the published docs at all. Two-line move.- The
400onDELETE /rooms/{roomId}(~L302) only documents "room has bookings assigned", butRoomService.deleteRoomByIdalso 400s when the room is a child of a composite room — a case this PR itself introduces. - The
deletable200 body is an inline anonymous schema (~L320-333) where everything else in the file uses a named component; codegen had to inventIsRoomDeletable200Response. Would extract asRoomDeletableResponse. - Constraint asymmetry between request and response schemas — create-requests get
minLength/maxLength, responses get only the pattern, andRoom.namegets no pattern at all. Harmless, but anyone generating validators off the response models gets different rules than the server enforces.
For what it's worth I checked whether the breaking renames hurt anyone today: org-wide search turns up no consumer referencing studentGroupIds or composedOf, and nothing else in this repo references BuildingState, so the removals leave no dangling refs.
Happy to approve once #1 is resolved — the rest I'd merge with.
|
Janne lässt anscheinend claude hier laufen oder was |
|
Hahah Digga was? Mein Claude ist komplett durchgedreht. Den PR sollte der sich eigentlich nicht angucken |
|
Aber jaja ist schon wichtig das der room mgmt api PR hier gemerged wird |
|
(claude hat das "bitte guck dir alle PRs an bei denen ich als Reviewer eingestellt bin" ein bisschen zu ernst genommen) |
No description provided.