Skip to content

fix: update room mgmt api - #66

Open
0utplay wants to merge 1 commit into
mainfrom
0utplay-patch-1
Open

fix: update room mgmt api#66
0utplay wants to merge 1 commit into
mainfrom
0utplay-patch-1

Conversation

@0utplay

@0utplay 0utplay commented Oct 23, 2025

Copy link
Copy Markdown
Contributor

No description provided.

@0utplay
0utplay requested a review from Janne6565 October 23, 2025 16:10

@Janne6565 Janne6565 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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:

  • composable param description is in the wrong place (~L163-170): it's nested inside schema rather than on the parameter object. The force param 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 400 on DELETE /rooms/{roomId} (~L302) only documents "room has bookings assigned", but RoomService.deleteRoomById also 400s when the room is a child of a composite room — a case this PR itself introduces.
  • The deletable 200 body is an inline anonymous schema (~L320-333) where everything else in the file uses a named component; codegen had to invent IsRoomDeletable200Response. Would extract as RoomDeletableResponse.
  • Constraint asymmetry between request and response schemas — create-requests get minLength/maxLength, responses get only the pattern, and Room.name gets 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.

@0utplay

0utplay commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Janne lässt anscheinend claude hier laufen oder was

@Janne6565

Copy link
Copy Markdown
Member

Hahah Digga was? Mein Claude ist komplett durchgedreht. Den PR sollte der sich eigentlich nicht angucken

@Janne6565

Copy link
Copy Markdown
Member

Aber jaja ist schon wichtig das der room mgmt api PR hier gemerged wird

@Janne6565

Copy link
Copy Markdown
Member

(claude hat das "bitte guck dir alle PRs an bei denen ich als Reviewer eingestellt bin" ein bisschen zu ernst genommen)

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.

2 participants