Skip to content

fix: do not require optional query parameters#260

Merged
cb80 merged 1 commit intomainfrom
queryParameters
Mar 23, 2026
Merged

fix: do not require optional query parameters#260
cb80 merged 1 commit intomainfrom
queryParameters

Conversation

@cb80
Copy link
Copy Markdown
Contributor

@cb80 cb80 commented Mar 23, 2026

In #250 we introduced single tenant support. In these cases we don't require the multi tenant relevant query parameters so let's skip them.

This is quick fix. Midterm we should setup proper trust types, so we exactly know, which query parameters are required.

Summary by CodeRabbit

Release Notes

  • Tests

    • Updated test infrastructure to use proper test context injection across multiple test suites.
  • Bug Fixes

    • Enhanced robustness: Missing or absent parameters in configuration mappings are now silently handled instead of causing request failures.

@cb80 cb80 self-assigned this Mar 23, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: afb1822a-f646-4c55-9571-efdfe24bbc00

📥 Commits

Reviewing files that changed from the base of the PR and between 4a1ca8a and 492662c.

📒 Files selected for processing (7)
  • internal/business/server/metrics_test.go
  • internal/credentials/credentials_test.go
  • internal/middleware/responsewriter_test.go
  • internal/session/housekeeper.go
  • internal/session/manager.go
  • internal/session/manager_test.go
  • internal/trust/mapping.go
💤 Files with no reviewable changes (1)
  • internal/session/manager_test.go

📝 Walkthrough

Walkthrough

Tests are updated to use t.Context() for request construction, and error-handling logic is modified across session and trust management to silently skip missing parameters instead of returning errors when configured keys are absent from their mappings.

Changes

Cohort / File(s) Summary
Test Context Propagation
internal/business/server/metrics_test.go, internal/credentials/credentials_test.go, internal/middleware/responsewriter_test.go
Updated test files to capture and reuse t.Context() when constructing HTTP requests via httptest.NewRequestWithContext(ctx, ...) instead of httptest.NewRequest(...).
Parameter Handling Logic
internal/session/housekeeper.go, internal/session/manager.go, internal/trust/mapping.go
Modified error-handling behavior to silently skip missing parameters/claims when they are absent from their mappings (if ok { ... }) instead of returning errors. Removed context and slog-context imports from mapping.go.
Test Case Removal
internal/session/manager_test.go
Removed "Auth context error" test case from TestManager_FinaliseOIDCLogin that validated error handling for missing auth context keys.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~18 minutes

Poem

🐰 Tests now dance with time they know,
Context flows where requests go,
Missing keys? No tears we shed,
Silent skips instead of dread—
Smoother paths the rabbits tread! 🌿✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning No pull request description was provided by the author. The template requires 'What this PR does / why we need it' section which is missing, making the description incomplete. Add a description explaining what this PR does and why it's needed. Include sections for objectives, any special notes for reviewers, and release notes per the template.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: do not require optional query parameters' clearly summarizes the main change across all modified files, which involve making query parameters optional instead of required.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch queryParameters

Comment @coderabbitai help to get the list of available commands and usage tips.

@cb80 cb80 marked this pull request as ready for review March 23, 2026 10:28
@cb80 cb80 merged commit 6e6523c into main Mar 23, 2026
8 checks passed
@cb80 cb80 deleted the queryParameters branch March 23, 2026 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants