Add localized client metadata support for applications and DCR#2286
Merged
ThaminduDilshan merged 1 commit intoasgardeo:mainfrom Apr 30, 2026
Merged
Add localized client metadata support for applications and DCR#2286ThaminduDilshan merged 1 commit intoasgardeo:mainfrom
ThaminduDilshan merged 1 commit intoasgardeo:mainfrom
Conversation
nandhu-kumar
requested review from
ThaminduDilshan,
rajithacharith and
thiva-k
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds BCP47-tagged localized client metadata to DCR flows, threads an Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DCR as DCR Service
participant DB as Database<br/>(Transaction)
participant I18n as I18n Service
participant AppSvc as Application Service
Client->>DCR: POST RegisterClient (body includes localized fields)
DCR->>DB: Begin transaction / CreateApplication
DB-->>AppSvc: appID (within tx)
DB->>DB: Commit
DCR->>I18n: For each localized field -> SetTranslationOverrideForKey(namespace, key, lang, value)
alt i18n writes succeed
I18n-->>DCR: ok
DCR->>Client: 201 Created (response includes localized maps)
else i18n write failure
I18n-->>DCR: error
DCR->>I18n: DeleteTranslationsByNamespace(namespace) -- compensation
DCR->>AppSvc: DeleteApplication(appID) -- compensation
DCR->>Client: 500 Server Error (mapped from i18n error)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2286 +/- ##
==========================================
- Coverage 86.29% 86.27% -0.02%
==========================================
Files 916 919 +3
Lines 65513 65806 +293
==========================================
+ Hits 56532 56772 +240
- Misses 6900 6927 +27
- Partials 2081 2107 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nandhu-kumar
force-pushed
the
main-local-2106
branch
from
100b645 to
7cd46ba
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/internal/oauth/oauth2/dcr/handler.go (1)
92-98: Consider logging server errors for the GET endpoint.Per repository conventions, server errors (HTTP 500) should be logged at the handler layer. The current implementation logs via
writeServiceErrorResponsefor generic errors, but the 404 path bypasses this. This is acceptable since 404 is a client error, but consider adding logging for unexpected server-type errors before returning 404:if svcErr.Code == ErrorClientNotFound.Code { sysutils.WriteJSONError(w, svcErr.Code, svcErr.ErrorDescription, http.StatusNotFound, nil) return } if svcErr.Type == serviceerror.ServerErrorType { logger := log.GetLogger().With(log.String(log.LoggerKeyComponentName, "DCRHandler")) logger.Error("Internal server error processing DCR GET request", log.String("app_id", appID), log.String("error_code", svcErr.Code), ) } dh.writeServiceErrorResponse(w, svcErr)This mirrors the logging pattern in
HandleDCRRegistration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/oauth/oauth2/dcr/handler.go` around lines 92 - 98, The GET handler currently returns 404 for ErrorClientNotFound but doesn't log server-type errors; update the svcErr handling in HandleDCRGet (handler.go) to keep the existing 404 path, and before calling dh.writeServiceErrorResponse(w, svcErr) add a check if svcErr.Type == serviceerror.ServerErrorType then create a logger via log.GetLogger().With(log.String(log.LoggerKeyComponentName, "DCRHandler")) and call logger.Error with a brief message like "Internal server error processing DCR GET request" including appID and svcErr.Code; this mirrors the logging pattern used in HandleDCRRegistration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/cmd/server/servicemanager.go`:
- Around line 269-270: Add documentation for the new public OAuth2 DCR read
endpoint introduced by oauth.Initialize: document the GET
/oauth2/dcr/register/{app_id} endpoint in docs (e.g., docs/content/apis.mdx or a
dedicated DCR guide), include request/response examples showing localized fields
using the `#` suffix (e.g., `client_name#fr`), specify the 404 response when a
client is not found, and link or reference the OAuth2 DCR behavior so consumers
of the API understand usage and error handling.
In `@backend/internal/oauth/oauth2/dcr/handler.go`:
- Around line 76-102: Add API docs for the new GET /oauth2/dcr/register/{app_id}
endpoint implemented by HandleDCRGetClient: create/update the relevant docs/*
API reference page to describe the endpoint path, HTTP method, authorization
requirements (same as DCR registration), request parameters (path param app_id),
full response schema including localized variant fields like client_name#<lang>
and logo_uri#<lang>, and list possible error responses (400 Invalid Request
Format, 401 Unauthorized when DCR auth fails, 404 Client Not Found) with
examples; ensure the response examples match the service return shape produced
by dh.dcrService.GetClient and add any necessary schema definitions and
cross-references used by the existing DCR docs.
In `@backend/internal/oauth/oauth2/dcr/service.go`:
- Around line 44-46: Add documentation for the new GET
/oauth2/dcr/register/{app_id} endpoint and the extended DCR request/response
schema that now supports language-tagged metadata keys (e.g.,
client_name#<lang>, logo_uri#<lang>, tos_uri#<lang>, policy_uri#<lang>); update
the API reference entry to include the new endpoint, request/response examples
showing localized keys, and expected error cases, and add or update the DCR
guide to describe how localized metadata is chosen/merged, validation rules, and
sample payloads so callers of GetClient and the DCR registration flow know how
to produce and consume the localized fields.
- Around line 381-398: The code currently ignores errors from
ds.i18nService.GetTranslationsByNamespace, causing GetClient to return success
while localized metadata is missing; update the GetClient implementation (the
block using ds.i18nService and calling GetTranslationsByNamespace) to handle
i18nErr explicitly by returning or propagating an error (wrap with context like
"failed to load localized metadata for app <appID>") instead of silently
skipping, so callers see a failure when translations can't be read; ensure you
reference ds.i18nService, GetTranslationsByNamespace, and the GetClient response
handling
(response.LocalizedClientName/LocalizedLogoURI/LocalizedTosURI/LocalizedPolicyURI)
when making this change.
---
Nitpick comments:
In `@backend/internal/oauth/oauth2/dcr/handler.go`:
- Around line 92-98: The GET handler currently returns 404 for
ErrorClientNotFound but doesn't log server-type errors; update the svcErr
handling in HandleDCRGet (handler.go) to keep the existing 404 path, and before
calling dh.writeServiceErrorResponse(w, svcErr) add a check if svcErr.Type ==
serviceerror.ServerErrorType then create a logger via
log.GetLogger().With(log.String(log.LoggerKeyComponentName, "DCRHandler")) and
call logger.Error with a brief message like "Internal server error processing
DCR GET request" including appID and svcErr.Code; this mirrors the logging
pattern used in HandleDCRRegistration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6e749e4-3b4f-49c2-92d2-4450aad3397f
⛔ Files ignored due to path filters (2)
backend/tests/mocks/i18n/mgtmock/I18nServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/i18n/mgtmock/i18nStoreInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (22)
backend/cmd/server/servicemanager.gobackend/internal/application/init.gobackend/internal/application/init_test.gobackend/internal/application/service.gobackend/internal/oauth/init.gobackend/internal/oauth/oauth2/dcr/DCRServiceInterface_mock_test.gobackend/internal/oauth/oauth2/dcr/error_constants.gobackend/internal/oauth/oauth2/dcr/handler.gobackend/internal/oauth/oauth2/dcr/init.gobackend/internal/oauth/oauth2/dcr/init_test.gobackend/internal/oauth/oauth2/dcr/model.gobackend/internal/oauth/oauth2/dcr/service.gobackend/internal/oauth/oauth2/dcr/service_test.gobackend/internal/system/i18n/mgt/I18nServiceInterface_mock_test.gobackend/internal/system/i18n/mgt/constants.gobackend/internal/system/i18n/mgt/file_based_store.gobackend/internal/system/i18n/mgt/i18nStoreInterface_mock_test.gobackend/internal/system/i18n/mgt/service.gobackend/internal/system/i18n/mgt/store.gobackend/internal/system/i18n/mgt/store_constants.gotests/integration/oauth/dcr/dcr_test.gotests/integration/oauth/dcr/model.go
nandhu-kumar
force-pushed
the
main-local-2106
branch
from
7cd46ba to
27d9609
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/internal/oauth/oauth2/dcr/service.go (1)
149-160: Keep 500 logging in the handler, not the service.These new
logger.Errorcalls will double-log the same server failure once the handler writes the 500. Return theServiceErrorhere and let the DCR handlers own the error log.Based on learnings, "ensure server (HTTP 500/InternalServerError) errors are logged at the handler layer, while services return a ServiceError without logging."
Also applies to: 335-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/oauth/oauth2/dcr/service.go` around lines 149 - 160, The service currently logs internal errors (logger.Error) during i18n write compensation and delete failures inside the DCR service method; remove these logger.Error calls and instead return a ServiceError (or wrap the original error) from the method so the DCR handlers can log the 500. Specifically, in the block that calls ds.i18nService.DeleteTranslationsByNamespace("app."+createdAppID) and ds.appService.DeleteApplication(ctx, createdAppID) remove the logger.Error invocations and ensure the function returns a ServiceError containing the underlying error (preserving context like createdAppID) for the handler to log; apply the same change to the other similar logging at the referenced lines (335-337).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/oauth/oauth2/dcr/handler.go`:
- Around line 91-98: HandleDCRGetClient is missing server-side logging when
dh.dcrService.GetClient returns a non-not-found error; mirror the pattern used
in HandleDCRRegistration by logging the svcErr before responding. Modify the
error branch in HandleDCRGetClient: after detecting svcErr and before calling
dh.writeServiceErrorResponse (and in the not-found branch if desired), call the
handler's logger to log svcErr (include svcErr.Code and svcErr.ErrorDescription
or the whole error) so failures from dh.dcrService.GetClient are recorded;
ensure you reference dh.dcrService.GetClient, svcErr, HandleDCRGetClient and
dh.writeServiceErrorResponse when locating the change.
In `@backend/internal/oauth/oauth2/dcr/service.go`:
- Around line 340-345: The code assumes the OAuth entry lives at
app.InboundAuthConfig[0]; instead iterate the app.InboundAuthConfig slice to
find the element whose OAuthAppConfig is non-nil, assign that to oauthCfg, and
only if none found return &ErrorClientNotFound; then compute scopeString from
oauthCfg.Scopes as before. Update references to
app.InboundAuthConfig[0].OAuthAppConfig, oauthCfg, and scopeString accordingly
so the logic works regardless of entry order.
---
Nitpick comments:
In `@backend/internal/oauth/oauth2/dcr/service.go`:
- Around line 149-160: The service currently logs internal errors (logger.Error)
during i18n write compensation and delete failures inside the DCR service
method; remove these logger.Error calls and instead return a ServiceError (or
wrap the original error) from the method so the DCR handlers can log the 500.
Specifically, in the block that calls
ds.i18nService.DeleteTranslationsByNamespace("app."+createdAppID) and
ds.appService.DeleteApplication(ctx, createdAppID) remove the logger.Error
invocations and ensure the function returns a ServiceError containing the
underlying error (preserving context like createdAppID) for the handler to log;
apply the same change to the other similar logging at the referenced lines
(335-337).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3364a99-84b2-4ddd-b928-3d4b6cd881bb
⛔ Files ignored due to path filters (2)
backend/tests/mocks/i18n/mgtmock/I18nServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/i18n/mgtmock/i18nStoreInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (20)
backend/cmd/server/servicemanager.gobackend/internal/application/init.gobackend/internal/application/init_test.gobackend/internal/application/service.gobackend/internal/oauth/init.gobackend/internal/oauth/oauth2/dcr/DCRServiceInterface_mock_test.gobackend/internal/oauth/oauth2/dcr/error_constants.gobackend/internal/oauth/oauth2/dcr/handler.gobackend/internal/oauth/oauth2/dcr/init.gobackend/internal/oauth/oauth2/dcr/init_test.gobackend/internal/oauth/oauth2/dcr/model.gobackend/internal/oauth/oauth2/dcr/service.gobackend/internal/oauth/oauth2/dcr/service_test.gobackend/internal/system/i18n/mgt/I18nServiceInterface_mock_test.gobackend/internal/system/i18n/mgt/constants.gobackend/internal/system/i18n/mgt/file_based_store.gobackend/internal/system/i18n/mgt/i18nStoreInterface_mock_test.gobackend/internal/system/i18n/mgt/service.gobackend/internal/system/i18n/mgt/store.gobackend/internal/system/i18n/mgt/store_constants.go
✅ Files skipped from review due to trivial changes (2)
- backend/internal/oauth/oauth2/dcr/error_constants.go
- backend/internal/system/i18n/mgt/store_constants.go
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/internal/oauth/oauth2/dcr/init_test.go
- backend/internal/oauth/init.go
- backend/internal/application/init_test.go
- backend/internal/application/init.go
- backend/internal/system/i18n/mgt/constants.go
- backend/internal/oauth/oauth2/dcr/service_test.go
- backend/internal/oauth/oauth2/dcr/DCRServiceInterface_mock_test.go
- backend/internal/system/i18n/mgt/i18nStoreInterface_mock_test.go
nandhu-kumar
force-pushed
the
main-local-2106
branch
from
27d9609 to
fe54da6
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/internal/oauth/oauth2/dcr/handler.go (1)
76-110:⚠️ Potential issue | 🟠 Major🔴 Documentation Required: This PR introduces a user-facing change (the new
GET /oauth2/dcr/register/{app_id}endpoint plus language-tagged DCR metadata fields likeclient_name#<lang>,logo_uri#<lang>,tos_uri#<lang>, andpolicy_uri#<lang>) but does not include corresponding documentation updates underdocs/. Please update the API reference for the new GET route and the DCR docs for the request/response schema, auth requirements, and 400/401/404 responses before merging.docs/content/apis.mdxand the DCR guide underdocs/content/guides/are the likely places.As per coding guidelines, "🔴 Documentation Required: This PR introduces a user-facing change (e.g., new/modified API endpoint, configuration option, authentication flow, or SDK behavior) but does not include corresponding documentation updates under
docs/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/oauth/oauth2/dcr/handler.go` around lines 76 - 110, Add documentation for the new GET /oauth2/dcr/register/{app_id} endpoint introduced by HandleDCRGetClient: update the API reference and the DCR guide to describe the route, authentication requirements (when DCR.Insecure is false), request/response schema including localized metadata fields client_name#<lang>, logo_uri#<lang>, tos_uri#<lang>, policy_uri#<lang>, and example responses for 200, 400 (invalid request format), 401 (auth failure), and 404 (ErrorClientNotFound). Ensure the docs mention the possible server error path (serviceerror.ServerErrorType) and include a minimal example curl request and JSON response to show the language-tagged fields.
🧹 Nitpick comments (1)
backend/internal/oauth/oauth2/dcr/service.go (1)
144-168: Verify compensation atomicity on partial i18n write failure.The compensation logic deletes i18n rows and then the created application on write failure. However, if the i18n write partially succeeds (e.g., writes 2 of 4 localized fields before failing),
DeleteTranslationsByNamespaceshould clean up all partial rows. This looks correct since it deletes by namespace rather than individual keys.One edge case: if
ds.i18nService.DeleteTranslationsByNamespaceitself fails (line 152-155), the code logs but continues to delete the app. This could leave orphaned i18n rows if both the cleanup and app deletion succeed partially. Consider whether this is acceptable or if the app deletion should be skipped when cleanup fails to maintain referential integrity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/oauth/oauth2/dcr/service.go` around lines 144 - 168, The compensation path after writeLocalizedVariants may leave orphaned i18n rows because you currently log a DeleteTranslationsByNamespace failure and still proceed to DeleteApplication; change the flow in the DCR service: in the block following writeLocalizedVariants (the code using ds.writeLocalizedVariants, ds.i18nService.DeleteTranslationsByNamespace, and ds.appService.DeleteApplication), if ds.i18nService.DeleteTranslationsByNamespace returns an error, log it and abort further compensation (do not call ds.appService.DeleteApplication) and return an appropriate server error; only call ds.appService.DeleteApplication when i18n cleanup succeeded (or when ds.i18nService is nil), ensuring you reference the same symbols (writeLocalizedVariants, ds.i18nService.DeleteTranslationsByNamespace, ds.appService.DeleteApplication, and ErrorServerError) to locate where to change control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/oauth/dcr/model.go`:
- Around line 96-137: The UnmarshalJSON for DCRRegistrationResponse currently
appends into existing LocalizedClientName, LocalizedLogoURI, LocalizedTosURI,
and LocalizedPolicyURI maps so reused instances retain stale entries; modify
UnmarshalJSON (method name UnmarshalJSON on type DCRRegistrationResponse) to
reset those localized maps before parsing the raw keys (e.g., set
r.LocalizedClientName, r.LocalizedLogoURI, r.LocalizedTosURI,
r.LocalizedPolicyURI to nil or make(map[string]string) at the top of the method
before the loop) so only keys present in the current payload are kept.
---
Duplicate comments:
In `@backend/internal/oauth/oauth2/dcr/handler.go`:
- Around line 76-110: Add documentation for the new GET
/oauth2/dcr/register/{app_id} endpoint introduced by HandleDCRGetClient: update
the API reference and the DCR guide to describe the route, authentication
requirements (when DCR.Insecure is false), request/response schema including
localized metadata fields client_name#<lang>, logo_uri#<lang>, tos_uri#<lang>,
policy_uri#<lang>, and example responses for 200, 400 (invalid request format),
401 (auth failure), and 404 (ErrorClientNotFound). Ensure the docs mention the
possible server error path (serviceerror.ServerErrorType) and include a minimal
example curl request and JSON response to show the language-tagged fields.
---
Nitpick comments:
In `@backend/internal/oauth/oauth2/dcr/service.go`:
- Around line 144-168: The compensation path after writeLocalizedVariants may
leave orphaned i18n rows because you currently log a
DeleteTranslationsByNamespace failure and still proceed to DeleteApplication;
change the flow in the DCR service: in the block following
writeLocalizedVariants (the code using ds.writeLocalizedVariants,
ds.i18nService.DeleteTranslationsByNamespace, and
ds.appService.DeleteApplication), if
ds.i18nService.DeleteTranslationsByNamespace returns an error, log it and abort
further compensation (do not call ds.appService.DeleteApplication) and return an
appropriate server error; only call ds.appService.DeleteApplication when i18n
cleanup succeeded (or when ds.i18nService is nil), ensuring you reference the
same symbols (writeLocalizedVariants,
ds.i18nService.DeleteTranslationsByNamespace, ds.appService.DeleteApplication,
and ErrorServerError) to locate where to change control flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e737fec-10e9-48a4-b4ad-254676658cb7
⛔ Files ignored due to path filters (2)
backend/tests/mocks/i18n/mgtmock/I18nServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/i18n/mgtmock/i18nStoreInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (28)
backend/cmd/server/servicemanager.gobackend/internal/application/init.gobackend/internal/application/init_test.gobackend/internal/application/service.gobackend/internal/application/service_test.gobackend/internal/oauth/init.gobackend/internal/oauth/oauth2/dcr/DCRServiceInterface_mock_test.gobackend/internal/oauth/oauth2/dcr/error_constants.gobackend/internal/oauth/oauth2/dcr/handler.gobackend/internal/oauth/oauth2/dcr/handler_test.gobackend/internal/oauth/oauth2/dcr/init.gobackend/internal/oauth/oauth2/dcr/init_test.gobackend/internal/oauth/oauth2/dcr/model.gobackend/internal/oauth/oauth2/dcr/model_test.gobackend/internal/oauth/oauth2/dcr/service.gobackend/internal/oauth/oauth2/dcr/service_test.gobackend/internal/system/i18n/mgt/I18nServiceInterface_mock_test.gobackend/internal/system/i18n/mgt/constants.gobackend/internal/system/i18n/mgt/file_based_store.gobackend/internal/system/i18n/mgt/file_based_store_test.gobackend/internal/system/i18n/mgt/i18nStoreInterface_mock_test.gobackend/internal/system/i18n/mgt/service.gobackend/internal/system/i18n/mgt/service_test.gobackend/internal/system/i18n/mgt/store.gobackend/internal/system/i18n/mgt/store_constants.gobackend/internal/system/i18n/mgt/store_test.gotests/integration/oauth/dcr/dcr_test.gotests/integration/oauth/dcr/model.go
✅ Files skipped from review due to trivial changes (5)
- backend/internal/oauth/oauth2/dcr/error_constants.go
- backend/internal/system/i18n/mgt/file_based_store.go
- backend/internal/application/init_test.go
- backend/internal/system/i18n/mgt/store_constants.go
- backend/internal/application/service_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/cmd/server/servicemanager.go
- backend/internal/application/init.go
- backend/internal/oauth/init.go
- backend/internal/system/i18n/mgt/constants.go
- backend/internal/oauth/oauth2/dcr/init.go
- backend/internal/oauth/oauth2/dcr/init_test.go
- backend/internal/oauth/oauth2/dcr/DCRServiceInterface_mock_test.go
- backend/internal/oauth/oauth2/dcr/service_test.go
nandhu-kumar
force-pushed
the
main-local-2106
branch
from
fe54da6 to
12b8cbf
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
nandhu-kumar
changed the title
nandhu-kumar
force-pushed
the
main-local-2106
branch
from
12b8cbf to
dbdfeb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/cmd/server/servicemanager.go (1)
269-270:⚠️ Potential issue | 🟠 Major🔴 Documentation Required: This PR introduces a user-facing change but does not include corresponding documentation updates under
docs/.
Please documentGET /oauth2/dcr/register/{app_id}and the new localized DCR metadata keys (client_name#<lang>,logo_uri#<lang>,tos_uri#<lang>,policy_uri#<lang>), including the 400 invalid-language-tag and 404 not-found cases, indocs/content/apis.mdxand the DCR guide before merge.As per coding guidelines, "🔴 Documentation Required: This PR introduces a user-facing change (e.g., new/modified API endpoint, configuration option, authentication flow, or SDK behavior) but does not include corresponding documentation updates under
docs/. Please update the relevant documentation before merging."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cmd/server/servicemanager.go` around lines 269 - 270, Add user-facing documentation for the new OAuth DCR endpoint and localized metadata: document the GET /oauth2/dcr/register/{app_id} endpoint (purpose, request/response, status codes), list the new localized DCR metadata keys (client_name#<lang>, logo_uri#<lang>, tos_uri#<lang>, policy_uri#<lang>), and explicitly document the 400 invalid-language-tag and 404 not-found error cases; add these entries to the API reference (apis.mdx) and the Dynamic Client Registration guide so the new behavior is fully described before merging.
🧹 Nitpick comments (1)
backend/internal/system/i18n/mgt/service.go (1)
257-276:TotalResultsmay be misleading after filtering.Line 285 sets
TotalResults: len(allTranslations), but for non-system namespaces with exact-match logic, some entries may be skipped (lines 274-276), causingTotalResultsto exceed the actual number of entries in the result map.Consider whether this is intentional (total stored vs. total returned) or if it should reflect the filtered count.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/system/i18n/mgt/service.go` around lines 257 - 276, The reported TotalResults uses len(allTranslations) which can be larger than the actually returned entries because non-system namespaces use exactLanguageMatch and some translations are skipped (compositeKey, allTranslations, translation, selectBestTranslation, exactLanguageMatch). Change TotalResults to reflect the filtered/returned count by computing the length of the results map after the loop (or incrementing a counter when you add a translation.Value to the result) instead of using len(allTranslations), or if you intentionally want stored vs returned semantics, explicitly include both a StoredTotal (len(allTranslations)) and ReturnedTotal (len(filteredResults)) in the response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/oauth/oauth2/dcr/service.go`:
- Around line 153-166: The rollback currently uses the original request context
so if the caller times out the compensation (writeLocalizedVariants cleanup and
appService.DeleteApplication) may be skipped; change the compensation path in
the error branch after writeLocalizedVariants to create a bounded background
context (e.g., ctxCleanup := context.WithTimeout(context.Background(),
someReasonableTimeout) and defer cancel()) and use ctxCleanup when calling
ds.i18nService.DeleteTranslationsByNamespace(application.AppI18nNamespace(createdAppID))
and ds.appService.DeleteApplication(ctxCleanup, createdAppID); keep existing
error logging and return &ErrorServerError as before.
---
Duplicate comments:
In `@backend/cmd/server/servicemanager.go`:
- Around line 269-270: Add user-facing documentation for the new OAuth DCR
endpoint and localized metadata: document the GET /oauth2/dcr/register/{app_id}
endpoint (purpose, request/response, status codes), list the new localized DCR
metadata keys (client_name#<lang>, logo_uri#<lang>, tos_uri#<lang>,
policy_uri#<lang>), and explicitly document the 400 invalid-language-tag and 404
not-found error cases; add these entries to the API reference (apis.mdx) and the
Dynamic Client Registration guide so the new behavior is fully described before
merging.
---
Nitpick comments:
In `@backend/internal/system/i18n/mgt/service.go`:
- Around line 257-276: The reported TotalResults uses len(allTranslations) which
can be larger than the actually returned entries because non-system namespaces
use exactLanguageMatch and some translations are skipped (compositeKey,
allTranslations, translation, selectBestTranslation, exactLanguageMatch). Change
TotalResults to reflect the filtered/returned count by computing the length of
the results map after the loop (or incrementing a counter when you add a
translation.Value to the result) instead of using len(allTranslations), or if
you intentionally want stored vs returned semantics, explicitly include both a
StoredTotal (len(allTranslations)) and ReturnedTotal (len(filteredResults)) in
the response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf5ba964-e166-43f8-886b-3b86d537c351
⛔ Files ignored due to path filters (2)
backend/tests/mocks/i18n/mgtmock/I18nServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/i18n/mgtmock/i18nStoreInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (28)
backend/cmd/server/servicemanager.gobackend/internal/application/init.gobackend/internal/application/init_test.gobackend/internal/application/service.gobackend/internal/application/service_test.gobackend/internal/oauth/init.gobackend/internal/oauth/oauth2/dcr/DCRServiceInterface_mock_test.gobackend/internal/oauth/oauth2/dcr/error_constants.gobackend/internal/oauth/oauth2/dcr/handler.gobackend/internal/oauth/oauth2/dcr/handler_test.gobackend/internal/oauth/oauth2/dcr/init.gobackend/internal/oauth/oauth2/dcr/init_test.gobackend/internal/oauth/oauth2/dcr/model.gobackend/internal/oauth/oauth2/dcr/model_test.gobackend/internal/oauth/oauth2/dcr/service.gobackend/internal/oauth/oauth2/dcr/service_test.gobackend/internal/system/i18n/mgt/I18nServiceInterface_mock_test.gobackend/internal/system/i18n/mgt/constants.gobackend/internal/system/i18n/mgt/file_based_store.gobackend/internal/system/i18n/mgt/file_based_store_test.gobackend/internal/system/i18n/mgt/i18nStoreInterface_mock_test.gobackend/internal/system/i18n/mgt/service.gobackend/internal/system/i18n/mgt/service_test.gobackend/internal/system/i18n/mgt/store.gobackend/internal/system/i18n/mgt/store_constants.gobackend/internal/system/i18n/mgt/store_test.gotests/integration/oauth/dcr/dcr_test.gotests/integration/oauth/dcr/model.go
✅ Files skipped from review due to trivial changes (3)
- backend/internal/system/i18n/mgt/file_based_store_test.go
- backend/internal/oauth/oauth2/dcr/error_constants.go
- backend/internal/system/i18n/mgt/file_based_store.go
🚧 Files skipped from review as they are similar to previous changes (13)
- backend/internal/oauth/init.go
- backend/internal/application/init.go
- backend/internal/oauth/oauth2/dcr/init_test.go
- backend/internal/system/i18n/mgt/store_test.go
- backend/internal/oauth/oauth2/dcr/handler.go
- backend/internal/system/i18n/mgt/store_constants.go
- backend/internal/oauth/oauth2/dcr/handler_test.go
- backend/internal/oauth/oauth2/dcr/init.go
- backend/internal/application/service_test.go
- backend/internal/system/i18n/mgt/i18nStoreInterface_mock_test.go
- tests/integration/oauth/dcr/dcr_test.go
- backend/internal/oauth/oauth2/dcr/service_test.go
- backend/internal/system/i18n/mgt/I18nServiceInterface_mock_test.go
nandhu-kumar
force-pushed
the
main-local-2106
branch
from
dbdfeb9 to
acf373d
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
backend/internal/oauth/oauth2/dcr/handler.go (1)
76-110:⚠️ Potential issue | 🟠 Major🔴 Documentation Required: This PR introduces a user-facing change, but there are still no matching docs updates under
docs/. Please document the newGET /oauth2/dcr/register/{app_id}endpoint and the localized DCR metadata fields (client_name#<lang>,logo_uri#<lang>,tos_uri#<lang>,policy_uri#<lang>) indocs/content/apis.mdx, and update the DCR guide underdocs/content/guides/with request/response examples and error cases before merge.As per coding guidelines "🔴 Documentation Required: This PR introduces a user-facing change (e.g., new/modified API endpoint, configuration option, authentication flow, or SDK behavior) but does not include corresponding documentation updates under
docs/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/oauth/oauth2/dcr/handler.go` around lines 76 - 110, Add documentation for the new GET /oauth2/dcr/register/{app_id} endpoint implemented by HandleDCRGetClient: document the endpoint path, purpose, authentication/authorization behavior, example request/response payloads, and possible error cases (including ErrorClientNotFound and server errors). Additionally, describe the localized DCR metadata fields (client_name#<lang>, logo_uri#<lang>, tos_uri#<lang>, policy_uri#<lang>) and how they should be used, and update the DCR guide with request/response examples and error handling; place these docs in docs/content/apis.mdx and the DCR guide under docs/content/guides/ per repo conventions.
🧹 Nitpick comments (1)
backend/internal/system/i18n/mgt/service_test.go (1)
620-647: Keep this coverage in the testify suite.This is the only new test in the file using plain
testing.Tand manualt.Errorfchecks. Please fold it intoI18nMgtServiceTestSuiteand use testify assertions so it stays consistent with the rest of the backend test setup.As per coding guidelines "Applies to backend/**/*_test.go : Use
stretchr/testifyfor tests and follow the test suite pattern".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/system/i18n/mgt/service_test.go` around lines 620 - 647, The standalone TestNormaliseBCP47Tag should be moved into the I18nMgtServiceTestSuite as a suite method (func (s *I18nMgtServiceTestSuite) TestNormaliseBCP47Tag()) and converted to use testify assertions instead of t.Errorf; call the existing NormaliseBCP47Tag inside the loop and use s.T().Run for subtests if desired, and replace manual checks with s.Require().Equal / s.Require().False / s.Require().True (or s.Assertions.Equal/True/False) to assert got vs wantTag and valid vs wantValid; remove the original top-level TestNormaliseBCP47Tag function after adding the suite method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/oauth/init.go`:
- Line 84: Add documentation for the new DCR endpoint and localized metadata
behavior: update docs/content/apis.mdx to document GET
/oauth2/dcr/register/{app_id} (endpoint path, parameters, expected responses
including 200, 400 for invalid BCP47 tags, and 404 when app_id not found) and
add a guide under docs/content/guides/ describing request/response examples, the
allowed localized client metadata keys (e.g., client_name#<lang-tag>,
logo_uri#<lang-tag>), the BCP 47 tag validation rules and examples of
valid/invalid tags, and explicit 400/404 response bodies; reference the
implementation entry point dcr.Initialize and the localized-key naming
convention in the guide so readers can map docs to code.
In `@backend/internal/oauth/oauth2/dcr/model.go`:
- Around line 51-55: The PR adds localized DCR metadata fields
(LocalizedClientName, LocalizedLogoURI, LocalizedTosURI, LocalizedPolicyURI
exposed as client_name#<lang>, logo_uri#<lang>, tos_uri#<lang>,
policy_uri#<lang>) and a new GET endpoint GET /oauth2/dcr/register/{app_id} but
did not update documentation; update the DCR API reference to document the new
localized metadata keys and the GET /oauth2/dcr/register/{app_id} response, and
add or extend a guide that shows how to register clients with localized fields
and how to retrieve them via the new client-read API so docs and code remain in
sync.
In `@backend/internal/oauth/oauth2/dcr/service.go`:
- Around line 323-327: The loop that calls
ds.i18nService.SetTranslationOverrideForKey for each f.variants currently maps
every failure to ErrorServerError, which masks client-side validation errors;
instead, propagate the actual svcErr (or translate it to the appropriate client
error) so localized validation failures are returned as client errors rather
than 500s, or pre-validate the localized maps (fields / f.variants) before
calling CreateApplication; update the loop in service.go to check the error
returned from SetTranslationOverrideForKey and return that error (or a mapped
client validation error) instead of always returning ErrorServerError.
- Around line 144-169: The detached compensation path uses
context.WithoutCancel(ctx) (cleanupCtx) which removes deadlines and can hang;
wrap that detached context with a bounded timeout before calling
ds.appService.DeleteApplication: create cleanupCtx :=
context.WithTimeout(context.WithoutCancel(ctx), <smallDuration>) (e.g., 3-10s)
and ensure you call cancel() (defer cancel()) so DeleteApplication cannot block
forever; keep the i18n cleanup using
ds.i18nService.DeleteTranslationsByNamespace(application.AppI18nNamespace(createdAppID))
unchanged and pass the new cleanupCtx into
ds.appService.DeleteApplication(cleanupCtx, createdAppID).
---
Duplicate comments:
In `@backend/internal/oauth/oauth2/dcr/handler.go`:
- Around line 76-110: Add documentation for the new GET
/oauth2/dcr/register/{app_id} endpoint implemented by HandleDCRGetClient:
document the endpoint path, purpose, authentication/authorization behavior,
example request/response payloads, and possible error cases (including
ErrorClientNotFound and server errors). Additionally, describe the localized DCR
metadata fields (client_name#<lang>, logo_uri#<lang>, tos_uri#<lang>,
policy_uri#<lang>) and how they should be used, and update the DCR guide with
request/response examples and error handling; place these docs in
docs/content/apis.mdx and the DCR guide under docs/content/guides/ per repo
conventions.
---
Nitpick comments:
In `@backend/internal/system/i18n/mgt/service_test.go`:
- Around line 620-647: The standalone TestNormaliseBCP47Tag should be moved into
the I18nMgtServiceTestSuite as a suite method (func (s *I18nMgtServiceTestSuite)
TestNormaliseBCP47Tag()) and converted to use testify assertions instead of
t.Errorf; call the existing NormaliseBCP47Tag inside the loop and use s.T().Run
for subtests if desired, and replace manual checks with s.Require().Equal /
s.Require().False / s.Require().True (or s.Assertions.Equal/True/False) to
assert got vs wantTag and valid vs wantValid; remove the original top-level
TestNormaliseBCP47Tag function after adding the suite method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5b44412-6db7-4b7d-95ad-c3c063dddbc3
⛔ Files ignored due to path filters (2)
backend/tests/mocks/i18n/mgtmock/I18nServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/i18n/mgtmock/i18nStoreInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (28)
backend/cmd/server/servicemanager.gobackend/internal/application/init.gobackend/internal/application/init_test.gobackend/internal/application/service.gobackend/internal/application/service_test.gobackend/internal/oauth/init.gobackend/internal/oauth/oauth2/dcr/DCRServiceInterface_mock_test.gobackend/internal/oauth/oauth2/dcr/error_constants.gobackend/internal/oauth/oauth2/dcr/handler.gobackend/internal/oauth/oauth2/dcr/handler_test.gobackend/internal/oauth/oauth2/dcr/init.gobackend/internal/oauth/oauth2/dcr/init_test.gobackend/internal/oauth/oauth2/dcr/model.gobackend/internal/oauth/oauth2/dcr/model_test.gobackend/internal/oauth/oauth2/dcr/service.gobackend/internal/oauth/oauth2/dcr/service_test.gobackend/internal/system/i18n/mgt/I18nServiceInterface_mock_test.gobackend/internal/system/i18n/mgt/constants.gobackend/internal/system/i18n/mgt/file_based_store.gobackend/internal/system/i18n/mgt/file_based_store_test.gobackend/internal/system/i18n/mgt/i18nStoreInterface_mock_test.gobackend/internal/system/i18n/mgt/service.gobackend/internal/system/i18n/mgt/service_test.gobackend/internal/system/i18n/mgt/store.gobackend/internal/system/i18n/mgt/store_constants.gobackend/internal/system/i18n/mgt/store_test.gotests/integration/oauth/dcr/dcr_test.gotests/integration/oauth/dcr/model.go
✅ Files skipped from review due to trivial changes (5)
- backend/internal/application/init_test.go
- backend/cmd/server/servicemanager.go
- backend/internal/system/i18n/mgt/store_constants.go
- backend/internal/oauth/oauth2/dcr/handler_test.go
- backend/internal/oauth/oauth2/dcr/service_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- backend/internal/oauth/oauth2/dcr/init_test.go
- backend/internal/oauth/oauth2/dcr/error_constants.go
- backend/internal/system/i18n/mgt/file_based_store.go
- backend/internal/oauth/oauth2/dcr/init.go
- backend/internal/oauth/oauth2/dcr/model_test.go
- backend/internal/application/service.go
- backend/internal/application/service_test.go
- backend/internal/system/i18n/mgt/store_test.go
- tests/integration/oauth/dcr/dcr_test.go
- backend/internal/system/i18n/mgt/service.go
- backend/internal/system/i18n/mgt/I18nServiceInterface_mock_test.go
nandhu-kumar
force-pushed
the
main-local-2106
branch
from
acf373d to
4501fb3
Compare
nandhu-kumar
force-pushed
the
main-local-2106
branch
from
c6242c7 to
ed7a0e1
Compare
nandhu-kumar
force-pushed
the
main-local-2106
branch
2 times, most recently
from
41fcd8b to
48abb2b
Compare
nandhu-kumar
requested review from
ThaminduDilshan,
ThumulaPerera and
brionmario
nandhu-kumar
force-pushed
the
main-local-2106
branch
from
48abb2b to
316d74a
Compare
nandhu-kumar
force-pushed
the
main-local-2106
branch
from
316d74a to
9ef4a78
Compare
Signed-off-by: Nandhukumar <nandhukumare@gmail.com>
nandhu-kumar
force-pushed
the
main-local-2106
branch
from
9ef4a78 to
206b8b2
Compare
ThaminduDilshan
approved these changes
Apr 30, 2026
ThaminduDilshan
changed the title
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #2106
Purpose
OAuth 2.0 Dynamic Client Registration (RFC 7591) requires clients to register localized metadata
variants using OIDC language-tagged fields (e.g.
"client_name#fr": "Mon Client"). Previously,Thunder's DCR endpoint ignored these fields entirely, meaning clients could not register
language-specific display names, logo URIs, terms-of-service, or policy URIs.
This PR adds full support for OIDC language-tagged DCR metadata fields.
Approach
Parsing language-tagged fields (RFC 7591 §2.1)
DCRRegistrationRequest.UnmarshalJSONnow scans all top-level JSON keys for thefield#lang-tagpattern. Valid BCP 47 tags (normalised via
golang.org/x/text/language) are extracted into fourtyped maps:
LocalizedClientName,LocalizedLogoURI,LocalizedTosURI,LocalizedPolicyURI.Invalid tags are rejected immediately with a 400 error before any storage occurs.
DCRRegistrationResponse.MarshalJSONserializes those maps back as top-level#-keyed keys soclients receive the echoed variants in the 201 response.
Persisting to i18n store (transaction limitation + compensation)
The DCR outer transactioner operates on the runtime DB, while both the application store and i18n
store use configDB. Because of this mismatch, they cannot participate in the same transaction.
Localized variants are written to the i18n store after the application record commits. On write
failure, a compensation path cleans up any partial i18n rows and deletes the newly-created
application, maintaining consistency without distributed transactions.
Each app's translations are stored under the namespace
app.<id>with keysclient_name,logo_uri,tos_uri,policy_uri.i18n integration
Added
SetTranslationOverridesForNamespaceto perform bulk writes of localized variants during DCRregistration, reducing multiple DB calls into a single operation.
A new
GetTranslationsByNamespacemethod onI18nServiceInterfacefetches all locale rows for anamespace in a single query, avoiding the N+1 pattern. This is used to support localized metadata
handling within the DCR flow.
Cleanup on application deletion
The existing
deleteLocalizedVariantscall in the application service lifecycle ensures i18n rowsare cleaned up when an application is deleted.
Related Issues
Related PRs
Checklist
backend/internal/oauth/oauth2/dcr/service_test.go: coveringRegisterClientwith localized variants, i18n write failure compensation, and related scenariostests/integration/oauth/dcr/dcr_test.go: register with localizedfields (assert echoed in response), invalid BCP 47 tag rejection (400)
breaking changelabel added.Security checks