starred/pocket-id
1
0
Fork 0
mirror of https://github.com/pocket-id/pocket-id.git synced 2025-12-09 17:23:22 +03:00
Code Issues 121 Packages Projects Releases 16 Wiki Activity

[PR #423] [MERGED] refactor: simplify app_config service and fix race conditions #807

New Issue
Closed
opened 2025-10-07 00:22:44 +03:00 by OVERLORD · 0 comments
OVERLORD commented 2025-10-07 00:22:44 +03:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/pocket-id/pocket-id/pull/423
Author: @ItalyPaleAle
Created: 4/9/2025
Status: ✅ Merged
Merged: 4/10/2025
Merged by: @stonith404

Base: main ← Head: config-refactor

📝 Commits (10+)

📊 Changes

28 files changed (+1241 additions, -538 deletions)

View changed files

📝 .github/workflows/backend-linter.yml (+1 -1)
📝 Dockerfile (+1 -1)
📝 backend/go.mod (+1 -1)
📝 backend/internal/bootstrap/bootstrap.go (+1 -0)
📝 backend/internal/controller/app_config_controller.go (+14 -22)
📝 backend/internal/controller/e2etest_controller.go (+1 -1)
📝 backend/internal/controller/user_controller.go (+3 -3)
📝 backend/internal/controller/webauthn_controller.go (+1 -1)
📝 backend/internal/dto/app_config_dto.go (+1 -1)
📝 backend/internal/job/ldap_job.go (+1 -1)
📝 backend/internal/model/app_config.go (+152 -40)
📝 backend/internal/model/app_config_test.go (+71 -2)
📝 backend/internal/service/app_config_service.go (+308 -355)
➕ backend/internal/service/app_config_service_test.go (+561 -0)
📝 backend/internal/service/audit_log_service.go (+1 -1)
📝 backend/internal/service/e2etest_service.go (+5 -9)
📝 backend/internal/service/email_service.go (+17 -13)
📝 backend/internal/service/jwt_service.go (+1 -1)
📝 backend/internal/service/jwt_service_test.go (+18 -30)
📝 backend/internal/service/ldap_service.go (+51 -46)

...and 8 more files

📄 Description

Fixes #391
Fixes #407

As discussed with @stonith404 , this PR refactors the app_config service with the goals of greatly simplify the logic, and fixing race conditions.

There are significant performance improvements too, especially when updating config, as the number of database round-trips has been reduced to no more than 2 (currently, it's 2 per each key). API calls to retrieve config are also much faster as they return the data from memory only.

  1. Stores the config object in-memory in an atomic.Pointer, which removes the risk of data races.
  2. Ensures consistency when updating config in the database, by using a transaction and acquiring a table-level lock. This ensures that config is only written by a single goroutine.
  3. Simplifies the data structures a lot. In particular, default values and attributes (such as marking fields as non-public) can are not stored in the database anymore, but only in the code. This reduces the complexity of keeping all data in-sync, since it wasn't necessary to have that info int eh DB.

PR includes extensive unit tests which use an in-memory SQLite database to validate the behavior of the service.

Note: I had to update to Go 1.24 to be able to use testing.Context().

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/pocket-id/pocket-id/pull/423 **Author:** [@ItalyPaleAle](https://github.com/ItalyPaleAle) **Created:** 4/9/2025 **Status:** ✅ Merged **Merged:** 4/10/2025 **Merged by:** [@stonith404](https://github.com/stonith404) **Base:** `main` ← **Head:** `config-refactor` --- ### 📝 Commits (10+) - [`81a9643`](https://github.com/pocket-id/pocket-id/commit/81a9643b18c8d3f198aff47ad965406488ea4323) WIP - [`314e3b0`](https://github.com/pocket-id/pocket-id/commit/314e3b0e5747020819445cdd0aab48456869377c) More WIP - [`c3bb9a8`](https://github.com/pocket-id/pocket-id/commit/c3bb9a8572f2edaf107ff88927e325c27c96e4b0) More WIP - [`2ed730a`](https://github.com/pocket-id/pocket-id/commit/2ed730a9ea748596d029f9dec66693091a768042) More WIP - [`2746f7b`](https://github.com/pocket-id/pocket-id/commit/2746f7bc5a8299235fbc57ce30c26a43468b1c55) More WIP - [`39d3116`](https://github.com/pocket-id/pocket-id/commit/39d3116c2ed4bfcca29819ca10c28be1cc0f8fef) Code compiles - [`d5c3d7b`](https://github.com/pocket-id/pocket-id/commit/d5c3d7bad993d03ce6f412d1cefd24ca5dc235bc) Initial unit tests for app_config_service - [`2992839`](https://github.com/pocket-id/pocket-id/commit/29928390898c5c9538634dcddff12554739906d8) More tests - [`87e258c`](https://github.com/pocket-id/pocket-id/commit/87e258c1813e44caf6bfec98268d7aecd11fa52f) More tests - [`f96b390`](https://github.com/pocket-id/pocket-id/commit/f96b390d5e46648cc8556491d8e1665ea1613a64) Load config from env when UI config is disabled ### 📊 Changes **28 files changed** (+1241 additions, -538 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/backend-linter.yml` (+1 -1) 📝 `Dockerfile` (+1 -1) 📝 `backend/go.mod` (+1 -1) 📝 `backend/internal/bootstrap/bootstrap.go` (+1 -0) 📝 `backend/internal/controller/app_config_controller.go` (+14 -22) 📝 `backend/internal/controller/e2etest_controller.go` (+1 -1) 📝 `backend/internal/controller/user_controller.go` (+3 -3) 📝 `backend/internal/controller/webauthn_controller.go` (+1 -1) 📝 `backend/internal/dto/app_config_dto.go` (+1 -1) 📝 `backend/internal/job/ldap_job.go` (+1 -1) 📝 `backend/internal/model/app_config.go` (+152 -40) 📝 `backend/internal/model/app_config_test.go` (+71 -2) 📝 `backend/internal/service/app_config_service.go` (+308 -355) ➕ `backend/internal/service/app_config_service_test.go` (+561 -0) 📝 `backend/internal/service/audit_log_service.go` (+1 -1) 📝 `backend/internal/service/e2etest_service.go` (+5 -9) 📝 `backend/internal/service/email_service.go` (+17 -13) 📝 `backend/internal/service/jwt_service.go` (+1 -1) 📝 `backend/internal/service/jwt_service_test.go` (+18 -30) 📝 `backend/internal/service/ldap_service.go` (+51 -46) _...and 8 more files_ </details> ### 📄 Description Fixes #391 Fixes #407 As discussed with @stonith404 , this PR refactors the app_config service with the goals of greatly simplify the logic, and fixing race conditions. There are significant performance improvements too, especially when updating config, as the number of database round-trips has been reduced to no more than 2 (currently, it's 2 per each key). API calls to retrieve config are also much faster as they return the data from memory only. 1. Stores the config object in-memory in an `atomic.Pointer`, which removes the risk of data races. 2. Ensures consistency when updating config in the database, by using a transaction and acquiring a table-level lock. This ensures that config is only written by a single goroutine. 3. Simplifies the data structures a lot. In particular, default values and attributes (such as marking fields as non-public) can are not stored in the database anymore, but only in the code. This reduces the complexity of keeping all data in-sync, since it wasn't necessary to have that info int eh DB. PR includes extensive unit tests which use an in-memory SQLite database to validate the behavior of the service. Note: I had to update to Go 1.24 to be able to use `testing.Context()`. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
OVERLORD added the pull-request label 2025-10-07 00:22:44 +03:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
breaking
bug
documentation
feature
needs more upvotes
open to pull requests
pull-request
Mirrored from GitHub Pull Request
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
OVERLORD
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/pocket-id#807
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?