fix: propagate errors on non-OK Core responses and store resolved payload in metadata#56
fix: propagate errors on non-OK Core responses and store resolved payload in metadata#56mathildeshagl wants to merge 6 commits into
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPropagate HTTP status codes and formatted errors through Keycloak request handlers (one signature extended). Fix resolution merging to store resolvedData into participation.PrevData instead of raw fetched data. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mathildeshagl
changed the title
There was a problem hiding this comment.
Pull request overview
This PR improves correctness and reliability when enriching course participation data with resolved DTOs and when calling core auth endpoints for role mapping / student checks.
Changes:
- Fixes merging logic to store
resolvedData(instead of the raw HTTP response bytes) intoPrevDatafor a single participation resolution. - Returns proper errors on non-200 responses from the core for
is_studentandcourse_phase rolesrequests. - Improves diagnostics by including HTTP status information in returned errors from core request helpers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
resolution.go |
Fixes resolved DTO merge to persist resolvedData into PrevData for single participation resolution. |
keycloakTokenVerifier/keycloakCoreRequests/isStudent.go |
Returns an error for non-OK responses (previously returned nil error), improving caller handling. |
keycloakTokenVerifier/keycloakCoreRequests/coursePhaseRoleMapping.go |
Returns an error for non-OK responses (previously returned nil error), improving caller handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if resp.StatusCode != http.StatusOK { | ||
| log.Error("Received non-OK response:", resp.Status) | ||
| return keycloakTokenVerifierDTO.GetCourseRoles{}, nil | ||
| return keycloakTokenVerifierDTO.GetCourseRoles{}, fmt.Errorf("course phase role mapping request failed with status %d: %s", resp.StatusCode, resp.Status) |
There was a problem hiding this comment.
The error message includes both resp.StatusCode and resp.Status, but resp.Status already includes the numeric code (e.g., "404 Not Found"), leading to duplication in logs/returned errors. Consider using just resp.Status or only the code plus a more specific context (method + path) for cleaner diagnostics.
| return keycloakTokenVerifierDTO.GetCourseRoles{}, fmt.Errorf("course phase role mapping request failed with status %d: %s", resp.StatusCode, resp.Status) | |
| return keycloakTokenVerifierDTO.GetCourseRoles{}, fmt.Errorf("course phase role mapping request GET %s failed: %s", path, resp.Status) |
| if participation.PrevData == nil { | ||
| participation.PrevData = make(promptTypes.MetaData) | ||
| } | ||
| participation.PrevData[res.DtoName] = data | ||
| participation.PrevData[res.DtoName] = resolvedData | ||
| cppWithRes.Participation = participation |
There was a problem hiding this comment.
Consider adding a unit/integration test for FetchAndMergeCourseParticipationWithResolution to ensure the resolved payload (from ResolveParticipation) is what gets written into Participation.PrevData. This line previously wrote the raw participation response bytes, so a regression here would be hard to spot without a test (can be done with an httptest server backing FetchJSON).
| return keycloakTokenVerifierDTO.GetCoursePhaseParticipation{IsStudentOfCoursePhase: false}, errors.New("not student of course") | ||
| } | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| log.Error("Received non-OK response:", resp.Status) | ||
| return keycloakTokenVerifierDTO.GetCoursePhaseParticipation{}, nil | ||
| return keycloakTokenVerifierDTO.GetCoursePhaseParticipation{}, fmt.Errorf("is student request failed with status %d: %s", resp.StatusCode, resp.Status) |
There was a problem hiding this comment.
The "not student of course" path relies on a stringly-typed error; callers currently compare err.Error() to decide behavior. Since this function is already being updated to return richer errors, it would be more robust to return a sentinel error (e.g., ErrNotStudent) and have callers use errors.Is instead of string comparison.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
keycloakTokenVerifier/keycloakCoreRequests/isStudent.go (1)
29-36:⚠️ Potential issue | 🟠 MajorUse sentinel and typed errors to make the error contract inspectable.
The caller in
getIsStudent.go:33currently relies onerr.Error() == "not student of course"and treats all other errors as500. Returning ad-hocerrors.New(...)/fmt.Errorf(...)keeps that brittle contract in place and loses the upstream HTTP status from non-401 failures. Define a sentinel error for the unauthorized case and a typed status error for other non-200responses so callers can useerrors.Is/errors.Asinstead of string matching. The codebase already follows this pattern elsewhere (e.g.,ErrUserNotInContext).Possible direction
+var ErrNotStudentOfCourse = errors.New("not student of course") + if resp.StatusCode == http.StatusUnauthorized { log.Info("Not student of course") - return keycloakTokenVerifierDTO.GetCoursePhaseParticipation{IsStudentOfCoursePhase: false}, errors.New("not student of course") + return keycloakTokenVerifierDTO.GetCoursePhaseParticipation{IsStudentOfCoursePhase: false}, ErrNotStudentOfCourse } if resp.StatusCode != http.StatusOK { log.Error("Received non-OK response:", resp.Status) - return keycloakTokenVerifierDTO.GetCoursePhaseParticipation{}, fmt.Errorf("is student request failed with status %d: %s", resp.StatusCode, resp.Status) + return keycloakTokenVerifierDTO.GetCoursePhaseParticipation{}, CoreStatusError{StatusCode: resp.StatusCode, Status: resp.Status} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keycloakTokenVerifier/keycloakCoreRequests/isStudent.go` around lines 29 - 36, Replace the ad-hoc errors in the Keycloak student-check path with an inspectable contract: introduce a package-level sentinel error (e.g., ErrNotStudent) and a typed error (e.g., HTTPStatusError with fields StatusCode and Status string implementing error) and return ErrNotStudent from the unauthorized branch in isStudent.go instead of errors.New("not student of course"), and return an instance of HTTPStatusError (populated with resp.StatusCode and resp.Status) for any non-200 responses instead of fmt.Errorf; then update the caller in getIsStudent.go to use errors.Is(err, ErrNotStudent) and errors.As(err, &HTTPStatusError{}) rather than string comparisons so the upstream status and identity are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keycloakTokenVerifier/keycloakCoreRequests/coursePhaseRoleMapping.go`:
- Around line 28-30: Replace the plain fmt.Errorf return in
SendCoursePhaseRoleMappingRequest with a typed error that carries the HTTP
status so callers can inspect it; define a CoreStatusError struct (fields
StatusCode int, Status string) that implements error (Error() string) and return
&CoreStatusError{StatusCode: resp.StatusCode, Status: resp.Status} instead of
fmt.Errorf in the non-OK branch, and update callers like
GetLecturerAndEditorRole (which currently calls
SendCoursePhaseRoleMappingRequest) to do a type assertion on CoreStatusError to
map/forward the actual HTTP status code.
---
Outside diff comments:
In `@keycloakTokenVerifier/keycloakCoreRequests/isStudent.go`:
- Around line 29-36: Replace the ad-hoc errors in the Keycloak student-check
path with an inspectable contract: introduce a package-level sentinel error
(e.g., ErrNotStudent) and a typed error (e.g., HTTPStatusError with fields
StatusCode and Status string implementing error) and return ErrNotStudent from
the unauthorized branch in isStudent.go instead of errors.New("not student of
course"), and return an instance of HTTPStatusError (populated with
resp.StatusCode and resp.Status) for any non-200 responses instead of
fmt.Errorf; then update the caller in getIsStudent.go to use errors.Is(err,
ErrNotStudent) and errors.As(err, &HTTPStatusError{}) rather than string
comparisons so the upstream status and identity are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 478574be-3bba-4c65-8ff7-831e2ba71571
📒 Files selected for processing (3)
keycloakTokenVerifier/keycloakCoreRequests/coursePhaseRoleMapping.gokeycloakTokenVerifier/keycloakCoreRequests/isStudent.goresolution.go
| if resp.StatusCode != http.StatusOK { | ||
| log.Error("Received non-OK response:", resp.Status) | ||
| return keycloakTokenVerifierDTO.GetCourseRoles{}, nil | ||
| return keycloakTokenVerifierDTO.GetCourseRoles{}, fmt.Errorf("course phase role mapping request failed with status %d: %s", resp.StatusCode, resp.Status) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "coursePhaseRoleMapping.go" | head -20Repository: prompt-edu/prompt-sdk
Length of output: 135
🏁 Script executed:
find . -type f -name "getLecturerAndEditorRole.go" | head -20Repository: prompt-edu/prompt-sdk
Length of output: 116
🏁 Script executed:
cat -n ./keycloakTokenVerifier/keycloakCoreRequests/coursePhaseRoleMapping.go | sed -n '20,40p'Repository: prompt-edu/prompt-sdk
Length of output: 929
🏁 Script executed:
cat -n ./keycloakTokenVerifier/getLecturerAndEditorRole.go | sed -n '25,45p'Repository: prompt-edu/prompt-sdk
Length of output: 955
🏁 Script executed:
rg "SendCoursePhaseRoleMappingRequest" --type go -B 2 -A 5Repository: prompt-edu/prompt-sdk
Length of output: 1790
🏁 Script executed:
find ./keycloakTokenVerifier -type f -name "*.go" | head -20Repository: prompt-edu/prompt-sdk
Length of output: 813
🏁 Script executed:
rg "type.*Error.*struct" ./keycloakTokenVerifier --type goRepository: prompt-edu/prompt-sdk
Length of output: 47
🏁 Script executed:
cat -n ./keycloakTokenVerifier/keycloakCoreRequests/sendCoreRequest.goRepository: prompt-edu/prompt-sdk
Length of output: 1026
🏁 Script executed:
cat -n ./keycloakTokenVerifier/keycloakCoreRequests/isStudent.go | sed -n '1,50p'Repository: prompt-edu/prompt-sdk
Length of output: 1943
🏁 Script executed:
rg "SendIsStudentRequest" --type go -B 2 -A 5Repository: prompt-edu/prompt-sdk
Length of output: 1641
🏁 Script executed:
cat -n ./keycloakTokenVerifier/getIsStudent.go | sed -n '25,50p'Repository: prompt-edu/prompt-sdk
Length of output: 1333
Return a status-carrying error, not just a formatted message.
keycloakTokenVerifier/getLecturerAndEditorRole.go:34 unconditionally returns http.StatusInternalServerError for any error from SendCoursePhaseRoleMappingRequest. Since this function returns a plain fmt.Errorf with the status code only in the message string, upstream callers cannot distinguish 401/404/503 without string parsing and cannot forward the actual HTTP status. Use a typed error that carries resp.StatusCode and let handlers map that to the appropriate response status.
Consider defining a CoreStatusError struct with StatusCode and Status fields, implementing the error interface, and returning that instead of fmt.Errorf.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@keycloakTokenVerifier/keycloakCoreRequests/coursePhaseRoleMapping.go` around
lines 28 - 30, Replace the plain fmt.Errorf return in
SendCoursePhaseRoleMappingRequest with a typed error that carries the HTTP
status so callers can inspect it; define a CoreStatusError struct (fields
StatusCode int, Status string) that implements error (Error() string) and return
&CoreStatusError{StatusCode: resp.StatusCode, Status: resp.Status} instead of
fmt.Errorf in the non-OK branch, and update callers like
GetLecturerAndEditorRole (which currently calls
SendCoursePhaseRoleMappingRequest) to do a type assertion on CoreStatusError to
map/forward the actual HTTP status code.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func SendCoursePhaseRoleMappingRequest(coreURL url.URL, authHeader string, coursePhaseID uuid.UUID) (keycloakTokenVerifierDTO.GetCourseRoles, int, error) { | ||
| path := path.Join("/api/auth/course_phase", coursePhaseID.String(), "roles") | ||
|
|
||
| resp, err := sendRequest(coreURL, "GET", path, authHeader, nil) | ||
| if err != nil { | ||
| return keycloakTokenVerifierDTO.GetCourseRoles{}, err | ||
| return keycloakTokenVerifierDTO.GetCourseRoles{}, http.StatusInternalServerError, err |
There was a problem hiding this comment.
SendCoursePhaseRoleMappingRequest now has a different exported signature (adds an int status code return). This is a breaking API change for any external consumers of keycloakCoreRequests. Consider keeping the original signature and encoding the status code into the returned error (e.g., a typed error), or introduce a new function instead; if the signature change is intentional, it should be explicitly called out in the upgrade documentation/changelog.
Summary
Three bugs are fixed in this PR:
1.
coursePhaseRoleMapping.go— propagate non-OK Core responses as errorsPreviously, a non-OK HTTP response from Core was logged but returned with a
nilerror, causing callers to silently treat outages or misconfigurations as an empty role set. The non-OK branch now returns a descriptivefmt.Errorfso callers can detect and handle upstream failures.2.
isStudent.go— propagate non-OK Core responses as errorsSame issue: any non-OK response other than
401 Unauthorizedwas swallowed. The401path ("not a student") is intentional and unchanged. All other non-OK status codes now return a non-nil error, preventing Core outages from being misinterpreted as a successful empty lookup.3.
resolution.go— store resolved payload instead of raw bytes in metadataFetchAndMergeCourseParticipationWithResolutionwas storing the raw response bytes (data) intoparticipation.PrevData[res.DtoName]instead of the processed result (resolvedData) returned byResolveParticipation. Fixed to storeresolvedData.4.
README.md— document module import path migrationAdded an upgrade note calling out the breaking import-path change from
github.com/ls1intum/prompt-sdktogithub.com/prompt-edu/prompt-sdk, with instructions to rungo get github.com/prompt-edu/prompt-sdk@latestand update all import lines.Summary by CodeRabbit
Bug Fixes
Refactor
Chores