diff --git a/.scratch/architecture-review-20260808.html b/.scratch/architecture-review-20260808.html new file mode 100644 index 0000000..eae14df --- /dev/null +++ b/.scratch/architecture-review-20260808.html @@ -0,0 +1,329 @@ + + + + + Architecture review — crm-backend-matt + + + + + +
+ +
+
+

Architecture Review

+ crm-backend-matt · 2026-08-08 +
+
+ module + shallow + deep + ⊡ dashed = seam + — red = leakage +
+

Three deepening opportunities across the crm-file, crm-dict, and crm-auth modules. Vocabulary from /codebase-design; domain terms from CONTEXT.md.

+
+ +
+ + +
+
+
+

1. Thumbnail lifecycle split across two modules

+
+ Strong + in-process +
+
+ +
+

Files

+
    +
  • crm-file/service/api/FileApi.java — 11 methods, includes getThumbnail()
    • +
  • crm-file/service/impl/FileApiImpl.java — 551 lines; ~100 lines are thumbnail read/fallback
    • +
  • crm-file/task/ThumbnailGenerationTask.java — generation + lock, but no retrieval
    • +
  • crm-file/service/ThumbnailPlaceholderService.java, ThumbnailRenderer.java
    • +
+
+ +
+
+
Before
+
+
+
FileApiImpl (551 lines)
+
upload · download · preview · multipart · getThumbnail · handlePending · readThumbnail · placeholderOf
+
↑ thumbnail read path leaks here
+
+
⊡⊡ seam (not a real one) ⊡⊡
+
+
ThumbnailGenerationTask
+
assignInitialStatus · generate · tryGenerateSync · generateSync · findRenderer
+
generation only — half the lifecycle
+
+
+

No locality: thumbnail bugs span two modules

+
+ +
+
After
+
+
+
FileApi (8 methods)
+
upload · download · preview · multipart
+
file ops only — interface narrows
+
+
⊡⊡ seam ⊡⊡
+
+
ThumbnailService (deep)
+
interface: getThumbnail · assignInitialStatus · generate
+
implementation: handlePending · poll · readThumbnail · generateSync · findRenderer · placeholderOf
+
+
+

Locality: full lifecycle in one module

+
+
+ +
+

Problem: The thumbnail capability has no single module — generation lives in ThumbnailGenerationTask, retrieval and sync-fallback (handlePending, readThumbnail, placeholderOf) lives in FileApiImpl. Understanding thumbnail behavior requires bouncing between two modules. FileApi's interface is 11 methods wide because it carries thumbnail concerns that belong to a separate concern.

+

Solution: Extract a ThumbnailService module that owns the entire thumbnail lifecycle — generation, retrieval, sync-fallback, placeholder. FileApi narrows to file operations only. FileApiImpl.upload() and completeMultipart() call ThumbnailService.assignInitialStatus() + generate() instead of reaching into ThumbnailGenerationTask directly.

+
+ +
+

Wins

+
    +
  • ▸ locality: thumbnail bugs concentrate in one module
    • +
  • ▸ leverage: one ThumbnailService interface, FileApi + FileController both call it
    • +
  • ▸ interface shrinks: FileApi drops getThumbnail
    • +
  • ▸ tests hit one interface for the full lifecycle
    • +
  • ▸ FileApiImpl shrinks ~100 lines
    • +
+
+
+
+ + +
+
+
+

2. Permission seeding: shadow entities + logic duplication

+
+ Strong + ports & adapters +
+
+ +
+

Files

+
    +
  • crm-dict/config/DictPermissionInitializer.java — 157 lines, seeds dict:* permission points
    • +
  • crm-dict/domain/entity/SysMenuSeed.java, SysRoleSeed.java, SysRoleMenuSeed.java — shadow entities mirroring auth tables
    • +
  • crm-auth/config/DataInitializer.java — 165 lines, seeds crm:role:* permission points (same pattern)
    • +
+
+ +
+
+
Before
+
+
+
crm-dict / DictPermissionInitializer
+
findMenuByName · insertButtonIfAbsent · bindIfAbsent
+
+
+
SysMenuSeed, SysRoleSeed, SysRoleMenuSeed
+
@TableName("sys_menu") — schema copy
+
+
↻
+
+
crm-auth / DataInitializer
+
findMenu · insertButtonIfAbsent · bindIfAbsent
+
+
+
SysMenu, SysRole, SysRoleMenu
+
@TableName("sys_menu") — real schema
+
+
+

Schema duplicated; logic duplicated; leak across seam

+
+ +
+
After
+
+
+
crm-base / PermissionSeeder (interface)
+
registerPoint(perms, apiUrl, name, parentPath)
+
+
⊡⊡ seam ⊡⊡
+
+
+
crm-dict initializer
+
thin caller
+
+
+
crm-auth initializer
+
thin caller
+
+
+
+
PermissionSeederImpl (crm-auth)
+
owns SysMenu, SysRole, SysRoleMenu schema
+
+
+

Schema knowledge in one module; one interface for seeding

+
+
+ +
+

Problem: Two initializers duplicate the same permission-seeding logic (findMenuByName, insertButtonIfAbsent, bindIfAbsent) with nearly identical structure. crm-dict creates shadow entities (SysMenuSeed, SysRoleSeed, SysRoleMenuSeed) that mirror crm-auth's physical schema. ADR-0015 says "the permission code string is the only contract with the auth system," but the shadow entities leak the entire sys_menu schema (column names, menu types, status values) into crm-dict. Schema changes in crm-auth silently break crm-dict's seeding.

+

Solution: Extract a PermissionSeeder seam — an interface in crm-base that declares registerPoint(perms, apiUrl, name, parentMenuPath) without exposing sys_menu's schema. crm-auth provides the implementation (owns SysMenu, SysRole, SysRoleMenu). Both initializers become thin callers of the same seam. Three shadow entities deleted.

+
+ +
+

+ ADR-0015 alignment: This fulfills the ADR, not contradicts it. The ADR says "crm-dict depends only on crm-base" and "the permission code string is the only contract." A PermissionSeeder interface in crm-base, implemented by crm-auth, preserves both constraints. The current shadow-entity approach actually violates the ADR's intent by leaking physical schema — the contract should be code strings, not @TableName("sys_menu"). +

+
+ +
+

Wins

+
    +
  • ▸ locality: sys_menu schema knowledge concentrates in one module
    • +
  • ▸ leverage: one PermissionSeeder interface, N seeders call it
    • +
  • ▸ delete 3 shadow entities + ~100 lines duplicated logic
    • +
  • ▸ future modules (crm-rule, crm-audit) seed permissions through the same seam
    • +
  • ▸ schema changes no longer break cross-module seeding
    • +
+
+
+
+ + +
+
+
+

3. DingTalkAuthClient: test-by-subclass instead of adapter

+
+ Worth exploring + mock +
+
+ +
+

Files

+
    +
  • crm-auth/service/client/DingTalkAuthClient.java — 314 lines; 4 protected HTTP stubs (~120 lines)
    • +
  • crm-auth/service/client/ThirdPartyAuthClient.java — 38 lines interface (getType, getUserInfo, isOrgMember)
    • +
  • crm-auth/service/client/ThirdPartyAuthClientFactory.java — 37 lines
    • +
+
+ +
+
+
Before — mixed mass
+
+
+
interface
+
+
3 methods
+
+
→
+
+
implementation
+
+
HTTP stubs
+
token cache
+
error parse
+
org verify
+
corp token
+
+
314 lines
+
+
+

HTTP plumbing tangled with business logic; tests subclass to stub

+
+ +
+
After — split by concern
+
+
+
client interface
+
+
3 methods
+
+
→
+
+
DingTalkAuthClient
+
+
token cache
+
error parse
+
org verify
+
+
~150 lines
+
+
+
DingTalkHttpClient
+
+
HTTP plumbing
+
+
~120 lines
+
+
+

Business logic testable through adapter; two adapters = real seam

+
+
+ +
+

Problem: Four protected HTTP methods (requestTokenApi, requestUserInfoApi, requestCorpTokenApi, requestGetByUnionIdApi) exist purely as test hooks — tests subclass and override them to inject canned responses. The business logic (token caching, error-code parsing, org membership verification) is tangled with HTTP plumbing. Tests exercise the protected stubs, not the real business logic through the ThirdPartyAuthClient interface. Bugs in error parsing or token refresh hide in private methods that call the stubs.

+

Solution: Extract a DingTalkHttpClient adapter (interface + real implementation) that owns all HTTP calls. DingTalkAuthClient injects the adapter and becomes pure business logic. Tests inject a stub DingTalkHttpClient through the constructor, testing business logic through the ThirdPartyAuthClient interface.

+
+ +
+

Wins

+
    +
  • ▸ locality: HTTP plumbing concentrates in the adapter
    • +
  • ▸ tests inject stub adapter, test through interface (no inheritance)
    • +
  • ▸ delete 4 protected methods + boilerplate
    • +
  • ▸ two adapters (real + test stub) justify the seam
    • +
+
+
+
+ +
+ + +
+

Top recommendation

+

+ #2 — Permission seeding seam +

+

+ Start here. It's the highest-leverage deepening: one new interface in crm-base deletes 3 shadow entities, eliminates ~100 lines of duplicated seeding logic, and future-proofs every new module that needs to seed permission points. It also aligns the codebase with ADR-0015's stated contract ("the permission code string is the only contract with the auth system") — the current shadow-entity approach leaks physical schema across the seam, which the ADR explicitly intended to prevent. The refactor is low-risk: the initializers already run at startup with no runtime callers, so the interface change has no downstream impact. +

+
+ #1 Thumbnail — highest friction but larger blast radius (FileApi is the most-used interface) + #3 DingTalk — testability gain, but only one client exists today +
+
+ +
+ + diff --git a/.scratch/review_permseed_diff.txt b/.scratch/review_permseed_diff.txt new file mode 100644 index 0000000..0dfeeeb Binary files /dev/null and b/.scratch/review_permseed_diff.txt differ diff --git a/.scratch/review_permseed_diff2.txt b/.scratch/review_permseed_diff2.txt new file mode 100644 index 0000000..3a292f0 Binary files /dev/null and b/.scratch/review_permseed_diff2.txt differ diff --git a/crm-auth/target/classes/com/crm/auth/config/DataInitializer.class b/crm-auth/target/classes/com/crm/auth/config/DataInitializer.class index b6709a2..ff64c05 100644 Binary files a/crm-auth/target/classes/com/crm/auth/config/DataInitializer.class and b/crm-auth/target/classes/com/crm/auth/config/DataInitializer.class differ