[DIGIT-361] Property Tax Onboarding #4
Conversation
…rvice designed to streamline user onboarding and profile management within the property tax system. Its core features include complete user profile CRUD operations, advanced filtering, zone and ward mapping, and seamless integration with Keycloak for user creation and role assignment. The service exposes robust RESTful APIs, supports efficient pagination, and ensures reliable data persistence in PostgreSQL. By providing a secure, scalable, and maintainable platform for managing user lifecycles, it enables efficient administration and enhances the overall property tax management process.
WalkthroughIntroduces a complete backend onboarding service for property tax management with user lifecycle operations, Keycloak identity management, PostgreSQL persistence via GORM, zone/ward validation via MDMS API, transactional workflows, role-based access control, soft-delete semantics, and comprehensive HTTP APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant UserService
participant KeycloakService
participant DB as Database
participant MDMS
Client->>Handler: POST /api/v1/users (CreateUserRequest)
Handler->>UserService: CreateUser(ctx, request)
UserService->>UserService: Validate input
UserService->>KeycloakService: CreateUser(keycloakUser)
KeycloakService->>KeycloakService: GetAdminToken()
KeycloakService->>MDMS: Keycloak REST API
MDMS-->>KeycloakService: 201 + Location header
KeycloakService-->>UserService: keycloak_user_id
UserService->>KeycloakService: AssignRoleToUser(id, role)
KeycloakService->>MDMS: Keycloak role assignment
MDMS-->>KeycloakService: OK
KeycloakService-->>UserService: Success
UserService->>DB: BeginTransaction()
DB-->>UserService: Transaction handle
UserService->>DB: Create User (in txn)
DB-->>UserService: OK
UserService->>DB: Create UserProfile (in txn)
DB-->>UserService: OK
UserService->>MDMS: ValidateZoneAndWards(zone, wards)
MDMS-->>UserService: Zone details (validated wards)
UserService->>DB: InsertZoneMappings(in txn)
DB-->>UserService: OK
UserService->>DB: Commit()
DB-->>UserService: OK
UserService->>DB: GetUser (enriched)
DB-->>UserService: User + Profile + Address + Zones
UserService-->>Handler: UserResponse
Handler-->>Client: 201 { success: true, data: user }
Note over UserService,DB: On validation failure: Keycloak user deleted, transaction rolled back
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Key areas requiring focused review:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (31)
onboarding-backend/internal/database/database.go-26-26 (1)
26-26: Hard-coded schema prefix should be configurable.The table prefix
"DIGIT3."is hard-coded, making the code inflexible for different deployments or environments. This should be sourced from configuration.Apply this diff:
db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{ NamingStrategy: schema.NamingStrategy{ - TablePrefix: "DIGIT3.", // Prefix all table names with the schema + TablePrefix: cfg.DBSchema + ".", // Prefix all table names with the schema }, })Note: Verify that
cfg.DBSchemacontains just the schema name (e.g., "DIGIT3") without a trailing dot.onboarding-backend/.gitignore-14-17 (1)
14-17: Don’t ignorego.sum(it should be committed)
Ignoringgo.sumundermines reproducible builds and dependency integrity for Go modules.# Go workspace file go.work -go.sumonboarding-backend/migrations/zone_mapping.sql-1-20 (1)
1-20: Foreign key should reference the intended schema explicitly
DIGIT3.zone_mappingreferencesusers(...)unqualified; this is fragile ifsearch_pathdiffers across environments.ALTER TABLE DIGIT3.zone_mapping ADD CONSTRAINT zone_mapping_user_id_fkey FOREIGN KEY (user_id) -REFERENCES users(keycloak_user_id) +REFERENCES DIGIT3.users(keycloak_user_id) ON DELETE CASCADE;onboarding-backend/cmd/server/main.go-75-88 (1)
75-88: CORS is likely broken with default config (*origin + credentials)
WithCORS_ALLOWED_ORIGINS="*"(default) andAllowCredentials: true, browsers will reject credentialed cross-origin requests.allowedOrigins := strings.Split(cfg.CORSAllowedOrigins, ",") allowedMethods := strings.Split(cfg.CORSAllowedMethods, ",") allowedHeaders := strings.Split(cfg.CORSAllowedHeaders, ",") + // If allowing credentials, do not use wildcard origins. + allowCreds := true + if len(allowedOrigins) == 1 && strings.TrimSpace(allowedOrigins[0]) == "*" { + allowCreds = false + } + corsConfig := middleware.CORSConfig{ AllowedOrigins: allowedOrigins, AllowedMethods: allowedMethods, AllowedHeaders: allowedHeaders, - AllowCredentials: true, + AllowCredentials: allowCreds, MaxAge: 86400, // 24 hours }onboarding-backend/migrations/address.sql-2-10 (1)
2-10: Schema + UUID extension assumptions can break deployments
If your migration runner doesn’t setsearch_pathtoDIGIT3, this will createpublic.addresswhile the app (via GORMTablePrefix: "DIGIT3.") will queryDIGIT3.address. Also,gen_random_uuid()will fail unlesspgcrypto(or equivalent) is enabled.-- Create the address table -CREATE TABLE address ( +CREATE EXTENSION IF NOT EXISTS pgcrypto; +CREATE TABLE IF NOT EXISTS DIGIT3.address ( id UUID NOT NULL DEFAULT gen_random_uuid(), -- Unique identifier for the address address_line1 VARCHAR(200) NOT NULL, -- First line of the address address_line2 VARCHAR(200), -- Second line of the address (nullable) city VARCHAR(100) NOT NULL, -- City (not nullable) state VARCHAR(100) NOT NULL, -- State (not nullable) pin_code VARCHAR(10) NOT NULL, -- Postal code (not nullable) PRIMARY KEY (id) -- Primary key on the `id` column );onboarding-backend/internal/utils/mdms_utils.go-54-55 (1)
54-55: Hardcoded tenant and client IDs should be configurable.The tenant ID (
"pb.amritsar") and client ID ("test-client") are hardcoded, making the code inflexible for different environments or tenants. These should be loaded from configuration.Add to
internal/config/config.go:type Config struct { // ... existing fields MDMSTenantID string MDMSClientID string } // In GetConfig(): MDMSTenantID: getEnvOrDefault("MDMS_TENANT_ID", "pb.amritsar"), MDMSClientID: getEnvOrDefault("MDMS_CLIENT_ID", "test-client"),Then update this code:
- req.Header.Set("X-Tenant-ID", "pb.amritsar") - req.Header.Set("X-Client-Id", "test-client") + cfg := config.GetConfig() + req.Header.Set("X-Tenant-ID", cfg.MDMSTenantID) + req.Header.Set("X-Client-Id", cfg.MDMSClientID)onboarding-backend/internal/scheduler/scheduler.go-21-29 (1)
21-29: Missing error handling for cron job registration.
c.AddFuncreturns an error that should be checked. If the cron expression is invalid, the job will silently fail to register.- c.AddFunc("*/2 * * * *", func() { + _, err := c.AddFunc("*/2 * * * *", func() { log.Println("Running daily user activation/deactivation task...") // Call the service layer to handle user activation/deactivation err := keycloakService.ManageUserActivation() if err != nil { log.Printf("Error in user activation/deactivation task: %v\n", err) } }) + if err != nil { + log.Fatalf("Failed to schedule user activation task: %v", err) + }onboarding-backend/pkg/logger/logger.go-34-37 (1)
34-37: Panic risk from unchecked type assertion.
keyvals[i].(string)will panic if the key is not a string. Use a comma-ok assertion or thetoStringhelper.for i := 0; i < len(keyvals); i += 2 { if i+1 < len(keyvals) { - logMessage += " " + keyvals[i].(string) + "=" + toString(keyvals[i+1]) + logMessage += " " + toString(keyvals[i]) + "=" + toString(keyvals[i+1]) } }Apply the same fix to
Error,Warn, andFatalfunctions.onboarding-backend/internal/repositories/zone_mapping_repo.go-28-38 (1)
28-38: Missing nil check onmappingparameter.If
mappingis nil, this will panic when accessingmapping.UserIDon line 29. Add a guard clause.func (r *PostgreSQLZoneMappingRepository) CreateZoneMapping(ctx context.Context, mapping *models.ZoneMapping) error { + if mapping == nil { + return fmt.Errorf("mapping cannot be nil") + } logger.Info("Creating zone mapping", "user_id", mapping.UserID, "zone", mapping.Zone)onboarding-backend/internal/config/config.go-100-103 (1)
100-103: CORS default of*is overly permissive for a production service.Allowing all origins by default (
*) can expose the API to CSRF attacks from any domain. Consider requiring explicit configuration or defaulting to a more restrictive value.// CORS config - CORSAllowedOrigins: getEnvOrDefault("CORS_ALLOWED_ORIGINS", "*"), + CORSAllowedOrigins: getEnvOrDefault("CORS_ALLOWED_ORIGINS", ""),Then add validation in
validateConfigto ensureCORSAllowedOriginsis explicitly set.Committable suggestion skipped: line range outside the PR's diff.
onboarding-backend/pkg/logger/logger.go-28-40 (1)
28-40: Race condition on lazy initialization.If multiple goroutines call
Info()(or other log functions) concurrently when loggers are nil,InitLogger()may be called multiple times, causing a data race on the logger variables. Usesync.Oncefor thread-safe initialization.+var initOnce sync.Once + func Info(message string, keyvals ...interface{}) { - if infoLogger == nil { - InitLogger() - } + initOnce.Do(InitLogger) // ... }Apply the same pattern to
Error,Warn, andFatal.onboarding-backend/internal/models/user.go-10-25 (1)
10-25: AlignZoneDataJSON field names (zonedatavszoneData) to avoid client-side breakage
Right now requests and responses use different JSON names for the same concept.type CreateUserRequest struct { @@ - ZoneData []ZoneData `json:"zonedata"` + ZoneData []ZoneData `json:"zoneData,omitempty"` @@ }Also applies to: 27-30, 32-48
onboarding-backend/internal/validator/user_validation_service.go-71-86 (1)
71-86: Aadhaar validation is unreliable with*int64(leading zeros lost) + error string bypasses constants
If Aadhaar can begin with0, an integer field cannot represent it faithfully. Consider using string in API + DB, or enforce a numeric range explicitly and standardize error constants.onboarding-backend/internal/models/user.go-223-239 (1)
223-239: Avoid accepting client-controlledCreatedBy/UpdatedBy/Deletedon create
These are typically derived from auth context and server-side behavior; taking them from request enables spoofing audit metadata (and letting a client create a “deleted” user). Consider removing them fromCreateUserRequestor ignoring them in service-layer mapping.onboarding-backend/internal/models/user.go-174-180 (1)
174-180:UsersListResponseis missing JSON tags for pagination fields
TotalCount,Limit, andOffsetwon’t serialize as expected (andTotalCountcurrently has no json tag at all).type UsersListResponse struct { Users []*UserResponse `json:"users"` - TotalCount int - Limit int - Offset int + TotalCount int `json:"totalCount"` + Limit int `json:"limit"` + Offset int `json:"offset"` }onboarding-backend/internal/models/user.go-182-221 (1)
182-221:CreateUserResponsebuilds an emptyaddress: {}and doesn’t map address fields
This produces misleading payloads and can break clients that treat presence as “address exists”. Map fromuserProfile.Addresswhen present; otherwise keepAddressnil.func (ur *UserResponse) CreateUserResponse(user *User, userProfile *UserProfile) { @@ if userProfile != nil { + var addr *Address + if userProfile.Address != nil { + addr = &Address{ + AddressLine1: userProfile.Address.AddressLine1, + AddressLine2: userProfile.Address.AddressLine2, + City: userProfile.Address.City, + State: userProfile.Address.State, + PinCode: userProfile.Address.PinCode, + } + } ur.Profile = UserProfileResponse{ @@ - Address: &Address{}, + Address: addr, } } }onboarding-backend/internal/repositories/users_repo.go-139-170 (1)
139-170: Docstring contradicts soft-delete implementation + inconsistent context usageThe docstring (lines 125-138) promises a transactional delete across user_profile, address, and user tables, but the implementation only soft-deletes by setting
deleted=true. Either implement the documented transaction-based hard delete or update the docstring to reflect soft-delete behavior.Additionally, lines 144 and 161-163 do not use
WithContext(ctx)while the parameter is accepted and all other repository methods consistently use it. AddWithContext(ctx)to maintain consistency.onboarding-backend/internal/handlers/user_handler.go-220-247 (1)
220-247: Pass request context toUpdateUserCompleteand use typed error checks instead of string comparison
UpdateUserCompleteshould acceptcontext.Contextas a parameter (matching the pattern used byCreateUser,GetUser, etc.) and receivec.Request.Context()from the handler—currently it createscontext.Background(), which prevents proper request timeout/cancellation propagation. Additionally, replaceerr.Error() == "user not found"with a typed error check usingerrors.As()or a custom error type, as the service layer returns formatted error messages that change based on context.onboarding-backend/internal/repositories/users_repo.go-272-322 (1)
272-322:UpdateUserProfileCompleterequires wrapping profile and address operations in a database transactionThe function performs interdependent database operations across lines 276–318: updating user profile, creating or updating an address, and linking them via AddressID. Without a transaction, a failure at any point leaves data inconsistent—for example, profile updated but address not created/linked, or address created but AddressID not updated. Wrap all operations starting from the profile update through the final address linkage in
db.BeginTx(...)or use GORM'sdb.Transaction(...)to ensure atomicity.onboarding-backend/internal/validator/user_validation_service.go-38-119 (1)
38-119: Start/end date validation gaps (StartDate-only and EndDate-only cases aren't validated)Currently the validation block only executes when both
StartDateandEndDateare non-nil, allowing invalid states:StartDatecan be in the past ifEndDateis omitted, andEndDateordering isn't checked ifStartDateis omitted.func (v *UserValidationService) ValidateCreateUserRequest(req *models.CreateUserRequest) error { @@ - if req.StartDate != nil && req.EndDate != nil { - if req.EndDate.Before(*req.StartDate) { - return fmt.Errorf(constants.ErrInvalidDateRange) - } - if req.StartDate.Before(time.Now()) { - return fmt.Errorf(constants.ErrStartDateInPast) - } - } + if req.StartDate != nil { + if req.StartDate.Before(time.Now()) { + return fmt.Errorf(constants.ErrStartDateInPast) + } + } + if req.StartDate != nil && req.EndDate != nil && req.EndDate.Before(*req.StartDate) { + return fmt.Errorf(constants.ErrInvalidDateRange) + } return nil }onboarding-backend/internal/handlers/user_handler.go-109-125 (1)
109-125: Map not-found to 404 inGetUserand other handlers (distinguish client errors from server errors)The repository layer properly detects
gorm.ErrRecordNotFoundseparately from other DB errors, but the service layer wraps all errors as genericServiceErrorwith only a message string. This prevents the handler from distinguishing a missing user (404) from actual server failures (500).Define typed error types (e.g.,
NotFoundError,ServiceError) and have the service check the repository error type before wrapping, then use type assertions in handlers to map to appropriate HTTP status codes.onboarding-backend/internal/models/database_models.go-22-40 (1)
22-40: AddTableName()methods toUserandUserProfilemodels for consistencyThe models
UserandUserProfiledo not defineTableName()methods, whileAddressDBandZoneMappingexplicitly define them (returning"DIGIT3.address"and"DIGIT3.zone_mapping"). Although the database configuration appliesTablePrefix: "DIGIT3."automatically, this inconsistency reduces clarity and maintainability. Add explicitTableName()methods to all models:func (User) TableName() string { return "DIGIT3.users" } func (UserProfile) TableName() string { return "DIGIT3.user_profiles" }onboarding-backend/internal/models/database_models.go-24-40 (1)
24-40: Use struct field names in GORM relation tags, not column namesLine 38:
gorm:"foreignKey:user_profile_id;references:keycloak_user_id"uses column names instead of struct field names. GORM requires struct field names in these tags. The correct syntax isgorm:"foreignKey:KeycloakUserID;references:ID"to match User.KeycloakUserID with UserProfile.ID.onboarding-backend/internal/validator/user_validation_service.go-121-151 (1)
121-151: Do not log full Aadhaar / phone numbers (PII) in validator logsThese logs will leak sensitive identifiers to log sinks, violating UIDAI regulations (Aadhaar numbers must not be stored or logged in plaintext) and GDPR requirements for personal data protection. Mask or omit these values:
- CheckAdhaarExists: remove or mask the
adhaarNoparameter from all logger calls (lines 124, 127, 130)- CheckPhoneExists: remove or mask the
phoneNumberparameter from all logger calls (lines 140, 143, 146)Example: log only a masked form such as last 4 digits (
****1234for Aadhaar,+91-XXXXX1234for phone), or integrate a sanitizer middleware in the logger to redact these patterns before emission.onboarding-backend/internal/services/user_service.go-334-349 (1)
334-349: populateAddress: guardresponse.Profilebefore writingresponse.Profile.Address.
IfCreateUserResponse(...)can leaveProfilenil whenuserProfileis nil, this will panic.onboarding-backend/internal/services/keycloak_service.go-26-75 (1)
26-75: Config mismatch: KeycloakService has ClientID/ClientSecret but GetAdminToken hardcodesadmin-cli.
Either remove unused fields or useks.ClientID(+client_secretwhen configured). As-is, deployments expecting a non-admin-cliclient will fail in non-obvious ways.onboarding-backend/internal/services/keycloak_service.go-146-184 (1)
146-184: ManageUserActivation: timezone + ctx + partial-failure behavior need tightening.
time.Now().Format("2006-01-02")is local-time dependent; DB “date” semantics often want UTC or explicit tenant timezone.context.Background()prevents cancel/timeout from a scheduler.- First failing user aborts the whole batch; you may want to continue and report aggregated failures.
onboarding-backend/internal/services/user_service.go-490-529 (1)
490-529: UpdateUserComplete: validate updateReq != nil and use caller ctx (not context.Background).
Right now a nilupdateReqwill panic, and you drop request-scoped cancellation/deadlines.onboarding-backend/internal/services/keycloak_service.go-589-627 (1)
589-627: EnableUser/DisableUser: propagate ctx + don’t ignore marshal errors + make error style consistent.
Both methods hardcodecontext.Background()for DB writes, and DisableUser dropsjson.Marshalerrors (body, _ := ...). Also error returns likefmt.Errorf(constants.ErrGetAdminToken)lose root cause.Also applies to: 640-676
onboarding-backend/internal/services/user_service.go-549-612 (1)
549-612: GetAllUsersWithFilters: “no users” as error + totalCount mismatch after skipping disabled users.
Returning an error for empty results breaks normal pagination semantics; alsototalCountcomes from DB but you latercontinuefor disabled Keycloak users, so the response metadata may be misleading.onboarding-backend/internal/services/keycloak_service.go-86-135 (1)
86-135: Add context timeout and prevent retry on auth/config failures.The current implementation retries indefinitely (up to 2 minutes) on any error, including permanent auth failures (400, 401, 403), and requests cannot be canceled. Wrap the operation with
context.WithTimeout(), usebackoff.Permanent()for non-retryable HTTP status codes, and return raw errors from the retry operation instead of wrapping them.func (ks *KeycloakService) GetAdminToken() (string, error) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + tokenURL := ks.TokenURL data := url.Values{} data.Set("grant_type", "password") - data.Set("client_id", "admin-cli") + data.Set("client_id", ks.ClientID) + if ks.ClientSecret != "" { + data.Set("client_secret", ks.ClientSecret) + } data.Set("username", ks.AdminUser) data.Set("password", ks.AdminPass) var tokenResp KeycloakTokenResponse operation := func() error { - req, err := http.NewRequest("POST", tokenURL, strings.NewReader(data.Encode())) + req, err := http.NewRequestWithContext(ctx, "POST", tokenURL, strings.NewReader(data.Encode())) if err != nil { - errMsg := errors.NewKeycloakServiceError(fmt.Sprintf("%s (url: %s)", constants.ErrCreateToken, tokenURL)) - logger.Error(constants.ErrCreateToken, errMsg) - return errMsg + return err } req.Header.Set("Content-Type", "application/x-www-form-urlencoded") resp, err := ks.httpClient.Do(req) if err != nil { - errMsg := errors.NewKeycloakServiceError(fmt.Sprintf("%s (url: %s)", constants.ErrRequestToken, tokenURL)) - logger.Error(constants.ErrRequestToken, errMsg) - return errMsg + return err } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { body, _ := io.ReadAll(resp.Body) - errMsg := errors.NewKeycloakServiceError(fmt.Sprintf("%s (status: %d, url: %s): %s", constants.ErrGetAdminToken, resp.StatusCode, tokenURL, string(body))) - logger.Error(constants.ErrGetAdminToken, errMsg) - return errMsg + if resp.StatusCode == http.StatusBadRequest || resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { + return backoff.Permanent(fmt.Errorf("%s (status: %d): %s", constants.ErrGetAdminToken, resp.StatusCode, string(body))) + } + return fmt.Errorf("%s (status: %d): %s", constants.ErrGetAdminToken, resp.StatusCode, string(body)) } if err := json.NewDecoder(resp.Body).Decode(&tokenResp); err != nil { - errMsg := errors.NewKeycloakServiceError(fmt.Sprintf("%s (url: %s)", constants.ErrDecodeToken, tokenURL)) - logger.Error(constants.ErrDecodeToken, errMsg) - return errMsg + return err } return nil }Committable suggestion skipped: line range outside the PR's diff.
🟡 Minor comments (11)
onboarding-backend/internal/database/database.go-33-33 (1)
33-33: Misleading log message: No migration is performed.The log states "Database connected and migrated successfully", but no migration code is present in this function. The message should reflect only the connection success.
Apply this diff:
- logger.Info("Database connected and migrated successfully") + logger.Info("Database connected successfully")onboarding-backend/Dockerfile-33-33 (1)
33-33: Update Dockerfile to expose port 8089 to match the application server and Postman configuration.The Dockerfile exposes port 8080, but the application server defaults to port 8089 (
SERVER_PORTenvironment variable inonboarding-backend/internal/config/config.go), and all Postman collection endpoints use port 8089. This mismatch will cause connection failures when using Docker with default settings.onboarding-backend/internal/constants/logs_constants.go-55-55 (1)
55-55: Fix typo in constant name.The constant name
LogUpdateUserStartthas a double 't' at the end. It is defined at line 55 and used in one location:onboarding-backend/internal/repositories/users_repo.go:240.Apply these changes:
In
onboarding-backend/internal/constants/logs_constants.goline 55:- LogUpdateUserStartt = "Starting update of user" + LogUpdateUserStart = "Starting update of user"In
onboarding-backend/internal/repositories/users_repo.goline 240:- logger.Info(constants.LogUpdateUserStartt, "keycloak_user_id", keycloakUserID) + logger.Info(constants.LogUpdateUserStart, "keycloak_user_id", keycloakUserID)onboarding-backend/README.md-32-32 (1)
32-32: Version mismatch: README states Go 1.21+ but go.mod requires 1.24.0.The README specifies
Go 1.21+as a requirement, butgo.moddeclaresgo 1.24.0. Ensure the documentation matches the actual requirement.-- **Language**: Go 1.21+ +- **Language**: Go 1.24+Committable suggestion skipped: line range outside the PR's diff.
onboarding-backend/go.mod-3-3 (1)
3-3: Update Go version from 1.24.0 to the latest stable release.Go 1.24.0 is outdated. As of December 2025, the latest stable version is Go 1.25.5 (released December 2, 2025). Update to 1.25.5 or maintain a more recent stable version.
onboarding-backend/internal/repositories/interfaces.go-13-14 (1)
13-14:Count()is missing thecontext.Contextparameter.All other methods accept a
context.Contextfor cancellation and deadline propagation, butCount()does not. This inconsistency could cause issues with request timeouts and tracing.- Count() (int64, error) - GetUserCounts(ctx context.Context) (int64, int64, int64, error) + Count(ctx context.Context) (int64, error) + GetUserCounts(ctx context.Context) (int64, int64, int64, error)onboarding-backend/internal/config/config.go-122-133 (1)
122-133: Docstring contradicts implementation.The comment on lines 123-124 states the function "returns an error...instead of terminating the process directly", but the implementation calls
log.Fatalfwhich terminates the process immediately. Either update the docstring or refactor to return an error.If you want to keep the current behavior, fix the docstring:
// validateConfig verifies that required configuration fields are present. -// It returns an error describing missing or invalid settings instead of -// terminating the process directly (caller decides how to handle it). +// It terminates the process with a fatal log if critical settings are missing. func validateConfig(cfg *Config) {onboarding-backend/internal/config/config.go-66-69 (1)
66-69: Incomplete log message.The warning message at line 68 is incomplete - it says "not found at" but doesn't include the path value.
err := godotenv.Load() if err != nil { - log.Println("Warning: .env file not found at", "using system environment variables") + log.Println("Warning: .env file not found, using system environment variables") }onboarding-backend/internal/validator/user_validation_service.go-100-107 (1)
100-107: Phone validation checks length only; ensure digits-only to prevent garbage values
Alen == 10check allows non-numeric phone strings.onboarding-backend/internal/handlers/user_handler.go-266-345 (1)
266-345: Fix typo and resolve query parameter naming inconsistency + mask PII in logsThree issues found:
- Line 273:
roleLisdtshould beroleList- Line 289: Query parameter name is
is_active(snake_case) but error message (line 293) and logging (line 306) referenceisActive(camelCase) — align naming consistently- Line 306: Email and phone number are logged unmasked; mask these PII fields in the logger output
onboarding-backend/internal/services/keycloak_service.go-198-267 (1)
198-267: Location parsing can return empty ID on trailing slash; prefer URL/path parsing.
strings.Split(location, "/")+ last element breaks for.../users/{id}/. Usepath.Base(strings.TrimRight(location, "/"))and validate non-empty.
🧹 Nitpick comments (38)
onboarding-backend/postman/new_onboarding.postman_collection.json (2)
3-7: Remove or genericize placeholder metadata.The collection contains placeholder IDs and links (
postman-id,exporter-id,your-collection-link) that should be removed or replaced with generic values before committing to the repository.Apply this diff:
- "_postman_id": "postman-id", + "_postman_id": "00000000-0000-0000-0000-000000000000", "name": "new_onboarding", "schema": "https://schema.getpostman.com/json/collection/v2.1.0/collection.json", - "_exporter_id": "exporter-id", - "_collection_link": "https://www.postman.com/collections/your-collection-link" + "_exporter_id": "00000000"
14-19: Add authentication headers for secured endpoints.All endpoints have empty authentication headers. For a production-ready API, consider adding placeholder authentication headers (e.g., Bearer token) to guide developers on proper API usage.
Example:
"header": [ { "key": "Authorization", "value": "Bearer <access_token>", "type": "default" } ]onboarding-backend/internal/errors/errors.go (2)
5-8: Consider adding error wrapping and context fields.The
ServiceErrortype only stores a message string, which limits the ability to:
- Wrap underlying errors for root cause analysis
- Add structured context (error codes, request IDs, timestamps)
- Use Go's standard
errors.Is()anderrors.As()for error classificationConsider enhancing the error type:
type ServiceError struct { Message string Code string // e.g., "USER_NOT_FOUND", "KEYCLOAK_ERROR" Err error // wrapped underlying error } func (e *ServiceError) Error() string { if e.Err != nil { return fmt.Sprintf("%s: %v", e.Message, e.Err) } return e.Message } func (e *ServiceError) Unwrap() error { return e.Err }
22-40: Evaluate necessity of specialized error constructors.The four specialized constructors (
NewUserServiceError,NewKeycloakServiceError,NewRepositoryError,NewUtilityError) only add a service prefix to the message. This provides minimal value and could be simplified.Consider either:
- Using the generic
NewServiceErrordirectly with the prefix in the message at call sites- Adding more meaningful differentiation (error codes, types) if programmatic error handling is needed
- Using standard
fmt.Errorf("UserService: %w", err)for error wrappingThe current approach adds API surface without significant benefit.
onboarding-backend/internal/constants/errors_constants.go (1)
1-174: Consider adding structured error codes for programmatic handling.The constants file provides comprehensive error messages but lacks structured error codes. Adding codes would enable:
- Programmatic error classification on the client side
- Internationalization of error messages
- Better API error documentation
- Consistent error handling patterns
Example approach:
const ( // Error codes ErrCodeUserNotFound = "USER_NOT_FOUND" ErrCodeInvalidInput = "INVALID_INPUT" ErrCodeKeycloakError = "KEYCLOAK_ERROR" // ... etc ) // Then pair codes with messages var ErrorMessages = map[string]string{ ErrCodeUserNotFound: "User not found", // ... etc }onboarding-backend/.dockerignore (1)
1-7: Consider adding common development artifacts to .dockerignore.The file covers basic exclusions but could be enhanced with additional common development and build artifacts.
Consider adding:
*.json *.md /migrations .env +.env.* *.yaml *.yml .gitignore +*.log +.vscode/ +.idea/ +*.swp +*.swo +*~ +.DS_Store +Thumbs.dbonboarding-backend/internal/database/database.go (1)
29-31: Consider returning errors instead of fatal exit for better testability.Using
logger.Fatal()causes an immediate process exit, which makes the function difficult to test and prevents graceful error handling. Consider returning the error to the caller.-func Connect(cfg *config.Config) *gorm.DB { +func Connect(cfg *config.Config) (*gorm.DB, error) { dsn := fmt.Sprintf( "host=%s user=%s password=%s dbname=%s port=%s sslmode=disable client_encoding=UTF8 search_path=%s", cfg.DBHost, cfg.DBUser, cfg.DBPassword, cfg.DBName, cfg.DBPort, cfg.DBSchema, ) db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{ NamingStrategy: schema.NamingStrategy{ TablePrefix: "DIGIT3.", }, }) if err != nil { - logger.Fatal("Failed to connect to database:", err) + return nil, fmt.Errorf("failed to connect to database: %w", err) } logger.Info("Database connected successfully") - return db + return db, nil }Then handle the fatal error in
main.gowhere appropriate.onboarding-backend/Dockerfile (1)
18-36: Consider running the application as a non-root user for security.The final image runs the application as root, which increases the attack surface if the container is compromised.
Add a non-root user in the final stage:
FROM alpine:3.19 WORKDIR /app # Install ca-certificates for HTTPS requests RUN apk --no-cache add ca-certificates +# Create a non-root user +RUN addgroup -g 1000 appgroup && \ + adduser -D -u 1000 -G appgroup appuser + # Copy the binary from the builder stage COPY --from=builder /app/property-service . # Set permissions -RUN chmod +x /app/property-service +RUN chown appuser:appgroup /app/property-service && \ + chmod +x /app/property-service + +# Switch to non-root user +USER appuser # Expose port 8080 (default property service port) EXPOSE 8080 # Set the entry point ENTRYPOINT ["./property-service"]onboarding-backend/internal/utils/keycloak_utils.go (1)
13-22: LGTM! Well-designed defensive utility function.The function correctly handles all edge cases:
- Missing key in the attributes map
- Non-slice attribute values
- Empty slices
- Non-string values in the slice
The defensive type assertions and early returns make the code safe and easy to reason about.
Optional: Consider adding debug logging when extraction fails to aid troubleshooting:
func ExtractStringAttribute(attributes map[string]interface{}, key string) string { if attr, exists := attributes[key]; exists { if attrSlice, ok := attr.([]interface{}); ok && len(attrSlice) > 0 { if str, ok := attrSlice[0].(string); ok { return str } logger.Debug("Attribute value is not a string", "key", key, "type", fmt.Sprintf("%T", attrSlice[0])) } else { logger.Debug("Attribute is not a non-empty slice", "key", key) } } return "" }onboarding-backend/cmd/server/main.go (1)
5-20: Prefer one logger; avoidlog.Fatalfso defers run
Right now startup usespkg/loggerand stdliblogmixed, andlog.Fatalfwillos.Exit(1)(skippingdefer sqlDB.Close()).- "log" + // "log" ... - log.Printf("Starting property tax onboarding service on %s", serverAddr) + logger.Info("Starting property tax onboarding service", "addr", serverAddr) if err := router.Run(serverAddr); err != nil { - log.Fatalf("Failed to start server: %v", err) + logger.Fatal("Failed to start server", "error", err) }Also applies to: 99-105
onboarding-backend/internal/routes/routes.go (2)
18-18: Health check delegated to UserHandler is unconventional.Health checks typically verify service dependencies (database, external services) and should not be tied to a domain-specific handler. Consider creating a dedicated health handler or moving this logic to a standalone function.
16-35: Consider adding middleware to routes.The route setup does not apply any middleware (e.g., CORS, authentication, rate limiting, logging). While a CORS middleware exists at
internal/middleware/cors.go, it's not wired here. Consider applying middleware during route setup for consistent cross-cutting concerns.Example integration with CORS middleware:
func SetupRoutes(router *gin.Engine, userHandler *handlers.UserHandler, corsConfig middleware.CORSConfig) { router.Use(middleware.CORSWithConfig(corsConfig)) router.GET("/health", userHandler.HealthCheck) // ... rest of routes }onboarding-backend/go.mod (1)
26-26: Dependencies marked as indirect but likely used directly.Several dependencies are marked
// indirectbut appear to be used directly in the codebase:
- Line 26:
github.com/google/uuid(used for UUID generation)- Line 33:
github.com/joho/godotenv(used in config loading)- Line 37:
github.com/lib/pq(PostgreSQL driver)- Lines 52-53:
gorm.io/driver/postgresandgorm.io/gorm(ORM and driver)Run
go mod tidyto correctly classify direct vs. indirect dependencies:#!/bin/bash # Verify which dependencies are actually imported directly rg -n "github.com/google/uuid|github.com/joho/godotenv|github.com/lib/pq|gorm.io" --type=go -g '!go.mod' -g '!go.sum'Also applies to: 33-33, 37-37, 52-53
onboarding-backend/internal/scheduler/scheduler.go (2)
17-17: Unused parameter: userRepo is never used.The
userRepoparameter is declared but never used within the function. Consider removing it or documenting why it's present.-func ScheduleUserActivation(keycloakService *services.KeycloakService, userRepo repositories.UserRepository) { +func ScheduleUserActivation(keycloakService *services.KeycloakService) {
17-33: Missing graceful shutdown mechanism for the scheduler.The cron scheduler is started but never stopped. Consider returning the cron instance or providing a shutdown mechanism for graceful cleanup.
-func ScheduleUserActivation(keycloakService *services.KeycloakService, userRepo repositories.UserRepository) { +func ScheduleUserActivation(keycloakService *services.KeycloakService, userRepo repositories.UserRepository) *cron.Cron { c := cron.New() // Schedule the task... c.AddFunc("*/2 * * * *", func() { // ... }) // Start the cron scheduler c.Start() + return c }Then in
main.go, capture and stop the scheduler on shutdown:cronScheduler := scheduler.ScheduleUserActivation(keycloakService, userRepo) defer cronScheduler.Stop()onboarding-backend/migrations/user_profiles.sql (1)
8-8: Consider VARCHAR for adhaar_no to preserve leading zeros.Using
BIGINTfor Aadhaar numbers may cause issues:
- Aadhaar numbers can have leading zeros, which
BIGINTwill truncate- Aadhaar is a 12-digit identifier, not a numeric value for arithmetic
Consider using
VARCHAR(12)instead:- adhaar_no BIGINT , -- Aadhaar number (unique constraint) + adhaar_no VARCHAR(12), -- Aadhaar number (unique constraint)onboarding-backend/internal/middleware/cors.go (2)
23-42: CORS middleware allows disallowed origins to proceed.When an origin is not in the allowed list, no
Access-Control-Allow-Originheader is set, but the request continues processing. This means disallowed origins can still access the API; only the browser's CORS check fails. Consider explicitly rejecting disallowed origins or documenting this behavior.For stricter CORS enforcement, abort disallowed origins:
if allowedOrigin == "*" || allowedOrigin == origin { allowed = true break } } + // Reject requests from disallowed origins + if !allowed && origin != "" { + c.AbortWithStatus(403) + return + } + if allowed {Note: This changes behavior - currently, requests without CORS headers can proceed.
45-54: Use strings.Join for cleaner header concatenation.The manual string concatenation for methods and headers is verbose and less efficient. Use
strings.Joinfor cleaner, more idiomatic code.+import "strings" // Set allowed HTTP methods for CORS - methods := "GET, POST, PUT, DELETE, PATCH, OPTIONS" if len(config.AllowedMethods) > 0 { - methods = "" - for i, method := range config.AllowedMethods { - if i > 0 { - methods += ", " - } - methods += method - } + methods := strings.Join(config.AllowedMethods, ", ") + c.Header("Access-Control-Allow-Methods", methods) + } else { + c.Header("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, PATCH, OPTIONS") } - c.Header("Access-Control-Allow-Methods", methods) // Set allowed HTTP headers for CORS - headers := "Origin, Content-Type, Accept, Authorization, X-Requested-With, X-Tenant-ID, X-User-ID" if len(config.AllowedHeaders) > 0 { - headers = "" - for i, header := range config.AllowedHeaders { - if i > 0 { - headers += ", " - } - headers += header - } + headers := strings.Join(config.AllowedHeaders, ", ") + c.Header("Access-Control-Allow-Headers", headers) + } else { + c.Header("Access-Control-Allow-Headers", "Origin, Content-Type, Accept, Authorization, X-Requested-With, X-Tenant-ID, X-User-ID") } - c.Header("Access-Control-Allow-Headers", headers)Also applies to: 58-67
onboarding-backend/pkg/response/response.go (1)
31-38: Consider variadic error parameter for flexibility.The
Errorfunction accepts a singleerrorDetailstring but wraps it in a slice. Consider using a variadic parameter to support multiple errors in one call.-func Error(c *gin.Context, statusCode int, message string, errorDetail string) { +func Error(c *gin.Context, statusCode int, message string, errorDetails ...string) { response := APIResponse{ Success: false, Message: message, - Errors: []string{errorDetail}, + Errors: errorDetails, } c.JSON(statusCode, response) }This allows both single and multiple errors:
response.Error(c, 400, "Validation failed", "Invalid email") response.Error(c, 400, "Validation failed", "Invalid email", "Invalid phone")onboarding-backend/internal/utils/mdms_utils.go (2)
57-57: HTTP client created on every call is inefficient.Creating a new
http.Clientfor each request is inefficient. HTTP clients should be reused to benefit from connection pooling and keep-alive.Create a package-level HTTP client with proper configuration:
+var ( + mdmsHTTPClient = &http.Client{ + Timeout: 30 * time.Second, + Transport: &http.Transport{ + MaxIdleConns: 100, + MaxIdleConnsPerHost: 10, + IdleConnTimeout: 90 * time.Second, + }, + } +) + func GetZoneDetailsFromMDMS(zone string) (*MDMSResponse, error) { // ... - client := &http.Client{} - resp, err := client.Do(req) + resp, err := mdmsHTTPClient.Do(req)
43-43: Validate zone parameter to prevent potential issues.The
zoneparameter is directly appended to the URL without validation or sanitization. Whilefmt.Sprintfhandles this safely for path parameters, consider validating the zone format to catch errors early.func GetZoneDetailsFromMDMS(zone string) (*MDMSResponse, error) { // Validate zone parameter if zone == "" { return nil, fmt.Errorf("zone parameter cannot be empty") } // Continue with existing logic... }onboarding-backend/README.md (2)
312-339: Use spaces instead of hard tabs for consistency.Lines 312-339 use hard tabs, which can render inconsistently across editors and violate common Markdown style guidelines. Consider converting to spaces (typically 2 or 4 spaces per indent level).
Based on learnings, markdownlint issues can be addressed with automated formatters:
#!/bin/bash # Fix markdown formatting issues npx markdownlint-cli2-fix "onboarding-backend/README.md"
60-60: Add language specifiers to fenced code blocks for syntax highlighting.Several fenced code blocks are missing language specifiers, which prevents proper syntax highlighting. Add appropriate language tags (e.g.,
text,plaintext, or specific languages).Examples:
- Line 60: Add
```textfor ASCII diagrams- Line 145: Add
```textfor base URL- Line 230: Add
```textor```plaintextfor directory treeAlso applies to: 145-145, 230-230
onboarding-backend/internal/repositories/zone_mapping_repo.go (1)
11-15: Consider expanding the interface for completeness.The interface lacks
DeleteZoneMappingandUpdateZoneMappingmethods, which are typically needed for full CRUD operations on zone mappings. If these operations are planned, consider adding them now to maintain consistency with the PR's stated goal of "complete CRUD for user profiles."onboarding-backend/internal/repositories/interfaces.go (2)
14-14:GetUserCountsreturn values are ambiguous.Returning three unnamed
int64values makes it unclear what each count represents. Consider using a struct or adding documentation to clarify (e.g., total, active, inactive counts).// Option 1: Document the return values // GetUserCounts returns (totalCount, activeCount, inactiveCount, error) GetUserCounts(ctx context.Context) (int64, int64, int64, error) // Option 2: Use a struct for clarity type UserCounts struct { Total int64 Active int64 Inactive int64 } GetUserCounts(ctx context.Context) (*UserCounts, error)
17-17: Minor formatting: missing space after comma.Lines 17 and 27 have inconsistent spacing in method signatures.
- GetUserProfileByUserID(ctx context.Context,userID string) (*models.UserProfile, error) + GetUserProfileByUserID(ctx context.Context, userID string) (*models.UserProfile, error)- GetAllUsersWithFilters(ctx context.Context,filters models.UserFilters, limit, offset int) ([]*models.User, int64, error) + GetAllUsersWithFilters(ctx context.Context, filters models.UserFilters, limit, offset int) ([]*models.User, int64, error)Also applies to: 27-27
onboarding-backend/pkg/logger/logger.go (1)
51-56: Modifying thekeyvalsslice can cause unexpected behavior.Reslicing
keyvals(line 54) modifies the caller's view of the slice if they retain a reference. While unlikely to cause issues in practice, it's cleaner to use an index offset instead.+ startIdx := 0 if len(keyvals) > 0 { if err, ok := keyvals[0].(error); ok { logMessage += " " + err.Error() - keyvals = keyvals[1:] // Remove the error from keyvals + startIdx = 1 } } - for i := 0; i < len(keyvals); i += 2 { + for i := startIdx; i < len(keyvals); i += 2 {onboarding-backend/internal/repositories/transaction.go (2)
92-102: Rollback may log spurious errors after a successful commit.GORM returns
gorm.ErrInvalidTransactionwhen rolling back an already-committed transaction. Logging this as an error is misleading. Consider ignoring this specific error or not logging it as an error.func (t *PostgreSQLTransaction) Rollback() error { logger.Info(constants.LogRollbackTransaction) if err := t.tx.Rollback().Error; err != nil { + // Ignore error if transaction was already committed + if errors.Is(err, gorm.ErrInvalidTransaction) { + logger.Info("Transaction already committed or rolled back, skipping rollback") + return nil + } logger.Error(constants.ErrRollbackTransaction, "error", err) return fmt.Errorf("%s: %w", constants.ErrRollbackTransaction, err) }You'll need to add
"errors"to imports.
18-23: Transaction interface is tightly coupled to User/UserProfile.The
Transactioninterface only supportsCreate(user)andCreateUserProfile(profile). For a more generic and reusable design, consider exposing the underlying*gorm.DBor adding a genericCreate(model interface{}) errormethod that can handle any model.type Transaction interface { Create(model interface{}) error Commit() error Rollback() error // Or expose DB() *gorm.DB for flexibility }onboarding-backend/internal/models/user.go (1)
128-162: DTOs are coupled to persistence models (UpdateUserProfileembeds*AddressDB+ hasgormtags)
This makes the HTTP contract brittle and risks accidental DB-field exposure/updates when refactoring. Prefer a pure API type (e.g.,*Address) and keepgormtags in DB models only.onboarding-backend/internal/validator/user_validation_service.go (1)
44-47: Moveregexp.MustCompileto package scope to avoid recompilation per request
Minor perf/readability win.onboarding-backend/internal/models/database_models.go (1)
83-91:ZoneMappinghas no primary key; updates/deletes may behave unexpectedly
If you need to update mappings, add anID(uuid) or composite primary key (user_id + zone).onboarding-backend/internal/repositories/users_repo.go (1)
387-409: Repository error handling/logging is inconsistent across methods
Some methods wrap/log witherrors.NewRepositoryError+ constants, others return rawerr/fmt.Errorfwithout logging. Consider standardizing for easier ops/debugging.Also applies to: 411-472, 474-496
onboarding-backend/internal/services/keycloak_service.go (1)
280-343: Avoid double token fetch and URL-escape role/user identifiers.
AssignRoleToUser calls GetAdminToken, then GetRole calls GetAdminToken again. AlsoroleNameshould beurl.PathEscape(roleName)when building role URLs.Also applies to: 356-398
onboarding-backend/internal/services/user_service.go (4)
212-234: buildKeycloakUser: consider not forcing EmailVerified=true and handle empty passwords explicitly.
If email verification is a real requirement, this should likely be false until verified; otherwise you’re bypassing the concept. Also considerTemporary: truefor first-login flows.
369-433: GetUsersByRole: watch for nil Profile + N+1 Keycloak calls.
- Ensure
userResponse.Profileis non-nil before settinguserResponse.Profile.Address.- Per-user Keycloak calls can become a hot path; consider relying on DB
IsActive(or caching) if feasible.
448-472: DeleteUserByKeycloakID: consider ctx threading + failure ordering/compensation.
If DB delete fails after Keycloak delete, you can end up with a DB record referencing a non-existent Keycloak user (or vice versa if you swap the order). Consider a transactional/compensating approach and acceptctxinstead ofcontext.Background().
629-641: Zone mappings/MDMS validation: transactional integrity + faster ward lookup.
- InsertZoneMappings can partially insert; wrap in a transaction or provide cleanup on failure.
- ValidateZoneAndWards: convert
allowedWardsto a set (map) instead of O(n*m)contains.Also applies to: 657-684
| // Logs fatal and exits if the connection fails. | ||
| func Connect(cfg *config.Config) *gorm.DB { | ||
| dsn := fmt.Sprintf( | ||
| "host=%s user=%s password=%s dbname=%s port=%s sslmode=disable client_encoding=UTF8 search_path=%s", |
There was a problem hiding this comment.
Critical security issue: Database connections are unencrypted.
The DSN uses sslmode=disable, which means all database traffic is sent in plaintext. This exposes sensitive data (passwords, PII like Aadhaar numbers, user credentials) to network sniffing attacks.
Change to a secure SSL mode:
- "host=%s user=%s password=%s dbname=%s port=%s sslmode=disable client_encoding=UTF8 search_path=%s",
+ "host=%s user=%s password=%s dbname=%s port=%s sslmode=require client_encoding=UTF8 search_path=%s",For even better security, consider sslmode=verify-full with proper certificate validation:
"host=%s user=%s password=%s dbname=%s port=%s sslmode=verify-full client_encoding=UTF8 search_path=%s"Make the SSL mode configurable via environment variable for flexibility across environments.
🤖 Prompt for AI Agents
in onboarding-backend/internal/database/database.go around line 19, the DSN
hardcodes sslmode=disable which sends DB traffic unencrypted; change the code to
read an environment variable (e.g., DB_SSLMODE) and use that value in the DSN,
defaulting to a secure value such as "verify-full" (or "require" for
environments without cert validation); ensure the chosen value is
validated/whitelisted to prevent invalid inputs and update any docs/config
templates to include DB_SSLMODE so environments can opt into less-strict modes
for dev while production uses verify-full.
| func (uh *UserHandler) DeleteUser(c *gin.Context) { | ||
| keycloakUserID := c.Param("id") | ||
| logger.Info(constants.LogDeleteUserRequest, "keycloakUserID", keycloakUserID) | ||
|
|
||
| if err := uh.userService.DeleteUserByKeycloakID(keycloakUserID); err != nil { | ||
| logger.Error(constants.LogFailedToDeleteUser, "error", err, "keycloakUserID", keycloakUserID) | ||
|
|
||
| if err.Error() == fmt.Sprintf("user not found with keycloak_user_id: %s", keycloakUserID) { | ||
| response.Error(c, http.StatusNotFound, constants.ErrMsgUserNotFound, err.Error()) | ||
| return | ||
| } | ||
| response.Error(c, http.StatusInternalServerError, constants.ErrMsgFailedToDeleteUser, err.Error()) | ||
| return | ||
| } | ||
| logger.Info("User deleted successfully", "keycloakUserID", keycloakUserID) | ||
| response.Success(c, http.StatusOK, "User deleted successfully", nil) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the GetUser handler to understand the "consistent" empty id handling
cd onboarding-backend && find . -name "user_handler.go" -type fRepository: egovernments/Infosys
Length of output: 99
🏁 Script executed:
# Search for GetUser implementation to see how it handles empty id
rg -A 15 "func \(uh \*UserHandler\) GetUser" --type go onboarding-backend/Repository: egovernments/Infosys
Length of output: 4357
🏁 Script executed:
# Check the DeleteUserByKeycloakID method signature in the service layer
rg -A 5 "DeleteUserByKeycloakID" --type go onboarding-backend/Repository: egovernments/Infosys
Length of output: 1758
🏁 Script executed:
# Look for error handling patterns and typed errors in the service layer
rg -B 2 -A 10 "user not found with keycloak_user_id" --type go onboarding-backend/Repository: egovernments/Infosys
Length of output: 1304
🏁 Script executed:
# Check how other handlers use context and handle errors
rg -B 2 -A 8 "c.Request.Context()" --type go onboarding-backend/internal/handlers/Repository: egovernments/Infosys
Length of output: 5348
🏁 Script executed:
# Find all callers of DeleteUserByKeycloakID
rg "DeleteUserByKeycloakID" --type go onboarding-backend/Repository: egovernments/Infosys
Length of output: 464
🏁 Script executed:
# Check other service methods to understand the context pattern
rg -A 2 "func \(s \*UserService\) Get" --type go onboarding-backend/internal/services/user_service.go | head -30Repository: egovernments/Infosys
Length of output: 869
🏁 Script executed:
# Look at the DeleteUserByKeycloakID full implementation in service
rg -A 20 "func \(s \*UserService\) DeleteUserByKeycloakID" --type go onboarding-backend/Repository: egovernments/Infosys
Length of output: 1886
DeleteUser should use request context + avoid brittle string-based error matching
Also handle empty id consistently (like GetUser does).
Note: The suggested fix requires updating the service method signature. DeleteUserByKeycloakID currently accepts only keycloakUserID but needs to accept ctx context.Context as the first parameter to align with other service methods (which all take context.Context). The service also currently uses context.Background() internally, which should be replaced with the passed context. Additionally, the brittle string matching for the 404 case should be replaced with a typed error returned from the service layer.
func (uh *UserHandler) DeleteUser(c *gin.Context) {
keycloakUserID := c.Param("id")
@@
+ if keycloakUserID == "" {
+ response.Error(c, http.StatusBadRequest, constants.ErrMsgMissingUserID, constants.ErrMsgMissingUserIDDetail)
+ return
+ }
- if err := uh.userService.DeleteUserByKeycloakID(keycloakUserID); err != nil {
+ if err := uh.userService.DeleteUserByKeycloakID(c.Request.Context(), keycloakUserID); err != nil {
logger.Error(constants.LogFailedToDeleteUser, "error", err, "keycloakUserID", keycloakUserID)
-
- if err.Error() == fmt.Sprintf("user not found with keycloak_user_id: %s", keycloakUserID) {
- response.Error(c, http.StatusNotFound, constants.ErrMsgUserNotFound, err.Error())
- return
- }
+ // Prefer errors.Is(...) or a typed error from service for 404 mapping.
response.Error(c, http.StatusInternalServerError, constants.ErrMsgFailedToDeleteUser, err.Error())
return
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In onboarding-backend/internal/handlers/user_handler.go around lines 185 to 201,
Update DeleteUser to use the request context, validate empty id like GetUser,
and stop brittle string-based error matching: change the call to
uh.userService.DeleteUserByKeycloakID to pass c.Request.Context() as the first
arg (so the service signature becomes DeleteUserByKeycloakID(ctx
context.Context, keycloakUserID string)), check for keycloakUserID == "" and
return the same bad-request handling as GetUser, and replace err.Error() string
comparison with a typed sentinel error returned by the service (e.g.
service.ErrUserNotFound) to decide between 404 and 500; also ensure the service
implementation stops using context.Background() and uses the passed ctx instead.
| func (r *PostgreSQLUserRepository) GetAllUsersWithFilters(ctx context.Context, filters models.UserFilters, limit, offset int) ([]*models.User, int64, error) { | ||
| logger.Info(constants.LogGetUsersWithFiltersStartRetrive) | ||
|
|
||
| var users []*models.User | ||
| var totalCount int64 | ||
| query := r.dbd.WithContext(ctx).Model(&models.User{}).Where("deleted = false") | ||
|
|
||
| // Apply filters | ||
| if filters.Role != nil { | ||
| query = query.WithContext(ctx).Where("role IN ?", filters.Role) | ||
| } | ||
| if filters.IsActive != nil { | ||
| query = query.WithContext(ctx).Where("is_active = ?", *filters.IsActive) | ||
| } | ||
| if filters.Username != nil && *filters.Username != "" { | ||
| query = query.WithContext(ctx).Where("username ILIKE ?", "%"+*filters.Username+"%") | ||
| } | ||
| if filters.Email != nil && *filters.Email != "" { | ||
| query = query.WithContext(ctx).Where("email ILIKE ?", "%"+*filters.Email+"%") | ||
| } | ||
|
|
||
| if filters.Ward != nil { | ||
| query = query.WithContext(ctx).Joins("JOIN \"DIGIT3\".\"zone_mapping\" ON \"DIGIT3\".\"zone_mapping\".user_id = \"DIGIT3\".\"users\".keycloak_user_id"). | ||
| Where("\"DIGIT3\".\"zone_mapping\".ward && ?", pq.Array(filters.Ward)) | ||
| } | ||
|
|
||
| if filters.PhoneNumber != nil { | ||
| query = query.WithContext(ctx).Joins("JOIN \"DIGIT3\".\"user_profiles\" ON \"DIGIT3\".\"users\".keycloak_user_id = \"DIGIT3\".\"user_profiles\".user_profile_id"). | ||
| Where("\"DIGIT3\".\"user_profiles\".phone_number = ?", *filters.PhoneNumber) | ||
| } | ||
|
|
||
| // Count total records | ||
| if err := query.WithContext(ctx).Count(&totalCount).Error; err != nil { | ||
| logger.Error(constants.ErrCountUsersWithFiltersFailed, "error", err) | ||
| return nil, 0, errors.NewRepositoryError(fmt.Sprintf("%s: %v", constants.ErrCountUsersWithFiltersFailed, err)) | ||
| } | ||
|
|
||
| // Apply pagination and retrieve users | ||
| if err := query.WithContext(ctx).Limit(limit).Offset(offset).Find(&users).Error; err != nil { | ||
| logger.Error(constants.ErrRetrieveUsersWithFiltersFailed, "error", err) | ||
| return nil, 0, errors.NewRepositoryError(fmt.Sprintf("%s: %v", constants.ErrRetrieveUsersWithFiltersFailed, err)) | ||
| } | ||
| logger.Info(constants.LogGetUsersWithFiltersSuccess, "totalCount", totalCount, "returnedCount", len(users), "limit", limit, "offset", offset) | ||
| return users, totalCount, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the models.UserFilters definition to verify the type of filters.Ward
find . -type f -name "*.go" | head -20Repository: egovernments/Infosys
Length of output: 1125
🏁 Script executed:
# Search for UserFilters definition
rg "type UserFilters" -A 15Repository: egovernments/Infosys
Length of output: 1377
🏁 Script executed:
# Also look for the models.User definition to understand the structure
rg "type User struct" -A 20Repository: egovernments/Infosys
Length of output: 3398
🏁 Script executed:
# Check how pq.Array is used elsewhere in the codebase
rg "pq\.Array" -B 2 -A 2Repository: egovernments/Infosys
Length of output: 597
🏁 Script executed:
# Check the full GetAllUsersWithFilters function
sed -n '341,385p' onboarding-backend/internal/repositories/users_repo.goRepository: egovernments/Infosys
Length of output: 2243
🏁 Script executed:
# Look at the imports in users_repo.go to understand pq usage
head -50 onboarding-backend/internal/repositories/users_repo.goRepository: egovernments/Infosys
Length of output: 2066
🏁 Script executed:
# Check if there are any tests for this function
find . -type f -name "*test*.go" | xargs grep -l "GetAllUsersWithFilters" 2>/dev/nullRepository: egovernments/Infosys
Length of output: 48
🏁 Script executed:
# Look for other GORM usage patterns with Distinct in the codebase
rg "\.Distinct\(" -B 2 -A 2Repository: egovernments/Infosys
Length of output: 46
🏁 Script executed:
# Search for any GORM documentation or examples in comments
rg "gorm" -i onboarding-backend/internal/repositories/ | grep -i distinctRepository: egovernments/Infosys
Length of output: 46
🏁 Script executed:
# Check GORM package usage patterns in the codebase
rg "\.Count\(" -B 2 -A 2 onboarding-backend/internal/repositories/Repository: egovernments/Infosys
Length of output: 2795
🏁 Script executed:
# Look for other instances where joins might cause similar issues
rg "Joins" -B 1 -A 1 onboarding-backend/internal/repositories/users_repo.goRepository: egovernments/Infosys
Length of output: 599
Fix pq.Array() pointer dereference and add DISTINCT to handle join duplicates
The filters.Ward field is *[]string, so it must be dereferenced: pq.Array(*filters.Ward). More critically, both the zone_mapping and user_profiles joins can produce duplicate rows (one user can have multiple zone mappings or profiles), causing Count() to inflate the total and Find() to return duplicate user objects. Add DISTINCT on the primary key to both operations:
if filters.Ward != nil {
query = query.WithContext(ctx).Joins("JOIN \"DIGIT3\".\"zone_mapping\" ON \"DIGIT3\".\"zone_mapping\".user_id = \"DIGIT3\".\"users\".keycloak_user_id").
- Where("\"DIGIT3\".\"zone_mapping\".ward && ?", pq.Array(filters.Ward))
+ Where("\"DIGIT3\".\"zone_mapping\".ward && ?", pq.Array(*filters.Ward))
}
- // Count total records
- if err := query.WithContext(ctx).Count(&totalCount).Error; err != nil {
+ // Count total records (guard against join duplicates)
+ if err := query.WithContext(ctx).Distinct("\"DIGIT3\".\"users\".keycloak_user_id").Count(&totalCount).Error; err != nil {
logger.Error(constants.ErrCountUsersWithFiltersFailed, "error", err)
return nil, 0, errors.NewRepositoryError(fmt.Sprintf("%s: %v", constants.ErrCountUsersWithFiltersFailed, err))
}
- if err := query.WithContext(ctx).Limit(limit).Offset(offset).Find(&users).Error; err != nil {
+ if err := query.WithContext(ctx).Distinct("\"DIGIT3\".\"users\".keycloak_user_id").Limit(limit).Offset(offset).Find(&users).Error; err != nil {
logger.Error(constants.ErrRetrieveUsersWithFiltersFailed, "error", err)
return nil, 0, errors.NewRepositoryError(fmt.Sprintf("%s: %v", constants.ErrRetrieveUsersWithFiltersFailed, err))
}🤖 Prompt for AI Agents
in onboarding-backend/internal/repositories/users_repo.go around lines 341 to
385, the code incorrectly passes filters.Ward (a *[ ]string) to pq.Array without
dereferencing and the JOINs can create duplicate rows which inflate Count() and
return duplicate users; fix by using pq.Array(*filters.Ward) and apply DISTINCT
on the users primary key for both the count and the retrieval (e.g., use
Distinct on the primary key column in the query before Count() and before
Find()) so duplicates from joins are eliminated.
| // Schedule the task to run every day at midnight | ||
| c.AddFunc("*/2 * * * *", func() { |
There was a problem hiding this comment.
Critical: Cron expression does not match the comment.
The comment states "every day at midnight" but the cron expression "*/2 * * * *" runs every 2 minutes. This is a significant discrepancy that could cause unexpected behavior in production.
For daily execution at midnight, use:
- // Schedule the task to run every day at midnight
- c.AddFunc("*/2 * * * *", func() {
+ // Schedule the task to run every day at midnight
+ c.AddFunc("0 0 * * *", func() {If every 2 minutes is intentional (e.g., for development/testing), update the comment:
- // Schedule the task to run every day at midnight
+ // Schedule the task to run every 2 minutes (development mode)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Schedule the task to run every day at midnight | |
| c.AddFunc("*/2 * * * *", func() { | |
| // Schedule the task to run every day at midnight | |
| c.AddFunc("0 0 * * *", func() { |
🤖 Prompt for AI Agents
in onboarding-backend/internal/scheduler/scheduler.go around lines 20 to 21 the
comment states "Schedule the task to run every day at midnight" but the cron
expression "*/2 * * * *" runs every 2 minutes; replace the cron expression with
"0 0 * * *" to schedule daily at midnight, or if the 2-minute interval is
intentional (e.g., for testing) update the comment to accurately reflect "every
2 minutes".
| func (ks *KeycloakService) GetUserByID(userID string) (*models.KeycloakUser, error) { | ||
| token, err := ks.GetAdminToken() | ||
| if err != nil { | ||
| return nil, fmt.Errorf(constants.ErrGetAdminToken) | ||
| } | ||
| url := fmt.Sprintf("%s/admin/realms/%s/users/%s", ks.BaseURL, ks.Realm, userID) | ||
|
|
||
| req, err := http.NewRequest("GET", url, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf(constants.ErrCreateUserRequest) | ||
| } | ||
|
|
||
| req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) | ||
|
|
||
| client := &http.Client{} | ||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf(constants.ErrFetchKeycloakUser) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf(constants.ErrKeycloakAPIStatus+": %d", resp.StatusCode) | ||
| } | ||
|
|
||
| var keycloakUser models.KeycloakUser | ||
| if err := json.NewDecoder(resp.Body).Decode(&keycloakUser); err != nil { | ||
| return nil, fmt.Errorf(constants.ErrDecodeKeycloakUser+": %w", err) | ||
| } | ||
|
|
||
| // Check expiryDate | ||
| expiryDateInterface, exists := keycloakUser.Attributes["expiryDate"] | ||
| if exists { | ||
| expiryDateSlice, ok := expiryDateInterface.([]interface{}) | ||
| log.Printf("expiryDateInterface: %v, Type: %T\n", expiryDateInterface, expiryDateInterface) | ||
| if !ok || len(expiryDateSlice) == 0 { | ||
| return nil, fmt.Errorf("expiryDate attribute is missing or invalid for user %s", userID) | ||
| } | ||
|
|
||
| expiryDate, err := time.Parse("2006-01-02", expiryDateSlice[0].(string)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid expiryDate format for user %s: %w", userID, err) | ||
| } | ||
|
|
||
| if time.Now().After(expiryDate) { | ||
| // Disable the user in Keycloak | ||
| if disableErr := ks.DisableUser(userID); disableErr != nil { | ||
| return nil, fmt.Errorf("failed to disable expired user %s: %w", userID, disableErr) | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("user %s is expired and has been disabled", userID) | ||
| } | ||
| } | ||
|
|
||
| return &keycloakUser, nil | ||
| } |
There was a problem hiding this comment.
GetUserByID can hang and can panic (unsafe attribute casting); also has side effects + debug logging.
- Uses
client := &http.Client{}with no timeout (hang risk). expiryDateSlice[0].(string)can panic; you also treat a date as midnight, which will expire users at the start of the expiry day.log.Printf(...)looks like debug residue and may leak data; use your structured logger (or remove).
- client := &http.Client{}
- resp, err := client.Do(req)
+ resp, err := ks.httpClient.Do(req)
if err != nil {
return nil, fmt.Errorf(constants.ErrFetchKeycloakUser)
}
@@
- expiryDateInterface, exists := keycloakUser.Attributes["expiryDate"]
- if exists {
- expiryDateSlice, ok := expiryDateInterface.([]interface{})
- log.Printf("expiryDateInterface: %v, Type: %T\n", expiryDateInterface, expiryDateInterface)
- if !ok || len(expiryDateSlice) == 0 {
- return nil, fmt.Errorf("expiryDate attribute is missing or invalid for user %s", userID)
- }
-
- expiryDate, err := time.Parse("2006-01-02", expiryDateSlice[0].(string))
+ if raw, exists := keycloakUser.Attributes["expiryDate"]; exists {
+ var expiryStr string
+ switch v := raw.(type) {
+ case []string:
+ if len(v) > 0 { expiryStr = v[0] }
+ case []interface{}:
+ if len(v) > 0 {
+ if s, ok := v[0].(string); ok { expiryStr = s }
+ }
+ }
+ if expiryStr == "" {
+ return nil, fmt.Errorf("expiryDate attribute is missing or invalid for user %s", userID)
+ }
+ expiryDate, err := time.Parse("2006-01-02", expiryStr)
if err != nil {
return nil, fmt.Errorf("invalid expiryDate format for user %s: %w", userID, err)
}
-
- if time.Now().After(expiryDate) {
+ // Treat expiry as end-of-day, not start-of-day.
+ if time.Now().After(expiryDate.Add(24 * time.Hour)) {
// Disable the user in Keycloak
if disableErr := ks.DisableUser(userID); disableErr != nil {
return nil, fmt.Errorf("failed to disable expired user %s: %w", userID, disableErr)
}Committable suggestion skipped: line range outside the PR's diff.
| client := &http.Client{} | ||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| logger.Error(constants.ErrCallMDMSAPI, "error", err) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Critical: HTTP client has no timeout, risking indefinite hangs.
The HTTP client is created without a timeout, which can cause the request to hang indefinitely if the MDMS API is unresponsive. This can exhaust goroutines and impact service availability.
- client := &http.Client{}
+ client := &http.Client{
+ Timeout: 30 * time.Second,
+ }
resp, err := client.Do(req)Don't forget to import time:
import (
"encoding/json"
"fmt"
"net/http"
+ "time"
"property-tax-onboarding/internal/config"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| client := &http.Client{} | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| logger.Error(constants.ErrCallMDMSAPI, "error", err) | |
| return nil, err | |
| } | |
| client := &http.Client{ | |
| Timeout: 30 * time.Second, | |
| } | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| logger.Error(constants.ErrCallMDMSAPI, "error", err) | |
| return nil, err | |
| } |
| client := &http.Client{} | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| logger.Error(constants.ErrCallMDMSAPI, "error", err) | |
| return nil, err | |
| } | |
| import ( | |
| "encoding/json" | |
| "fmt" | |
| "net/http" | |
| "time" | |
| "property-tax-onboarding/internal/config" | |
| ) |
🤖 Prompt for AI Agents
In onboarding-backend/internal/utils/mdms_utils.go around lines 57 to 62 the
http.Client is created without a timeout causing potential indefinite hangs;
replace client := &http.Client{} with a client that sets a sensible Timeout
(e.g. http.Client{Timeout: 10 * time.Second}) and add the time import; ensure
the timeout value is configurable if needed and use this client for
client.Do(req) so requests fail fast instead of blocking goroutines.
| ownership_share DOUBLE PRECISION NOT NULL DEFAULT 0 -- Ownership share (0-100%) | ||
| is_primary_owner BOOLEAN DEFAULT FALSE, -- Indicates if the user is the primary owner |
There was a problem hiding this comment.
Critical: Missing comma between columns.
There's a missing comma at the end of line 18 after the ownership_share definition, which will cause a SQL syntax error.
- ownership_share DOUBLE PRECISION NOT NULL DEFAULT 0 -- Ownership share (0-100%)
+ ownership_share DOUBLE PRECISION NOT NULL DEFAULT 0, -- Ownership share (0-100%)
is_primary_owner BOOLEAN DEFAULT FALSE, -- Indicates if the user is the primary owner📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ownership_share DOUBLE PRECISION NOT NULL DEFAULT 0 -- Ownership share (0-100%) | |
| is_primary_owner BOOLEAN DEFAULT FALSE, -- Indicates if the user is the primary owner | |
| ownership_share DOUBLE PRECISION NOT NULL DEFAULT 0, -- Ownership share (0-100%) | |
| is_primary_owner BOOLEAN DEFAULT FALSE, -- Indicates if the user is the primary owner |
🤖 Prompt for AI Agents
In onboarding-backend/migrations/user_profiles.sql around lines 18 to 19, the
column definition for ownership_share is missing a trailing comma which will
produce a SQL syntax error; add a comma at the end of the ownership_share line
so the list of column definitions is properly separated before the
is_primary_owner column.
| CONSTRAINT userprofile_address_id_fkey FOREIGN KEY (address_id) REFERENCES address(id) ON DELETE CASCADE -- Foreign key to the address table | ||
| CONSTRAINT userprofile_ownership_share_check CHECK (ownership_share >= 0 AND ownership_share <= 100) -- Checking the Ownership Constraints |
There was a problem hiding this comment.
Critical: Missing comma between constraints.
There's a missing comma at the end of line 25 after the foreign key constraint, which will cause a SQL syntax error.
- CONSTRAINT userprofile_address_id_fkey FOREIGN KEY (address_id) REFERENCES address(id) ON DELETE CASCADE -- Foreign key to the address table
+ CONSTRAINT userprofile_address_id_fkey FOREIGN KEY (address_id) REFERENCES address(id) ON DELETE CASCADE, -- Foreign key to the address table
CONSTRAINT userprofile_ownership_share_check CHECK (ownership_share >= 0 AND ownership_share <= 100) -- Checking the Ownership Constraints 📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CONSTRAINT userprofile_address_id_fkey FOREIGN KEY (address_id) REFERENCES address(id) ON DELETE CASCADE -- Foreign key to the address table | |
| CONSTRAINT userprofile_ownership_share_check CHECK (ownership_share >= 0 AND ownership_share <= 100) -- Checking the Ownership Constraints | |
| CONSTRAINT userprofile_address_id_fkey FOREIGN KEY (address_id) REFERENCES address(id) ON DELETE CASCADE, -- Foreign key to the address table | |
| CONSTRAINT userprofile_ownership_share_check CHECK (ownership_share >= 0 AND ownership_share <= 100) -- Checking the Ownership Constraints |
🤖 Prompt for AI Agents
In onboarding-backend/migrations/user_profiles.sql around lines 25 to 26, the
two CONSTRAINT declarations are missing a separating comma after the foreign key
constraint on line 25; add a comma at the end of the CONSTRAINT
userprofile_address_id_fkey ... ON DELETE CASCADE line so the subsequent
CONSTRAINT userprofile_ownership_share_check ... is a valid list element,
ensuring proper SQL syntax for the CREATE TABLE (or ALTER TABLE) statement.
| CREATE TABLE users ( | ||
| keycloak_user_id VARCHAR(255) NOT NULL PRIMARY KEY, -- Unique identifier for the user | ||
| username VARCHAR(255) NOT NULL UNIQUE, -- Username must be unique | ||
| email VARCHAR(255) NOT NULL UNIQUE, -- Email must be unique | ||
| role VARCHAR(50) NOT NULL, -- Role of the user (e.g., AGENT, CITIZEN, etc.) | ||
| is_active BOOLEAN NOT NULL DEFAULT TRUE, -- Indicates if the user is active | ||
| preferred_language VARCHAR(50), -- Preferred language of the user (nullable) | ||
| created_at TIMESTAMP NOT NULL DEFAULT NOW(), -- Timestamp when the record was created | ||
| updated_at TIMESTAMP NOT NULL DEFAULT NOW(), -- Timestamp when the record was last updated | ||
| created_by VARCHAR(255), -- User who created the record (nullable) | ||
| updated_by VARCHAR(255), -- User who last updated the record (nullable) | ||
| start_date DATE, -- start date of the user | ||
| end_date DATE -- end_date of the user | ||
| CONSTRAINT users_pkey PRIMARY KEY (keycloak_user_id), -- primary key | ||
| CONSTRAINT users_email_key UNIQUE (email), -- email set as a unique | ||
| CONSTRAINT users_username_key UNIQUE (username) -- username set as a unique | ||
| ); | ||
| CREATE INDEX IF NOT EXISTS idx_users_deleted ON users (deleted); --indexing for deleted |
There was a problem hiding this comment.
Migration is currently invalid SQL (duplicate PK + missing comma) and will fail
You define the primary key twice and end_date is missing a trailing comma before constraints.
-CREATE TABLE users (
- keycloak_user_id VARCHAR(255) NOT NULL PRIMARY KEY, -- Unique identifier for the user
+CREATE TABLE DIGIT3.users (
+ keycloak_user_id VARCHAR(255) NOT NULL, -- Unique identifier for the user
username VARCHAR(255) NOT NULL UNIQUE, -- Username must be unique
email VARCHAR(255) NOT NULL UNIQUE, -- Email must be unique
role VARCHAR(50) NOT NULL, -- Role of the user (e.g., AGENT, CITIZEN, etc.)
is_active BOOLEAN NOT NULL DEFAULT TRUE, -- Indicates if the user is active
preferred_language VARCHAR(50), -- Preferred language of the user (nullable)
created_at TIMESTAMP NOT NULL DEFAULT NOW(), -- Timestamp when the record was created
updated_at TIMESTAMP NOT NULL DEFAULT NOW(), -- Timestamp when the record was last updated
created_by VARCHAR(255), -- User who created the record (nullable)
updated_by VARCHAR(255), -- User who last updated the record (nullable)
start_date DATE, -- start date of the user
- end_date DATE -- end_date of the user
- CONSTRAINT users_pkey PRIMARY KEY (keycloak_user_id), -- primary key
- CONSTRAINT users_email_key UNIQUE (email), -- email set as a unique
- CONSTRAINT users_username_key UNIQUE (username) -- username set as a unique
+ end_date DATE, -- end_date of the user
+ CONSTRAINT users_pkey PRIMARY KEY (keycloak_user_id)
);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In onboarding-backend/migrations/users.sql around lines 1 to 18, the CREATE
TABLE is invalid because you declared keycloak_user_id as a PRIMARY KEY inline
and then declared a duplicate PRIMARY KEY constraint, and the end_date column is
missing a trailing comma before the CONSTRAINT blocks; fix by removing one of
the primary key declarations (either remove the inline PRIMARY KEY on
keycloak_user_id or remove the CONSTRAINT users_pkey line) and add a comma after
the end_date column definition so the subsequent CONSTRAINT lines are valid.
| CONSTRAINT users_email_key UNIQUE (email), -- email set as a unique | ||
| CONSTRAINT users_username_key UNIQUE (username) -- username set as a unique | ||
| ); | ||
| CREATE INDEX IF NOT EXISTS idx_users_deleted ON users (deleted); --indexing for deleted |
There was a problem hiding this comment.
Index references a non-existent column (deleted)
Either add a deleted column (and semantics) or drop this index.
-CREATE INDEX IF NOT EXISTS idx_users_deleted ON users (deleted); --indexing for deleted
+-- CREATE INDEX IF NOT EXISTS idx_users_deleted ON DIGIT3.users (deleted);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE INDEX IF NOT EXISTS idx_users_deleted ON users (deleted); --indexing for deleted | |
| -- CREATE INDEX IF NOT EXISTS idx_users_deleted ON DIGIT3.users (deleted); |
🤖 Prompt for AI Agents
In onboarding-backend/migrations/users.sql around line 18, the CREATE INDEX
references a non-existent column "deleted"; either add a nullable
boolean/timestamp "deleted" column (with intended semantics: soft-delete flag or
deletion timestamp) to the users table before creating the index, or remove/drop
this index line from the migration; if adding the column, include its DDL (e.g.,
ALTER TABLE users ADD COLUMN deleted BOOLEAN DEFAULT FALSE) in the same
migration and then create the index, otherwise delete this CREATE INDEX
statement so migrations run without error.
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (15)
backend/onboarding/migrations/users.sql (2)
1-17: Migration has critical SQL syntax errors (duplicate PK + missing comma).The table definition has two issues:
keycloak_user_idis declared as PRIMARY KEY inline (line 2) AND via CONSTRAINT (line 14) - this is a duplicate declarationend_date(line 13) is missing a trailing comma before the CONSTRAINT block🔎 Proposed fix
CREATE TABLE users ( - keycloak_user_id VARCHAR(255) NOT NULL PRIMARY KEY, -- Unique identifier for the user + keycloak_user_id VARCHAR(255) NOT NULL, -- Unique identifier for the user username VARCHAR(255) NOT NULL UNIQUE, -- Username must be unique email VARCHAR(255) NOT NULL UNIQUE, -- Email must be unique role VARCHAR(50) NOT NULL, -- Role of the user (e.g., AGENT, CITIZEN, etc.) is_active BOOLEAN NOT NULL DEFAULT TRUE, -- Indicates if the user is active preferred_language VARCHAR(50), -- Preferred language of the user (nullable) created_at TIMESTAMP NOT NULL DEFAULT NOW(), -- Timestamp when the record was created updated_at TIMESTAMP NOT NULL DEFAULT NOW(), -- Timestamp when the record was last updated created_by VARCHAR(255), -- User who created the record (nullable) updated_by VARCHAR(255), -- User who last updated the record (nullable) start_date DATE, -- start date of the user - end_date DATE -- end_date of the user + end_date DATE, -- end_date of the user CONSTRAINT users_pkey PRIMARY KEY (keycloak_user_id), -- primary key CONSTRAINT users_email_key UNIQUE (email), -- email set as a unique CONSTRAINT users_username_key UNIQUE (username) -- username set as a unique );
18-18: Index references non-existentdeletedcolumn.The index
idx_users_deletedreferences a column nameddeletedwhich is not defined in the table schema. This will cause the migration to fail.🔎 Proposed fix
Either add the
deletedcolumn for soft-delete functionality:start_date DATE, -- start date of the user end_date DATE, -- end_date of the user + deleted BOOLEAN DEFAULT FALSE, -- soft delete flag CONSTRAINT users_pkey PRIMARY KEY (keycloak_user_id),Or remove the index if soft-delete is not needed:
-CREATE INDEX IF NOT EXISTS idx_users_deleted ON users (deleted); --indexing for deletedbackend/onboarding/pkg/logger/logger.go (2)
44-63: Type assertion on keys can panic (Error function).Same issue as Info: line 59 has unsafe type assertion that will panic if a key is not a string.
Apply the same safe type assertion fix as suggested for the Info function.
67-79: Type assertion on keys can panic (Warn function).Same issue: line 75 has unsafe type assertion.
Apply the same safe type assertion fix as suggested for the Info function.
backend/onboarding/internal/database/database.go (1)
18-21: Database connections remain unencrypted.The DSN still uses
sslmode=disable, exposing all database traffic to potential interception. This issue was previously flagged and has not been addressed.backend/onboarding/internal/scheduler/scheduler.go (1)
20-21: Cron expression still doesn't match the comment.The comment states "every day at midnight" but the cron expression
"*/2 * * * *"runs every 2 minutes. This critical discrepancy was previously flagged and remains unresolved.backend/onboarding/migrations/user_profiles.sql (2)
18-19: Missing comma causes SQL syntax error.Line 18 is missing a trailing comma after the
ownership_sharecolumn definition. This syntax error was previously flagged and has not been fixed.
25-26: Missing comma between constraints causes SQL syntax error.Line 25 is missing a trailing comma after the foreign key constraint. This syntax error was previously flagged and has not been fixed.
backend/onboarding/internal/utils/mdms_utils.go (1)
57-62: HTTP client lacks timeout, risking indefinite hangs.The HTTP client is created without a timeout. If the MDMS API becomes unresponsive, this will cause goroutines to hang indefinitely, potentially exhausting resources.
backend/onboarding/internal/handlers/user_handler.go (1)
185-201: Issues previously flagged: missing empty ID check, no context propagation, brittle string matching.This was flagged in a previous review. The handler should:
- Validate that
keycloakUserIDis not empty (likeGetUserdoes)- Pass
c.Request.Context()to the service method- Use typed errors instead of string comparison for 404 detection
backend/onboarding/internal/services/keycloak_service.go (1)
414-469: GetUserByID has multiple issues flagged in previous review.This method has issues that were already identified:
- Line 428: Creates
&http.Client{}without timeout instead of usingks.httpClient- Line 448: Debug
log.Printfshould use structured logger- Line 453: Type assertion
expiryDateSlice[0].(string)can panic if the element is not a stringbackend/onboarding/internal/repositories/users_repo.go (1)
341-385: Fixpq.Array()pointer dereference and addDISTINCTto handle join duplicates.The issues previously identified still apply:
filters.Wardis*[]string, so it must be dereferenced:pq.Array(*filters.Ward)- The
zone_mappinganduser_profilesjoins can produce duplicate rows, inflatingCount()and returning duplicate usersbackend/onboarding/internal/services/user_service.go (3)
57-97: Critical issues previously identified in CreateUser.The following issues were already flagged:
- Lines 60-63: Hard-coded default password
"8472"is a critical security flaw- Lines 79-90: Rollback logic checks
err, but subsequentif err := ...statements shadow the outererrvariable, preventing rollback from triggering
245-266: GetUser has a nil-dereference bug previously identified.When
identifierlength ≤ 30, the function only logs an error (line 260) but continues without fetching a user. The variableuserremainsnil, causing a panic whenuser.KeycloakUserIDis accessed on line 269.
154-181: Zone validation and mapping happen after transaction commit.Lines 158-181 perform zone validation and insert zone mappings after the database transaction has been committed (line 148). If zone validation fails, the user is already created in Keycloak and the database, leaving the system in an inconsistent state.
This was noted in previous comments - zone validation should occur before Keycloak user creation, and zone mapping inserts should be part of the transaction.
🟡 Minor comments (18)
backend/onboarding/README.md-166-194 (1)
166-194: Specify language for environment variables code blocks.Both code blocks containing shell environment variables should declare
bashas the language.🔎 Suggested fix
-```bash +```bash # Database Configuration DB_HOST=localhost(Already correct in lines 166-194, but line 204-226 should also specify
bash.)Also applies to: 204-226
backend/onboarding/README.md-33-33 (1)
33-33: Fix version inconsistency between sections.Line 33 states "Go 1.21+" as the language requirement, but line 306 requires "Go 1.24 or higher". Align these to a single supported version.
🔎 Suggested fix
- **Language**: Go 1.21+ + **Language**: Go 1.24+Or update line 306 to match line 33 if 1.21+ is the actual requirement.
Also applies to: 306-306
backend/onboarding/README.md-230-282 (1)
230-282: Specify language for project structure code block.🔎 Suggested fix
-``` +```plaintext property-tax-onboarding/ ├── .gitignorebackend/onboarding/README.md-60-72 (1)
60-72: Specify language for architecture diagram code block.Fenced code blocks should declare a language for syntax highlighting and linting consistency.
🔎 Suggested fix
-``` +```plaintext ┌─────────────────────────────────────────┐ │ API Gateway │ ← Handles Auth/AuthZbackend/onboarding/README.md-339-339 (1)
339-339: Wrap bare URL with Markdown link syntax.🔎 Suggested fix
- The service will be available at http://localhost:8080 + The service will be available at [http://localhost:8080](http://localhost:8080)backend/onboarding/README.md-145-147 (1)
145-147: Specify language for base URL code block.🔎 Suggested fix
-``` +```plaintext http://localhost:8080/api/v1 -``` +```backend/onboarding/README.md-312-339 (1)
312-339: Replace hard tabs with spaces for code block indentation.Lines 312–339 use hard tabs (indented shell commands), which violate Markdown style guides and can render inconsistently across viewers. Replace tabs with spaces.
🔎 Suggested fix (sample)
2. **Install Go dependencies** - ```bash - go mod download - ``` + ```bash + go mod download + ``` 3. **Configure environment variables** - Copy the example environment file and update values as needed: - ```bash - cp .env.example .env - # Edit .env with your database and service URLs - ``` + Copy the example environment file and update values as needed: + ```bash + cp .env.example .env + # Edit .env with your database and service URLs + ```Apply this pattern to all indented code blocks in the "Getting Started" section (lines 312–339).
Committable suggestion skipped: line range outside the PR's diff.
backend/onboarding/internal/constants/logs_constants.go-55-55 (1)
55-55: Fix typo in constant name.
LogUpdateUserStartthas a double 't' at the end.🔎 Proposed fix
- LogUpdateUserStartt = "Starting update of user" + LogUpdateUserStart = "Starting update of user"Note: This will require updating any references to this constant throughout the codebase.
backend/onboarding/internal/constants/logs_constants.go-67-67 (1)
67-67: Inconsistent naming convention.
ErrSoftDeleteUserFaileduses theErrprefix while all other constants in this file use theLogprefix. This inconsistency can cause confusion.🔎 Proposed fix
- ErrSoftDeleteUserFailed = "Failed to soft delete user" + LogSoftDeleteUserFailed = "Failed to soft delete user"backend/onboarding/internal/utils/mdms_utils.go-41-48 (1)
41-48: Missing input validation for zone parameter.The
zoneparameter is directly concatenated into the URL without validation. An empty string or special characters could lead to malformed URLs or unexpected behavior.🔎 Proposed fix
func GetZoneDetailsFromMDMS(zone string) (*MDMSResponse, error) { + if zone == "" { + return nil, fmt.Errorf("zone parameter cannot be empty") + } + // Fetch the MDMS API URL from the configuration url := fmt.Sprintf("%s%s", config.GetConfig().MDMS_API_URL, zone)backend/onboarding/internal/config/config.go-122-133 (1)
122-133: Docstring is misleading; function terminates process instead of returning error.The docstring states the function "returns an error describing missing or invalid settings" but the implementation calls
log.Fatalf, which terminates the process. Either update the docstring to reflect the actual behavior, or refactor to return an error as documented.🔎 Proposed docstring fix
-// validateConfig verifies that required configuration fields are present. -// It returns an error describing missing or invalid settings instead of -// terminating the process directly (caller decides how to handle it). +// validateConfig verifies that required configuration fields are present. +// It terminates the application with a fatal log if critical settings are missing. func validateConfig(cfg *Config) {backend/onboarding/internal/repositories/transaction.go-63-65 (1)
63-65: Verify logger.Error signature with multiple key-value pairs.Based on the logger implementation in
pkg/logger/logger.go, theErrorfunction expects key-value pairs after the message. Line 64 passes"error", err, "profileID", profile.IDwhich should work, but the first argument after the message is checked for being anerrortype. Here"error"is a string, so the error won't be extracted correctly.🔎 Proposed fix to match logger convention
if err := t.tx.Create(profile).Error; err != nil { - logger.Error(constants.ErrCreateUserProfileInTransaction, "error", err, "profileID", profile.ID) + logger.Error(constants.ErrCreateUserProfileInTransaction, err, "profileID", profile.ID) return fmt.Errorf("%s: %w", constants.ErrCreateUserProfileInTransaction, err) }Apply the same pattern to other
logger.Errorcalls in this file (lines 45, 80, 96, 114) if they also pass"error", errinstead of justerr.backend/onboarding/internal/validator/user_validation_service.go-109-116 (1)
109-116: Date validation may reject same-day start dates due to timestamp precision.
req.StartDate.Before(time.Now())compares timestamps, so a start date of "today" could fail if the time component is before the current moment. Consider truncating both to date-only comparison or allowing same-day starts.🔎 Suggested fix
if req.StartDate != nil && req.EndDate != nil { if req.EndDate.Before(*req.StartDate) { return fmt.Errorf(constants.ErrInvalidDateRange) } - if req.StartDate.Before(time.Now()) { + today := time.Now().Truncate(24 * time.Hour) + startDay := req.StartDate.Truncate(24 * time.Hour) + if startDay.Before(today) { return fmt.Errorf(constants.ErrStartDateInPast) } }backend/onboarding/internal/handlers/user_handler.go-117-122 (1)
117-122: Distinguish "not found" from other errors; use constant for error message.All errors return HTTP 500, but a "user not found" scenario should return HTTP 404. Also, line 120 uses a hardcoded string instead of
constants.ErrMsgFailedToGetUser.🔎 Suggested approach
userResponse, err := uh.userService.GetUser(c.Request.Context(), identifier) if err != nil { logger.Error(constants.LogFailedToGetUser, "error", err, "identifier", identifier) + if strings.Contains(err.Error(), "not found") { + response.Error(c, http.StatusNotFound, constants.ErrMsgUserNotFound, err.Error()) + return + } - response.Error(c, http.StatusInternalServerError, "Failed to get user", err.Error()) + response.Error(c, http.StatusInternalServerError, constants.ErrMsgFailedToGetUsers, err.Error()) return }Note: Ideally, use typed errors from the service layer instead of string matching.
backend/onboarding/internal/handlers/user_handler.go-273-273 (1)
273-273: Typo:roleLisdtshould beroleList.Minor typo affecting code readability.
🔎 Fix
- roleLisdt := strings.Split(roleStr, ",") + roleList := strings.Split(roleStr, ",") var roles []models.UserRole - for _, r := range roleLisdt { + for _, r := range roleList {Committable suggestion skipped: line range outside the PR's diff.
backend/onboarding/internal/repositories/zone_mapping_repo.go-27-38 (1)
27-38: Add GORM primary key tags to ZoneMapping struct.The database schema defines a composite primary key on
(user_id, zone)in the migration, but theZoneMappingstruct lacksgorm:"primaryKey"tags. Add these tags to bothUserIDandZonefields so GORM is aware of the primary key for proper handling of updates and other operations.type ZoneMapping struct { UserID string `gorm:"column:user_id;type:varchar(255);not null;primaryKey" json:"user_id"` Zone string `gorm:"column:zone;type:varchar(50);primaryKey" json:"zone"` // ... rest of fields }backend/onboarding/internal/models/user.go-198-220 (1)
198-220: EmptyAddressstruct always created when profile is non-nil.When
userProfileis not nil,Profile.Addressis always set to&Address{}(an empty struct) regardless of whether the user actually has address data. This may cause empty address objects to appear in JSON responses.🔎 Proposed fix
if userProfile != nil { ur.Profile = UserProfileResponse{ FirstName: userProfile.FirstName, LastName: userProfile.LastName, FullName: userProfile.FullName, PhoneNumber: userProfile.PhoneNumber, AdhaarNo: userProfile.AdhaarNo, Gender: userProfile.Gender, Guardian: userProfile.Guardian, GuardianType: userProfile.GuardianType, DateOfBirth: userProfile.DateOfBirth, Department: userProfile.Department, Designation: userProfile.Designation, WorkLocation: userProfile.WorkLocation, ProfilePicture: userProfile.ProfilePicture, RelationshipToProperty: userProfile.RelationshipToProperty, OwnershipShare: userProfile.OwnershipShare, IsPrimaryOwner: userProfile.IsPrimaryOwner, IsVerified: userProfile.IsVerified, - Address: &Address{}, + // Address should be populated by the caller when available } }Committable suggestion skipped: line range outside the PR's diff.
backend/onboarding/internal/repositories/users_repo.go-143-151 (1)
143-151: Missing context in database query.The query on line 144 doesn't use
.WithContext(ctx)unlike other queries in this file. This could prevent proper request cancellation and timeout handling.🔎 Proposed fix
// First check if user exists var user models.User - if err := r.dbd.Where("keycloak_user_id = ?", keycloakUserID).First(&user).Error; err != nil { + if err := r.dbd.WithContext(ctx).Where("keycloak_user_id = ?", keycloakUserID).First(&user).Error; err != nil { if err == gorm.ErrRecordNotFound {
🧹 Nitpick comments (16)
backend/onboarding/.dockerignore (1)
1-1: *Consider narrowing the .json exclusion.The blanket
*.jsonexclusion prevents any JSON files from being included in the Docker image. This might exclude necessary configuration files or data files that the application needs at runtime.🔎 Recommended refinement
If you only want to exclude the Postman collection, be more specific:
-*.json +postman/*.json *.md /migrations .env *.yaml *.yml .gitignorebackend/onboarding/internal/errors/errors.go (1)
5-20: Consider using standard library error handling.The custom
ServiceErrortype only wraps a message string. The standard libraryfmt.Errorfwith%wverb provides error wrapping and theerrorspackage offers similar functionality with less maintenance overhead.Alternative approach
import "fmt" func NewUserServiceError(message string, err error) error { return fmt.Errorf("UserService: %s: %w", message, err) }This enables error unwrapping with
errors.Isanderrors.As.backend/onboarding/internal/constants/logs_constants.go (1)
6-126: Consider parameterized messages to reduce duplication.Many log messages follow similar patterns (e.g., "Starting X", "Successfully Y", "Failed to Z"). Consider using a smaller set of template functions instead of individual constants for every operation.
Example approach
func LogOperationStart(operation string) string { return fmt.Sprintf("Starting %s", operation) } func LogOperationSuccess(operation string) string { return fmt.Sprintf("Successfully %s", operation) }This reduces constant proliferation while maintaining consistency.
backend/onboarding/migrations/address.sql (1)
1-10: Consider adding indexes for common query patterns.While the current schema is functional, you may want to add indexes on
city,state, or a composite index on(city, state)if you anticipate frequent filtering or searching by these fields in property tax queries.💡 Optional index additions
-- Add these after the PRIMARY KEY constraint, before the closing ); CREATE INDEX idx_address_city ON address(city); CREATE INDEX idx_address_state ON address(state); -- Or a composite index if you often filter by both: CREATE INDEX idx_address_city_state ON address(city, state);backend/onboarding/cmd/server/main.go (1)
96-105: Consider graceful shutdown for the scheduler and server.The scheduler starts a cron job that runs indefinitely, but there's no mechanism to stop it gracefully on server shutdown. Additionally, the server doesn't handle OS signals for graceful termination, which could lead to in-flight requests being dropped or scheduled tasks being interrupted.
Also, there's inconsistent logger usage: lines 101 and 104 use the standard
logpackage instead of the customloggerpackage used elsewhere.🔎 Suggested improvement for graceful shutdown
+import ( + "context" + "os" + "os/signal" + "syscall" + "net/http" // ... existing imports +) func main() { // ... existing initialization code ... - // Start the scheduler for user activation/deactivation - scheduler.ScheduleUserActivation(keycloakService, userRepo) + // Start the scheduler for user activation/deactivation (returns stop function) + stopScheduler := scheduler.ScheduleUserActivation(keycloakService, userRepo) + defer stopScheduler() // Start server serverAddr := cfg.ServerHost + ":" + cfg.ServerPort - log.Printf("Starting property tax onboarding service on %s", serverAddr) + logger.Info("Starting property tax onboarding service", "address", serverAddr) - if err := router.Run(serverAddr); err != nil { - log.Fatalf("Failed to start server: %v", err) - } + srv := &http.Server{Addr: serverAddr, Handler: router} + go func() { + if err := srv.ListenAndServe(); err != nil && err != http.ErrServerClosed { + logger.Fatal("Failed to start server", "error", err) + } + }() + + // Wait for interrupt signal + quit := make(chan os.Signal, 1) + signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM) + <-quit + logger.Info("Shutting down server...") + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + if err := srv.Shutdown(ctx); err != nil { + logger.Fatal("Server forced to shutdown", "error", err) + } }backend/onboarding/pkg/response/response.go (1)
31-38: Consider adding a variant for multiple error details.The
Errorfunction accepts a singleerrorDetailstring, but for validation errors you may want to return multiple issues at once. TheErrorsfield is already a slice, so adding anErrorshelper function could be useful.🔎 Optional helper for multiple errors
// Errors sends a standardized error JSON response with multiple error details. func Errors(c *gin.Context, statusCode int, message string, errorDetails []string) { response := APIResponse{ Success: false, Message: message, Errors: errorDetails, } c.JSON(statusCode, response) }backend/onboarding/internal/config/config.go (1)
54-56: Inconsistent naming:MDMS_API_URLuses underscores while other fields use CamelCase.For consistency with other fields like
KeycloakBaseURL, consider renaming toMDMSApiURLorMDMSAPIURL.backend/onboarding/internal/repositories/transaction.go (1)
92-102: Rollback after Commit will return an error; consider idempotent handling.GORM's
Rollback()on an already-committed transaction returnssql.ErrTxDone. This could cause misleading error logs if callers usedefer tx.Rollback()as a safety net pattern. Consider checking for this specific error.🔎 Proposed fix for idempotent rollback
+import "database/sql" + func (t *PostgreSQLTransaction) Rollback() error { logger.Info(constants.LogRollbackTransaction) if err := t.tx.Rollback().Error; err != nil { + // Ignore error if transaction was already committed or rolled back + if errors.Is(err, sql.ErrTxDone) { + return nil + } logger.Error(constants.ErrRollbackTransaction, "error", err) return fmt.Errorf("%s: %w", constants.ErrRollbackTransaction, err) } logger.Info(constants.LogTransactionRolledBack) return nil }backend/onboarding/internal/handlers/user_handler.go (2)
69-89: Consider distinguishing server-side errors from client errors.All
CreateUserfailures return HTTP 400, but some failures (e.g., Keycloak unavailable, database errors) are server-side issues that should return HTTP 500. Consider differentiating error types in the service layer.
331-339: Avoid string-based error matching for "no users found".Using
strings.Contains(err.Error(), "no users found")is fragile. Consider returning an empty list with HTTP 200 instead of HTTP 404 for "no results" scenarios, which is more RESTful for collection endpoints.backend/onboarding/internal/validator/user_validation_service.go (1)
139-151: Consider masking phone numbers in logs.Phone numbers are PII. While less sensitive than Aadhaar, consider masking (e.g.,
****1234) in production logs to reduce privacy exposure.backend/onboarding/internal/repositories/interfaces.go (1)
9-37: Interface is comprehensive but has minor consistency issues.
Count()(line 13) lacks acontext.Contextparameter, unlike other methodsGetUserCountsreturns(int64, int64, int64, error)- consider a named struct for clarity- Minor: missing space after comma on lines 17 and 27
🔎 Suggested improvements
- Count() (int64, error) - GetUserCounts(ctx context.Context) (int64, int64, int64, error) + Count(ctx context.Context) (int64, error) + GetUserCounts(ctx context.Context) (*UserCountsResult, error) // UserProfile operations - GetUserProfileByUserID(ctx context.Context,userID string) (*models.UserProfile, error) + GetUserProfileByUserID(ctx context.Context, userID string) (*models.UserProfile, error) // Get all users with filters - GetAllUsersWithFilters(ctx context.Context,filters models.UserFilters, limit, offset int) ([]*models.User, int64, error) + GetAllUsersWithFilters(ctx context.Context, filters models.UserFilters, limit, offset int) ([]*models.User, int64, error)Where
UserCountsResultis a struct with named fields (e.g.,TotalUsers,ActiveUsers,Agents).backend/onboarding/internal/models/user.go (1)
159-161: Remove GORM tag from DTO struct.
UpdateUserProfileis a request DTO but contains a GORM tag onAddressID. This mixes concerns between API contracts and persistence. Additionally, theAddressfield references*AddressDB(a database model) rather than a DTO type like*Address.🔎 Proposed fix
type UpdateUserProfile struct { // ... other fields ... - AddressID *string `gorm:"column:address_id" json:"addressId"` - Address *AddressDB `json:"address,omitempty"` + AddressID *string `json:"addressId"` + Address *Address `json:"address,omitempty"` }backend/onboarding/internal/services/keycloak_service.go (1)
648-650: Ignored marshal error.The error from
json.Marshalis discarded. While marshaling a simplemap[string]boolis unlikely to fail, ignoring errors is a bad practice and inconsistent with error handling inEnableUser(line 597-600).🔎 Proposed fix
reqBody := map[string]bool{"enabled": false} - body, _ := json.Marshal(reqBody) + body, err := json.Marshal(reqBody) + if err != nil { + return fmt.Errorf("failed to marshal request body: %w", err) + }backend/onboarding/internal/services/user_service.go (1)
559-563: Returning error when no users found may be unexpected for list endpoints.When no users match the filters, the method returns an error (line 562). This may be unexpected behavior for list/search APIs, which typically return an empty list with a 200 status rather than an error.
🔎 Proposed fix
// Check if no users were found if len(users) == 0 { - logger.Error(constants.ErrNoUsersFound) - return nil, errors.NewServiceError(constants.ErrNoUsersFound) + logger.Info("No users found matching filters") + return &models.UsersListResponse{ + Users: []*models.UserResponse{}, + TotalCount: 0, + Limit: limit, + Offset: offset, + }, nil }backend/onboarding/internal/constants/errors_constants.go (1)
62-87: Minor inconsistency in error message casing.Some error messages use lowercase ("failed to retrieve user profile") while others use title case ("Failed to check user existence"). Consider standardizing to one style for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
backend/onboarding/.dockerignore(1 hunks)backend/onboarding/.gitignore(1 hunks)backend/onboarding/Dockerfile(1 hunks)backend/onboarding/README.md(1 hunks)backend/onboarding/cmd/server/main.go(1 hunks)backend/onboarding/go.mod(1 hunks)backend/onboarding/internal/config/config.go(1 hunks)backend/onboarding/internal/constants/errors_constants.go(1 hunks)backend/onboarding/internal/constants/logs_constants.go(1 hunks)backend/onboarding/internal/database/database.go(1 hunks)backend/onboarding/internal/errors/errors.go(1 hunks)backend/onboarding/internal/handlers/user_handler.go(1 hunks)backend/onboarding/internal/middleware/cors.go(1 hunks)backend/onboarding/internal/models/database_models.go(1 hunks)backend/onboarding/internal/models/user.go(1 hunks)backend/onboarding/internal/repositories/interfaces.go(1 hunks)backend/onboarding/internal/repositories/transaction.go(1 hunks)backend/onboarding/internal/repositories/users_repo.go(1 hunks)backend/onboarding/internal/repositories/zone_mapping_repo.go(1 hunks)backend/onboarding/internal/routes/routes.go(1 hunks)backend/onboarding/internal/scheduler/scheduler.go(1 hunks)backend/onboarding/internal/services/keycloak_service.go(1 hunks)backend/onboarding/internal/services/user_service.go(1 hunks)backend/onboarding/internal/utils/keycloak_utils.go(1 hunks)backend/onboarding/internal/utils/mdms_utils.go(1 hunks)backend/onboarding/internal/validator/user_validation_service.go(1 hunks)backend/onboarding/migrations/address.sql(1 hunks)backend/onboarding/migrations/user_profiles.sql(1 hunks)backend/onboarding/migrations/users.sql(1 hunks)backend/onboarding/migrations/zone_mapping.sql(1 hunks)backend/onboarding/pkg/logger/logger.go(1 hunks)backend/onboarding/pkg/response/response.go(1 hunks)backend/onboarding/postman/new_onboarding.postman_collection.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/onboarding/.gitignore
🧰 Additional context used
🧬 Code graph analysis (14)
backend/onboarding/internal/errors/errors.go (2)
backend/onboarding/pkg/logger/logger.go (1)
Error(44-63)backend/onboarding/pkg/response/response.go (1)
Error(31-38)
backend/onboarding/cmd/server/main.go (12)
backend/onboarding/pkg/logger/logger.go (2)
InitLogger(19-24)Fatal(82-95)backend/onboarding/internal/config/config.go (1)
GetConfig(62-112)backend/onboarding/internal/database/database.go (1)
Connect(17-35)backend/onboarding/internal/repositories/users_repo.go (1)
NewPostgreSQLUserRepository(21-23)backend/onboarding/internal/services/keycloak_service.go (1)
NewKeycloakService(56-75)backend/onboarding/internal/validator/user_validation_service.go (1)
NewUserValidationService(28-33)backend/onboarding/internal/repositories/zone_mapping_repo.go (1)
NewPostgreSQLZoneMappingRepository(23-25)backend/onboarding/internal/services/user_service.go (1)
NewUserService(38-45)backend/onboarding/internal/handlers/user_handler.go (1)
NewUserHandler(33-37)backend/onboarding/internal/middleware/cors.go (2)
CORSConfig(12-18)CORSWithConfig(22-91)backend/onboarding/internal/routes/routes.go (1)
SetupRoutes(16-36)backend/onboarding/internal/scheduler/scheduler.go (1)
ScheduleUserActivation(17-33)
backend/onboarding/internal/utils/mdms_utils.go (4)
backend/onboarding/internal/config/config.go (1)
GetConfig(62-112)backend/onboarding/pkg/logger/logger.go (1)
Error(44-63)backend/onboarding/pkg/response/response.go (1)
Error(31-38)backend/onboarding/internal/constants/errors_constants.go (4)
ErrCreateMDMSRequest(99-99)ErrCallMDMSAPI(100-100)ErrMDMSAPINon200Status(101-101)ErrDecodeMDMSResponse(102-102)
backend/onboarding/internal/repositories/interfaces.go (3)
backend/onboarding/internal/models/database_models.go (5)
User(24-40)UserRole(12-12)UserProfile(44-65)AddressDB(69-76)AddressDB(79-81)backend/onboarding/internal/models/user.go (3)
UpdateUserRequest(129-138)UpdateUserProfile(141-161)UserFilters(164-172)backend/onboarding/internal/repositories/transaction.go (1)
Transaction(18-23)
backend/onboarding/internal/handlers/user_handler.go (7)
backend/onboarding/internal/services/user_service.go (1)
UserService(21-26)backend/onboarding/pkg/logger/logger.go (2)
Info(28-40)Error(44-63)backend/onboarding/internal/constants/logs_constants.go (19)
LogHealthCheckRequested(91-91)LogCreateUserRequest(92-92)LogInvalidRequestPayload(111-111)LogFailedToCreateUser(113-113)LogUserCreatedSuccess(102-102)LogGetUserRequest(93-93)LogFailedToGetUser(114-114)LogUserRetrievedSuccess(103-103)LogFailedToGetUsersByRole(115-115)LogUsersRetrievedSuccess(104-104)LogDeleteUserRequest(97-97)LogFailedToDeleteUser(118-118)LogUpdateUserRequest(96-96)LogInvalidRequestBody(112-112)LogUpdateRequestDetails(124-124)LogFailedToUpdateUser(117-117)LogUserUpdatedSuccess(105-105)LogParsedFilters(125-125)LogFailedToGetUsersFilters(116-116)backend/onboarding/internal/constants/errors_constants.go (25)
ServiceStatus(165-165)ServiceName(163-163)ServiceVersion(164-164)ErrMsgInvalidRequestPayload(130-130)ErrMsgFailedToCreateUser(142-142)SuccessMsgUserCreated(154-154)ErrMsgMissingUserID(138-138)ErrMsgMissingUserIDDetail(139-139)SuccessMsgUserRetrieved(155-155)ErrMsgInvalidRole(132-132)ErrMsgInvalidRoleDetail(133-133)ErrMsgFailedToGetUsers(144-144)SuccessMsgUsersRetrieved(156-156)ErrMsgUserNotFound(147-147)ErrMsgFailedToDeleteUser(146-146)ErrMsgInvalidRequestBody(131-131)ErrMsgFailedToUpdateUser(145-145)SuccessMsgUserUpdated(157-157)ErrMsgInvalidIsActive(134-134)ErrMsgInvalidIsActiveDetail(135-135)ErrMsgInvalidPhoneNumber(136-136)ErrMsgInvalidPhoneDetail(137-137)DefaultPageLimit(170-170)DefaultOffset(172-172)MaxPageLimit(171-171)backend/onboarding/pkg/response/response.go (2)
Success(20-27)Error(31-38)backend/onboarding/internal/models/user.go (3)
CreateUserRequest(12-25)UpdateUserRequest(129-138)UserFilters(164-172)backend/onboarding/internal/models/database_models.go (6)
UserRole(12-12)RoleAgent(15-15)RoleCitizen(16-16)RoleCommissioner(17-17)RoleServiceManager(18-18)RoleAdmin(19-19)
backend/onboarding/internal/scheduler/scheduler.go (2)
backend/onboarding/internal/services/keycloak_service.go (1)
KeycloakService(26-41)backend/onboarding/internal/repositories/interfaces.go (1)
UserRepository(9-37)
backend/onboarding/internal/models/database_models.go (1)
backend/onboarding/internal/models/user.go (1)
Address(91-97)
backend/onboarding/internal/routes/routes.go (1)
backend/onboarding/internal/handlers/user_handler.go (1)
UserHandler(21-23)
backend/onboarding/internal/repositories/transaction.go (4)
backend/onboarding/internal/models/database_models.go (2)
User(24-40)UserProfile(44-65)backend/onboarding/internal/constants/logs_constants.go (9)
LogCreateUserInTransaction(80-80)LogUserCreatedInTransaction(81-81)LogCreateUserProfileInTransaction(82-82)LogUserProfileCreatedInTransaction(83-83)LogCommitTransaction(13-13)LogTransactionCommitted(84-84)LogRollbackTransaction(85-85)LogTransactionRolledBack(86-86)LogBeginTransaction(10-10)backend/onboarding/internal/constants/errors_constants.go (4)
ErrCreateUserInTransaction(91-91)ErrCreateUserProfileInTransaction(92-92)ErrCommitTransaction(94-94)ErrRollbackTransaction(93-93)backend/onboarding/internal/repositories/users_repo.go (1)
PostgreSQLUserRepository(16-18)
backend/onboarding/internal/repositories/zone_mapping_repo.go (3)
backend/onboarding/internal/models/database_models.go (2)
ZoneMapping(85-91)ZoneMapping(94-96)backend/onboarding/pkg/logger/logger.go (2)
Info(28-40)Error(44-63)backend/onboarding/pkg/response/response.go (1)
Error(31-38)
backend/onboarding/internal/database/database.go (2)
backend/onboarding/internal/config/config.go (1)
Config(22-57)backend/onboarding/pkg/logger/logger.go (2)
Fatal(82-95)Info(28-40)
backend/onboarding/internal/models/user.go (1)
backend/onboarding/internal/models/database_models.go (5)
UserRole(12-12)AddressDB(69-76)AddressDB(79-81)User(24-40)UserProfile(44-65)
backend/onboarding/internal/repositories/users_repo.go (8)
backend/onboarding/internal/repositories/interfaces.go (1)
UserRepository(9-37)backend/onboarding/internal/models/database_models.go (6)
UserProfile(44-65)User(24-40)UserRole(12-12)AddressDB(69-76)AddressDB(79-81)RoleAgent(15-15)backend/onboarding/pkg/logger/logger.go (3)
Info(28-40)Error(44-63)Warn(67-79)backend/onboarding/internal/constants/logs_constants.go (25)
LogRetrieveUserProfileStart(41-41)LogRetrieveUserProfileSuccess(42-42)LogRetrieveUserStart(30-30)LogRetrieveUserSuccess(35-35)LogGetUsersByRoleStart(27-27)LogGetUsersByRoleSuccess(26-26)LogDeleteUserStart(43-43)LogUserAlreadyDeleted(65-65)LogSoftDeleteUserStart(66-66)ErrSoftDeleteUserFailed(67-67)LogSoftDeleteUserSuccess(68-68)LogRetrieveAddressStart(51-51)LogRetrieveAddressSuccess(52-52)LogCountUsersStart(53-53)LogCountUsersSuccess(54-54)LogUpdateUserStartt(55-55)LogUpdateUserSuccess(22-22)LogUpdateUserProfileStart(56-56)LogCreateNewAddressStart(57-57)LogCreateNewAddressSuccess(58-58)LogUpdateExistingAddressStart(59-59)LogUpdateExistingAddressSuccess(60-60)LogUpdateUserProfileSuccess(61-61)LogGetUsersWithFiltersStartRetrive(62-62)LogGetUsersWithFiltersSuccess(63-63)backend/onboarding/pkg/response/response.go (1)
Error(31-38)backend/onboarding/internal/constants/errors_constants.go (18)
ErrUserProfileNotFound(64-64)ErrRetrieveUserProfileFailed(65-65)ErrUserNotFound(46-46)ErrRetrieveUserFailed(66-66)ErrGetUsersByRole(48-48)ErrNoUsersFound(50-50)ErrCheckUserExistenceFailed(67-67)ErrAddressNotFound(72-72)ErrRetrieveAddressFailed(73-73)ErrCountUsersFailed(74-74)ErrUpdateUserFailed(56-56)ErrUpdateUserProfileFailed(79-79)ErrRetrieveAddressIDFailed(75-75)ErrCreateNewAddressFailed(76-76)ErrUpdateAddressIDFailed(77-77)ErrUpdateExistingAddressFailed(78-78)ErrCountUsersWithFiltersFailed(80-80)ErrRetrieveUsersWithFiltersFailed(81-81)backend/onboarding/internal/errors/errors.go (1)
NewRepositoryError(33-35)backend/onboarding/internal/models/user.go (4)
UpdateUserRequest(129-138)UpdateUserProfile(141-161)Address(91-97)UserFilters(164-172)
backend/onboarding/internal/services/keycloak_service.go (6)
backend/onboarding/internal/repositories/interfaces.go (1)
UserRepository(9-37)backend/onboarding/internal/errors/errors.go (1)
NewKeycloakServiceError(28-30)backend/onboarding/internal/constants/errors_constants.go (22)
ErrDecodeToken(13-13)ErrEmptyUsername(9-9)ErrEmptyEmail(10-10)ErrMarshalUserData(15-15)ErrCreateUserRequest(16-16)ErrCreateUser(17-17)ErrNoLocationHeader(18-18)ErrInvalidLocation(19-19)ErrEmptyUserID(7-7)ErrEmptyRoleName(8-8)ErrGetRole(20-20)ErrMarshalRole(21-21)ErrAssignRoleRequest(22-22)ErrAssignRole(23-23)ErrCreateRoleRequest(28-28)ErrFetchKeycloakUser(59-59)ErrKeycloakAPIStatus(29-29)ErrDecodeKeycloakUser(30-30)ErrGetUserRoles(24-24)ErrDeleteUserRequest(25-25)ErrDeleteUser(26-26)ErrDeleteUserKeycloak(27-27)backend/onboarding/pkg/logger/logger.go (1)
Error(44-63)backend/onboarding/pkg/response/response.go (1)
Error(31-38)backend/onboarding/internal/models/user.go (2)
KeycloakUser(101-111)KeycloakRole(122-126)
🪛 markdownlint-cli2 (0.18.1)
backend/onboarding/README.md
60-60: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
145-145: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
230-230: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
312-312: Hard tabs
Column: 1
(MD010, no-hard-tabs)
313-313: Hard tabs
Column: 1
(MD010, no-hard-tabs)
314-314: Hard tabs
Column: 1
(MD010, no-hard-tabs)
315-315: Hard tabs
Column: 1
(MD010, no-hard-tabs)
317-317: Hard tabs
Column: 1
(MD010, no-hard-tabs)
318-318: Hard tabs
Column: 1
(MD010, no-hard-tabs)
319-319: Hard tabs
Column: 1
(MD010, no-hard-tabs)
321-321: Hard tabs
Column: 1
(MD010, no-hard-tabs)
322-322: Hard tabs
Column: 1
(MD010, no-hard-tabs)
323-323: Hard tabs
Column: 1
(MD010, no-hard-tabs)
324-324: Hard tabs
Column: 1
(MD010, no-hard-tabs)
325-325: Hard tabs
Column: 1
(MD010, no-hard-tabs)
327-327: Hard tabs
Column: 1
(MD010, no-hard-tabs)
328-328: Hard tabs
Column: 1
(MD010, no-hard-tabs)
330-330: Hard tabs
Column: 1
(MD010, no-hard-tabs)
331-331: Hard tabs
Column: 1
(MD010, no-hard-tabs)
332-332: Hard tabs
Column: 1
(MD010, no-hard-tabs)
333-333: Hard tabs
Column: 1
(MD010, no-hard-tabs)
334-334: Hard tabs
Column: 1
(MD010, no-hard-tabs)
336-336: Hard tabs
Column: 1
(MD010, no-hard-tabs)
337-337: Hard tabs
Column: 1
(MD010, no-hard-tabs)
338-338: Hard tabs
Column: 1
(MD010, no-hard-tabs)
339-339: Hard tabs
Column: 1
(MD010, no-hard-tabs)
339-339: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (33)
backend/onboarding/internal/utils/keycloak_utils.go (1)
13-22: LGTM! Proper guard clauses prevent panics.The function correctly validates the attribute exists, is a slice, has elements, and the first element is a string before accessing it.
backend/onboarding/postman/new_onboarding.postman_collection.json (1)
1-349: LGTM! Well-structured API testing collection.The Postman collection provides comprehensive coverage of the user management API endpoints with appropriate placeholder values for testing.
backend/onboarding/internal/errors/errors.go (1)
22-40: LGTM! Domain-specific error constructors provide clear context.The specialized constructors (NewUserServiceError, NewKeycloakServiceError, etc.) make it easy to trace which layer originated an error.
backend/onboarding/go.mod (1)
3-3: Go 1.24.0 is a valid release and does not need to be changed.Go 1.24.0 is a major release of Go, having been released in February 2025. The go.mod file is correct as-is. If a version update is desired for other reasons, the latest stable version is go 1.25.5 (released December 2, 2025), but specifying go 1.24.0 is neither invalid nor problematic for builds.
backend/onboarding/internal/routes/routes.go (1)
16-36: LGTM! Well-structured route definitions.The route configuration is clean, well-documented, and follows best practices. The ordering of routes (with
/countbefore/:id) correctly prevents routing conflicts in Gin.backend/onboarding/migrations/zone_mapping.sql (1)
1-20: LGTM! Well-designed zone mapping schema.The table structure is appropriate with:
- Composite primary key preventing duplicate zone assignments per user
- GIN index optimizing array queries on the
wardcolumn- Cascading delete maintaining referential integrity
backend/onboarding/Dockerfile (1)
2-2: No action required — golang:1.25 is a stable, released version.Go 1.25 was released on August 12, 2025, and Go 1.25.5 was released on December 2, 2025. The Dockerfile's use of
golang:1.25-alpineis valid and references an established, production-ready version.backend/onboarding/cmd/server/main.go (1)
22-36: LGTM! Database connection lifecycle is handled correctly.The database connection is properly established and deferred for cleanup on shutdown. Error handling for getting the underlying SQL DB is appropriate.
backend/onboarding/internal/utils/mdms_utils.go (1)
12-24: LGTM! Well-structured response model.The MDMSResponse struct correctly maps the expected JSON structure from the MDMS API with appropriate field tags.
backend/onboarding/pkg/response/response.go (1)
11-16: LGTM! Clean and consistent API response structure.The
APIResponsestruct provides a well-designed standard format with appropriate JSON tags and omitempty for optional fields.backend/onboarding/internal/config/config.go (2)
15-18: LGTM! Thread-safe singleton pattern.The use of
sync.Onceensures the configuration is loaded exactly once in a thread-safe manner.
100-101: Wildcard CORS default is insecure for production.The default
CORS_ALLOWED_ORIGINSis"*", which allows any origin to make cross-origin requests. While convenient for development, this should be explicitly configured in production environments.Please ensure the deployment configuration sets
CORS_ALLOWED_ORIGINSto specific trusted origins rather than relying on the wildcard default.backend/onboarding/internal/repositories/transaction.go (2)
18-23: LGTM! Clean transaction interface design.The interface provides a clear abstraction for transactional operations with proper separation between entity creation and transaction lifecycle methods.
109-118: LGTM! Transaction initialization with proper context propagation.The
BeginTransactionmethod correctly usesWithContextto propagate the context and properly handles transaction start errors.backend/onboarding/internal/repositories/zone_mapping_repo.go (2)
11-15: LGTM! Clean interface definition.The interface follows Go conventions with context as first parameter and appropriate return types.
40-52: LGTM!The query method correctly propagates context, uses parameterized queries (safe from SQL injection), and handles errors appropriately.
backend/onboarding/internal/handlers/user_handler.go (4)
19-37: LGTM!The handler struct and constructor follow standard dependency injection patterns.
39-51: LGTM!Health check endpoint is straightforward and uses constants consistently.
346-377: LGTM!Pagination parsing handles edge cases (negative, zero, exceeding max) correctly. Role validation covers all defined roles.
392-404: LGTM!User counts endpoint follows consistent patterns with proper context propagation.
backend/onboarding/internal/validator/user_validation_service.go (1)
19-33: LGTM!Clean struct definition with appropriate dependencies for validation operations.
backend/onboarding/internal/models/database_models.go (3)
10-20: LGTM!UserRole type and constants are well-defined with clear naming.
22-40: LGTM!The User struct is well-defined with appropriate GORM tags. The Profile relation correctly establishes a 1:1 mapping using keycloak_user_id.
42-81: LGTM!UserProfile and AddressDB structs are well-defined with appropriate field types and GORM tags.
backend/onboarding/internal/models/user.go (1)
1-8: LGTM!Package declaration and imports are appropriate for the models layer.
backend/onboarding/internal/services/keycloak_service.go (3)
112-123: Response body consumed before decode attempt on success path.On lines 113-114,
io.ReadAll(resp.Body)is called to log the error response. However, if the status check passes, the code on line 119 attempts to decode fromresp.Bodywhich has already been fully read. This will result in an EOF error during decode.The current code structure handles this correctly because the body read only happens inside the
if resp.StatusCode != http.StatusOKblock, but the pattern is fragile. Consider using a more explicit pattern.Please verify the control flow ensures the body is not consumed before decode on the success path.
52-75: LGTM!The constructor properly initializes the HTTP client with a 30-second timeout, which is good practice for external API calls.
86-135: Good use of exponential backoff for token retrieval.The implementation correctly uses
backoff.NewExponentialBackOff()with a 2-minute max elapsed time for resilient token acquisition.backend/onboarding/internal/repositories/users_repo.go (2)
15-23: LGTM!The repository struct and constructor are appropriately implemented with dependency injection of the GORM DB handle.
272-322: LGTM!The
UpdateUserProfileCompletemethod correctly handles the address creation/update logic with proper conditional branching for new vs. existing addresses.backend/onboarding/internal/services/user_service.go (1)
19-45: LGTM!The
UserServicestruct and constructor appropriately inject all required dependencies for coordinating user lifecycle management.backend/onboarding/internal/constants/errors_constants.go (2)
1-31: LGTM!Well-organized Keycloak error constants with consistent naming convention.
151-174: LGTM!Success messages, service info, and pagination constants are well-defined and appropriately organized.
| @@ -0,0 +1,7 @@ | |||
| *.json | |||
| *.md | |||
| /migrations | |||
There was a problem hiding this comment.
Excluding /migrations may break database initialization in containers.
Database migrations are typically needed for schema setup when deploying containerized applications. Excluding /migrations means the SQL files won't be available in the Docker image for initialization tasks.
🔎 Proposed fix
If migrations are applied externally (e.g., via a separate init container or migration tool), this is acceptable. Otherwise, remove this line:
*.json
*.md
-/migrations
.env
*.yaml
*.yml
.gitignore📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /migrations | |
| *.json | |
| *.md | |
| .env | |
| *.yaml | |
| *.yml | |
| .gitignore |
🤖 Prompt for AI Agents
In backend/onboarding/.dockerignore around line 3 the entry "/migrations"
prevents migration files from being copied into the image which can break DB
initialization; if migrations are not applied externally, remove the
"/migrations" line so migration files are included in the build; if migrations
are applied by a separate init container or CI job, replace the line with a
short comment explaining that decision (or adjust to a conditional build stage)
so future maintainers know the exclusion is intentional.
| func (uh *UserHandler) UpdateUser(c *gin.Context) { | ||
| keycloakUserID := c.Param("id") | ||
| logger.Info(constants.LogUpdateUserRequest, "keycloakUserID", keycloakUserID) | ||
|
|
||
| var updateReq models.UpdateUserRequest | ||
| if err := c.ShouldBindJSON(&updateReq); err != nil { | ||
| logger.Error(constants.LogInvalidRequestBody, "error", err) | ||
| response.Error(c, http.StatusBadRequest, constants.ErrMsgInvalidRequestBody, err.Error()) | ||
| return | ||
| } | ||
|
|
||
| logger.Info(constants.LogUpdateRequestDetails, "email", updateReq.Email, "role", updateReq.Role, "preferredLanguage", updateReq.PreferredLanguage) | ||
|
|
||
| updatedUser, err := uh.userService.UpdateUserComplete(keycloakUserID, &updateReq) | ||
| if err != nil { | ||
| logger.Error(constants.LogFailedToUpdateUser, "error", err, "keycloakUserID", keycloakUserID) | ||
| if err.Error() == "user not found" { | ||
| response.Error(c, http.StatusNotFound, constants.ErrMsgUserNotFound, err.Error()) | ||
| return | ||
| } | ||
|
|
||
| response.Error(c, http.StatusBadRequest, constants.ErrMsgFailedToUpdateUser, err.Error()) | ||
| return | ||
| } | ||
|
|
||
| logger.Info(constants.LogUserUpdatedSuccess, "keycloakUserID", keycloakUserID) | ||
| response.Success(c, http.StatusOK, constants.SuccessMsgUserUpdated, updatedUser) | ||
| } |
There was a problem hiding this comment.
UpdateUser has similar issues: missing ID validation, no context, brittle error matching.
Consistent with DeleteUser, this handler should:
- Validate
keycloakUserIDis not empty before processing - Pass
c.Request.Context()toUpdateUserComplete - Replace string comparison
err.Error() == "user not found"with typed error checking
🔎 Suggested fix
func (uh *UserHandler) UpdateUser(c *gin.Context) {
keycloakUserID := c.Param("id")
+ if keycloakUserID == "" {
+ response.Error(c, http.StatusBadRequest, constants.ErrMsgMissingUserID, constants.ErrMsgMissingUserIDDetail)
+ return
+ }
logger.Info(constants.LogUpdateUserRequest, "keycloakUserID", keycloakUserID)
var updateReq models.UpdateUserRequest
if err := c.ShouldBindJSON(&updateReq); err != nil {
logger.Error(constants.LogInvalidRequestBody, "error", err)
response.Error(c, http.StatusBadRequest, constants.ErrMsgInvalidRequestBody, err.Error())
return
}
logger.Info(constants.LogUpdateRequestDetails, "email", updateReq.Email, "role", updateReq.Role, "preferredLanguage", updateReq.PreferredLanguage)
- updatedUser, err := uh.userService.UpdateUserComplete(keycloakUserID, &updateReq)
+ updatedUser, err := uh.userService.UpdateUserComplete(c.Request.Context(), keycloakUserID, &updateReq)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
backend/onboarding/internal/handlers/user_handler.go lines 220-247: The
UpdateUser handler currently omits ID validation, does not pass the request
context into the service call, and uses brittle string equality to detect a
not-found error; update it to (1) validate keycloakUserID is non-empty and
return 400 if missing, (2) call uh.userService.UpdateUserComplete with
c.Request.Context() instead of no context, and (3) replace err.Error() == "user
not found" with a typed check (e.g., errors.Is(err, service.ErrUserNotFound) or
compare to a package-level sentinel error returned by the user service) and
adjust the response accordingly; also include keycloakUserID in error logs for
context.
| if len(config.AllowedOrigins) == 1 && config.AllowedOrigins[0] == "*" { | ||
| c.Header("Access-Control-Allow-Origin", "*") |
There was a problem hiding this comment.
Critical: Wildcard origin with credentials violates CORS specification.
The CORS specification explicitly forbids using Access-Control-Allow-Origin: * together with Access-Control-Allow-Credentials: true. Browsers will reject this configuration.
If config.AllowedOrigins contains only "*" (lines 37-38) and config.AllowCredentials is true (lines 71-72), both headers will be set, causing a CORS policy violation.
🔎 Proposed fix
Add validation to prevent this invalid configuration:
if allowed {
// Allow all origins or echo back the request origin
if len(config.AllowedOrigins) == 1 && config.AllowedOrigins[0] == "*" {
+ if config.AllowCredentials {
+ // Cannot use wildcard with credentials - echo back origin instead
+ c.Header("Access-Control-Allow-Origin", origin)
+ } else {
c.Header("Access-Control-Allow-Origin", "*")
+ }
} else {
c.Header("Access-Control-Allow-Origin", origin)
}Alternatively, validate the configuration at middleware initialization:
func CORSWithConfig(config CORSConfig) gin.HandlerFunc {
// Validate configuration
if config.AllowCredentials {
for _, origin := range config.AllowedOrigins {
if origin == "*" {
panic("CORS: cannot use wildcard origin (*) with AllowCredentials=true")
}
}
}
return func(c *gin.Context) {
// ... rest of the middleware
}
}Also applies to: 71-72
🤖 Prompt for AI Agents
In backend/onboarding/internal/middleware/cors.go around lines 37-38 and 71-72,
the middleware sets Access-Control-Allow-Origin: "*" while also allowing
credentials which violates the CORS spec; update the code to validate the
CORSConfig at middleware initialization and reject or sanitize invalid combos:
if config.AllowCredentials is true, ensure no entry in config.AllowedOrigins is
"*" (panic or return an error) and/or remove the wildcard branch so the handler
always sets a concrete origin from the request when credentials are allowed;
make this validation before returning the gin.HandlerFunc so the server fails
fast on invalid config.
| // Set allowed HTTP methods for CORS | ||
| methods := "GET, POST, PUT, DELETE, PATCH, OPTIONS" | ||
| if len(config.AllowedMethods) > 0 { | ||
| methods = "" | ||
| for i, method := range config.AllowedMethods { | ||
| if i > 0 { | ||
| methods += ", " | ||
| } | ||
| methods += method | ||
| } | ||
| } | ||
| c.Header("Access-Control-Allow-Methods", methods) | ||
|
|
||
| // Set allowed HTTP headers for CORS | ||
| headers := "Origin, Content-Type, Accept, Authorization, X-Requested-With, X-Tenant-ID, X-User-ID" | ||
| if len(config.AllowedHeaders) > 0 { | ||
| headers = "" | ||
| for i, header := range config.AllowedHeaders { | ||
| if i > 0 { | ||
| headers += ", " | ||
| } | ||
| headers += header | ||
| } | ||
| } | ||
| c.Header("Access-Control-Allow-Headers", headers) | ||
|
|
||
| // Set whether credentials (cookies, auth) are allowed | ||
| if config.AllowCredentials { | ||
| c.Header("Access-Control-Allow-Credentials", "true") | ||
| } | ||
|
|
||
| // Set how long the results of a preflight request can be cached | ||
| maxAge := 86400 // 24 hours default | ||
| if config.MaxAge > 0 { | ||
| maxAge = config.MaxAge | ||
| } | ||
| c.Header("Access-Control-Max-Age", strconv.Itoa(maxAge)) |
There was a problem hiding this comment.
CORS headers are set regardless of origin validation.
Lines 44-80 set Access-Control-Allow-Methods, Access-Control-Allow-Headers, Access-Control-Allow-Credentials, and Access-Control-Max-Age unconditionally, even when the origin is not in the allowed list (determined by lines 26-33).
Per CORS best practices, all CORS headers should only be set when the origin is explicitly allowed. The current implementation only guards Access-Control-Allow-Origin (line 35) but not the other headers.
🔎 Proposed fix
}
if allowed {
// Allow all origins or echo back the request origin
if len(config.AllowedOrigins) == 1 && config.AllowedOrigins[0] == "*" {
c.Header("Access-Control-Allow-Origin", "*")
} else {
c.Header("Access-Control-Allow-Origin", origin)
}
- }
- // Set allowed HTTP methods for CORS
- methods := "GET, POST, PUT, DELETE, PATCH, OPTIONS"
- if len(config.AllowedMethods) > 0 {
- methods = ""
- for i, method := range config.AllowedMethods {
- if i > 0 {
- methods += ", "
- }
- methods += method
- }
- }
- c.Header("Access-Control-Allow-Methods", methods)
+ // Set allowed HTTP methods for CORS
+ methods := "GET, POST, PUT, DELETE, PATCH, OPTIONS"
+ if len(config.AllowedMethods) > 0 {
+ methods = ""
+ for i, method := range config.AllowedMethods {
+ if i > 0 {
+ methods += ", "
+ }
+ methods += method
+ }
+ }
+ c.Header("Access-Control-Allow-Methods", methods)
- // Set allowed HTTP headers for CORS
- headers := "Origin, Content-Type, Accept, Authorization, X-Requested-With, X-Tenant-ID, X-User-ID"
- if len(config.AllowedHeaders) > 0 {
- headers = ""
- for i, header := range config.AllowedHeaders {
- if i > 0 {
- headers += ", "
- }
- headers += header
- }
- }
- c.Header("Access-Control-Allow-Headers", headers)
+ // Set allowed HTTP headers for CORS
+ headers := "Origin, Content-Type, Accept, Authorization, X-Requested-With, X-Tenant-ID, X-User-ID"
+ if len(config.AllowedHeaders) > 0 {
+ headers = ""
+ for i, header := range config.AllowedHeaders {
+ if i > 0 {
+ headers += ", "
+ }
+ headers += header
+ }
+ }
+ c.Header("Access-Control-Allow-Headers", headers)
- // Set whether credentials (cookies, auth) are allowed
- if config.AllowCredentials {
- c.Header("Access-Control-Allow-Credentials", "true")
- }
+ // Set whether credentials (cookies, auth) are allowed
+ if config.AllowCredentials {
+ c.Header("Access-Control-Allow-Credentials", "true")
+ }
- // Set how long the results of a preflight request can be cached
- maxAge := 86400 // 24 hours default
- if config.MaxAge > 0 {
- maxAge = config.MaxAge
+ // Set how long the results of a preflight request can be cached
+ maxAge := 86400 // 24 hours default
+ if config.MaxAge > 0 {
+ maxAge = config.MaxAge
+ }
+ c.Header("Access-Control-Max-Age", strconv.Itoa(maxAge))
}
- c.Header("Access-Control-Max-Age", strconv.Itoa(maxAge))
// Handle preflight OPTIONS request by returning 204 No Content
if c.Request.Method == "OPTIONS" {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Set allowed HTTP methods for CORS | |
| methods := "GET, POST, PUT, DELETE, PATCH, OPTIONS" | |
| if len(config.AllowedMethods) > 0 { | |
| methods = "" | |
| for i, method := range config.AllowedMethods { | |
| if i > 0 { | |
| methods += ", " | |
| } | |
| methods += method | |
| } | |
| } | |
| c.Header("Access-Control-Allow-Methods", methods) | |
| // Set allowed HTTP headers for CORS | |
| headers := "Origin, Content-Type, Accept, Authorization, X-Requested-With, X-Tenant-ID, X-User-ID" | |
| if len(config.AllowedHeaders) > 0 { | |
| headers = "" | |
| for i, header := range config.AllowedHeaders { | |
| if i > 0 { | |
| headers += ", " | |
| } | |
| headers += header | |
| } | |
| } | |
| c.Header("Access-Control-Allow-Headers", headers) | |
| // Set whether credentials (cookies, auth) are allowed | |
| if config.AllowCredentials { | |
| c.Header("Access-Control-Allow-Credentials", "true") | |
| } | |
| // Set how long the results of a preflight request can be cached | |
| maxAge := 86400 // 24 hours default | |
| if config.MaxAge > 0 { | |
| maxAge = config.MaxAge | |
| } | |
| c.Header("Access-Control-Max-Age", strconv.Itoa(maxAge)) | |
| if allowed { | |
| // Allow all origins or echo back the request origin | |
| if len(config.AllowedOrigins) == 1 && config.AllowedOrigins[0] == "*" { | |
| c.Header("Access-Control-Allow-Origin", "*") | |
| } else { | |
| c.Header("Access-Control-Allow-Origin", origin) | |
| } | |
| // Set allowed HTTP methods for CORS | |
| methods := "GET, POST, PUT, DELETE, PATCH, OPTIONS" | |
| if len(config.AllowedMethods) > 0 { | |
| methods = "" | |
| for i, method := range config.AllowedMethods { | |
| if i > 0 { | |
| methods += ", " | |
| } | |
| methods += method | |
| } | |
| } | |
| c.Header("Access-Control-Allow-Methods", methods) | |
| // Set allowed HTTP headers for CORS | |
| headers := "Origin, Content-Type, Accept, Authorization, X-Requested-With, X-Tenant-ID, X-User-ID" | |
| if len(config.AllowedHeaders) > 0 { | |
| headers = "" | |
| for i, header := range config.AllowedHeaders { | |
| if i > 0 { | |
| headers += ", " | |
| } | |
| headers += header | |
| } | |
| } | |
| c.Header("Access-Control-Allow-Headers", headers) | |
| // Set whether credentials (cookies, auth) are allowed | |
| if config.AllowCredentials { | |
| c.Header("Access-Control-Allow-Credentials", "true") | |
| } | |
| // Set how long the results of a preflight request can be cached | |
| maxAge := 86400 // 24 hours default | |
| if config.MaxAge > 0 { | |
| maxAge = config.MaxAge | |
| } | |
| c.Header("Access-Control-Max-Age", strconv.Itoa(maxAge)) | |
| } |
🤖 Prompt for AI Agents
In backend/onboarding/internal/middleware/cors.go around lines 44 to 80, the
CORS response headers (Allow-Methods, Allow-Headers, Allow-Credentials, Max-Age)
are set unconditionally; modify the code so these headers are only set when the
request origin has already been validated as allowed (the boolean established in
the origin check at lines ~26-33). Move or wrap the blocks that set methods,
headers, credentials and max-age inside the same conditional that sets
Access-Control-Allow-Origin (or check the same originAllowed flag) so that
preflight and regular responses only emit full CORS headers for allowed origins;
keep existing defaults for building comma-separated lists when config lists are
present and ensure OPTIONS handling still returns the headers only when origin
is allowed.
| // ZoneMapping stores mapping of user to zones and wards. | ||
| // Used for zone/ward assignment and filtering. | ||
| type ZoneMapping struct { | ||
| UserID string `gorm:"column:user_id;type:varchar(255);not null" json:"user_id"` | ||
| Zone string `gorm:"column:zone;type:varchar(50)" json:"zone"` | ||
| Wards pq.StringArray `gorm:"column:ward;type:text[]" json:"wards"` | ||
| CreatedAt time.Time `gorm:"column:created_at;autoCreateTime" json:"created_at"` | ||
| UpdatedAt time.Time `gorm:"column:updated_at;autoUpdateTime" json:"updated_at"` | ||
| } | ||
|
|
||
| // TableName overrides the default table name for ZoneMapping. | ||
| func (ZoneMapping) TableName() string { | ||
| return "DIGIT3.zone_mapping" | ||
| } |
There was a problem hiding this comment.
ZoneMapping lacks a primary key, which may cause GORM issues.
GORM expects a primary key for proper record identification. Consider adding a composite primary key on (user_id, zone) or an auto-generated ID field.
🔎 Suggested fix with composite primary key
type ZoneMapping struct {
- UserID string `gorm:"column:user_id;type:varchar(255);not null" json:"user_id"`
- Zone string `gorm:"column:zone;type:varchar(50)" json:"zone"`
+ UserID string `gorm:"column:user_id;type:varchar(255);not null;primaryKey" json:"user_id"`
+ Zone string `gorm:"column:zone;type:varchar(50);primaryKey" json:"zone"`
Wards pq.StringArray `gorm:"column:ward;type:text[]" json:"wards"`
CreatedAt time.Time `gorm:"column:created_at;autoCreateTime" json:"created_at"`
UpdatedAt time.Time `gorm:"column:updated_at;autoUpdateTime" json:"updated_at"`
}🤖 Prompt for AI Agents
In backend/onboarding/internal/models/database_models.go around lines 83 to 96,
the ZoneMapping struct has no primary key which can break GORM operations; add a
primary key by either (A) making a composite primary key of user_id and zone
(mark both fields with gorm:"primaryKey" in their struct tags) or (B) add a
single auto-generated ID field (e.g., ID as primaryKey with appropriate type and
default generation tag) and keep other fields unchanged; update any DB
migrations or table definition to reflect the chosen primary key and run
migrations to apply the change.
| if req.Role != models.RoleCitizen { | ||
| if req.Password == "" { | ||
| return fmt.Errorf(constants.ErrPasswordRequiredForNonCitizens) | ||
| } | ||
| if len(req.Password) < 4 { | ||
| return fmt.Errorf(constants.ErrInvalidPassword) | ||
| } | ||
| } |
There was a problem hiding this comment.
Password policy is too weak for a government/tax system.
A 4-character minimum password is insufficient for security. Consider requiring at least 8-12 characters with complexity requirements (uppercase, lowercase, numbers, special characters).
🤖 Prompt for AI Agents
In backend/onboarding/internal/validator/user_validation_service.go around lines
53 to 60, the current password check allows only a 4-character password for
non-citizens; update this to enforce a stronger policy: require a minimum of 8
(or configurable 8-12) characters and validate complexity (at least one
uppercase, one lowercase, one digit, and one special character). Replace the
simple length check with a regex or dedicated validation helper that returns the
appropriate error constant (or add new error constants) when the password is
missing, too short, or fails complexity, and ensure tests are updated to cover
the stronger policy.
| func (v *UserValidationService) CheckAdhaarExists(adhaarNo int64) (bool, error) { | ||
| logger.Info(constants.LogCheckAdhaarExistsStart, "adhaarNo", adhaarNo) | ||
|
|
||
| var count int64 | ||
| if err := v.db.Model(&models.UserProfile{}).Where("adhaar_no = ?", adhaarNo).Count(&count).Error; err != nil { | ||
| logger.Error(constants.ErrCheckAdhaarExistsFailed, "error", err, "adhaarNo", adhaarNo) | ||
| return false, errors.NewRepositoryError(fmt.Sprintf("%s: %v", constants.ErrCheckAdhaarExistsFailed, err)) | ||
| } | ||
|
|
||
| exists := count > 0 | ||
| logger.Info(constants.LogCheckAdhaarExistsSuccess, "adhaarNo", adhaarNo, "exists", exists) | ||
| return exists, nil | ||
| } |
There was a problem hiding this comment.
Avoid logging Aadhaar numbers in plain text - major privacy/compliance risk.
Aadhaar is highly sensitive PII (India's national ID). Logging it violates data protection principles and may breach UIDAI regulations. Mask or omit Aadhaar from logs entirely.
🔎 Suggested fix
func (v *UserValidationService) CheckAdhaarExists(adhaarNo int64) (bool, error) {
- logger.Info(constants.LogCheckAdhaarExistsStart, "adhaarNo", adhaarNo)
+ // Do not log Aadhaar number - sensitive PII
+ logger.Info(constants.LogCheckAdhaarExistsStart)
var count int64
if err := v.db.Model(&models.UserProfile{}).Where("adhaar_no = ?", adhaarNo).Count(&count).Error; err != nil {
- logger.Error(constants.ErrCheckAdhaarExistsFailed, "error", err, "adhaarNo", adhaarNo)
+ logger.Error(constants.ErrCheckAdhaarExistsFailed, "error", err)
return false, errors.NewRepositoryError(fmt.Sprintf("%s: %v", constants.ErrCheckAdhaarExistsFailed, err))
}
exists := count > 0
- logger.Info(constants.LogCheckAdhaarExistsSuccess, "adhaarNo", adhaarNo, "exists", exists)
+ logger.Info(constants.LogCheckAdhaarExistsSuccess, "exists", exists)
return exists, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (v *UserValidationService) CheckAdhaarExists(adhaarNo int64) (bool, error) { | |
| logger.Info(constants.LogCheckAdhaarExistsStart, "adhaarNo", adhaarNo) | |
| var count int64 | |
| if err := v.db.Model(&models.UserProfile{}).Where("adhaar_no = ?", adhaarNo).Count(&count).Error; err != nil { | |
| logger.Error(constants.ErrCheckAdhaarExistsFailed, "error", err, "adhaarNo", adhaarNo) | |
| return false, errors.NewRepositoryError(fmt.Sprintf("%s: %v", constants.ErrCheckAdhaarExistsFailed, err)) | |
| } | |
| exists := count > 0 | |
| logger.Info(constants.LogCheckAdhaarExistsSuccess, "adhaarNo", adhaarNo, "exists", exists) | |
| return exists, nil | |
| } | |
| func (v *UserValidationService) CheckAdhaarExists(adhaarNo int64) (bool, error) { | |
| // Do not log Aadhaar number - sensitive PII | |
| logger.Info(constants.LogCheckAdhaarExistsStart) | |
| var count int64 | |
| if err := v.db.Model(&models.UserProfile{}).Where("adhaar_no = ?", adhaarNo).Count(&count).Error; err != nil { | |
| logger.Error(constants.ErrCheckAdhaarExistsFailed, "error", err) | |
| return false, errors.NewRepositoryError(fmt.Sprintf("%s: %v", constants.ErrCheckAdhaarExistsFailed, err)) | |
| } | |
| exists := count > 0 | |
| logger.Info(constants.LogCheckAdhaarExistsSuccess, "exists", exists) | |
| return exists, nil | |
| } |
| func Info(message string, keyvals ...interface{}) { | ||
| if infoLogger == nil { | ||
| InitLogger() | ||
| } | ||
|
|
||
| logMessage := message | ||
| for i := 0; i < len(keyvals); i += 2 { | ||
| if i+1 < len(keyvals) { | ||
| logMessage += " " + keyvals[i].(string) + "=" + toString(keyvals[i+1]) | ||
| } | ||
| } | ||
| infoLogger.Println(logMessage) | ||
| } |
There was a problem hiding this comment.
Type assertion on keys can panic.
Line 36 assumes all keys are strings with keyvals[i].(string). If a caller passes a non-string key, this will panic at runtime.
🔎 Proposed fix
Use safe type assertion:
logMessage := message
for i := 0; i < len(keyvals); i += 2 {
if i+1 < len(keyvals) {
- logMessage += " " + keyvals[i].(string) + "=" + toString(keyvals[i+1])
+ if key, ok := keyvals[i].(string); ok {
+ logMessage += " " + key + "=" + toString(keyvals[i+1])
+ }
}
}
infoLogger.Println(logMessage)Apply the same pattern to Error, Warn, and Fatal functions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func Info(message string, keyvals ...interface{}) { | |
| if infoLogger == nil { | |
| InitLogger() | |
| } | |
| logMessage := message | |
| for i := 0; i < len(keyvals); i += 2 { | |
| if i+1 < len(keyvals) { | |
| logMessage += " " + keyvals[i].(string) + "=" + toString(keyvals[i+1]) | |
| } | |
| } | |
| infoLogger.Println(logMessage) | |
| } | |
| func Info(message string, keyvals ...interface{}) { | |
| if infoLogger == nil { | |
| InitLogger() | |
| } | |
| logMessage := message | |
| for i := 0; i < len(keyvals); i += 2 { | |
| if i+1 < len(keyvals) { | |
| if key, ok := keyvals[i].(string); ok { | |
| logMessage += " " + key + "=" + toString(keyvals[i+1]) | |
| } | |
| } | |
| } | |
| infoLogger.Println(logMessage) | |
| } |
🤖 Prompt for AI Agents
In backend/onboarding/pkg/logger/logger.go around lines 28 to 40, the Info
function currently does an unchecked type assertion for keys
(keyvals[i].(string)) which can panic if a non-string key is passed; update the
loop to use a safe assertion: for each key at keyvals[i] try a string assertion
(k, ok := keyvals[i].(string)) and if ok use k, otherwise convert the key to a
string via fmt.Sprint(keyvals[i]); build the logMessage using that safe key
string and toString for the value; apply the identical safe-key pattern to
Error, Warn, and Fatal functions so none of them can panic on non-string keys.
| func Fatal(message string, keyvals ...interface{}) { | ||
| if fatalLogger == nil { | ||
| InitLogger() | ||
| } | ||
|
|
||
| logMessage := message | ||
| for i := 0; i < len(keyvals); i += 2 { | ||
| if i+1 < len(keyvals) { | ||
| logMessage += " " + keyvals[i].(string) + "=" + toString(keyvals[i+1]) | ||
| } | ||
| } | ||
| fatalLogger.Println(logMessage) | ||
| os.Exit(1) // Terminate the application | ||
| } |
There was a problem hiding this comment.
Fatal prevents deferred cleanup and complicates testing.
Calling os.Exit(1) immediately terminates the process without running deferred functions or allowing graceful shutdown. This also makes the Fatal function difficult to test.
Consider alternatives:
- Log the fatal error and return an error to the caller
- Use
panic()which allows recovery in tests - Provide a configurable exit function for testing
🤖 Prompt for AI Agents
In backend/onboarding/pkg/logger/logger.go around lines 82-95, the Fatal
function currently calls os.Exit(1) which prevents deferred cleanup and hinders
testing; replace the direct os.Exit call with a package-level configurable exit
function (e.g., var ExitFunc = os.Exit) and invoke ExitFunc(1) instead so tests
can override it, or alternatively have Fatal return an error or call panic()
depending on caller needs; ensure InitLogger remains called, logMessage is
printed, and update any callers/tests to override ExitFunc in tests to avoid
terminating the process.
| ## 4. Authentication & Authorization | ||
|
|
||
| This onboarding service does not require authentication or authorization. All API endpoints are accessible without tokens or credentials. No JWT, OAuth, or other authentication mechanisms are enforced for any endpoint. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n backend/onboarding/README.md | head -n 160Repository: egovernments/Infosys
Length of output: 8966
🏁 Script executed:
cat -n backend/onboarding/README.md | sed -n '30,45p;130,160p'Repository: egovernments/Infosys
Length of output: 2336
🏁 Script executed:
wc -l backend/onboarding/README.md && sed -n '300,320p' backend/onboarding/README.mdRepository: egovernments/Infosys
Length of output: 590
🏁 Script executed:
find . -name "*.go" -type f | xargs grep -l "middleware\|auth\|Keycloak" | head -20Repository: egovernments/Infosys
Length of output: 913
🏁 Script executed:
cat -n backend/onboarding/internal/middleware/cors.goRepository: egovernments/Infosys
Length of output: 3401
🏁 Script executed:
cat -n backend/onboarding/cmd/server/main.go | head -100Repository: egovernments/Infosys
Length of output: 3837
Fix authentication documentation contradiction and Go version inconsistency.
The README contains a critical internal contradiction: the architecture diagram (line 62) indicates the API Gateway "Handles Auth/AuthZ," but the Authentication & Authorization section (line 140) explicitly states the service requires no authentication or authorization.
Additionally, the Go version requirement is inconsistent: line 33 specifies "Go 1.21+" while line 306 (Getting Started prerequisites) requires "Go 1.24 or higher."
Clarify the authentication model: if the service is intended to be deployed exclusively behind an authenticated API Gateway, document this explicitly as a deployment constraint and remove the statement claiming no authentication is enforced. If authentication should be implemented at the service level, update the documentation and add appropriate middleware. Reconcile the Go version requirements to use a single minimum version across the document.
🤖 Prompt for AI Agents
backend/onboarding/README.md lines ~62, 33, 138-140, and 306: the README
currently contradicts itself by saying the API Gateway "Handles Auth/AuthZ"
(line ~62) while the Authentication & Authorization section (lines 138-140)
claims the service enforces no auth, and Go version is inconsistent between "Go
1.21+" (line 33) and "Go 1.24 or higher" (line 306). Fix by removing the blanket
"no authentication enforced" statement and replacing it with explicit deployment
guidance: state that the service is intended to run behind an authenticated API
Gateway which handles Auth/AuthZ (or, if you intend service-level auth, state
that and add instructions to enable middleware), and document the remaining
endpoints as relying on gateway-enforced auth as a deployment constraint;
finally reconcile the Go version to a single minimum (pick one, e.g., "Go 1.24
or higher") and update all version references to match.
2 participants
The Property Tax Onboarding Service is a Go-based microservice designed to streamline user onboarding and profile management within the property tax system. Its core features include complete user profile CRUD operations, advanced filtering, zone and ward mapping, and seamless integration with Keycloak for user creation and role assignment. The service exposes robust RESTful APIs, supports efficient pagination, and ensures reliable data persistence in PostgreSQL. By providing a secure, scalable, and maintainable platform for managing user lifecycles, it enables efficient administration and enhances the overall property tax management process.
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.