Current Limitation
ApplicationStore (internal/application/store.go) holds a *consignment.Store field and calls its exported methods (Upsert, UpdateStatus, MergeCustomData) directly, inside its own DB transactions. This crosses a domain boundary at the store layer — internal/application's persistence code has a hard dependency on internal/consignment's persistence code — rather than at the service layer, where cross-domain orchestration belongs. A store should only need a *gorm.DB/transaction handle, not another domain's store.
This isn't new: ApplicationStore has held a consignment.Store and called Upsert/UpdateStatus on it since #193 extracted consignment into its own package. #267 (consignment custom data) added a third call site (MergeCustomData) using the same existing pattern, which is what surfaced this in review — see PR #267 review comment.
Suggested Improvement
Stores should depend only on the database connection/transaction they're given, not on other domains' store types. The propagation of an application's status (and now custom data fields) onto its parent consignment — currently done by ApplicationStore reaching into consignment.Store — should move up to the service layer, with ApplicationService coordinating against a consignment-side dependency (service or store) that it's handed explicitly, rather than ApplicationStore importing internal/consignment at all.
The main constraint to design around: today this works inside one DB transaction, so an application row and its parent consignment row are always updated atomically. Whatever replaces this needs to preserve that guarantee — e.g. ApplicationStore returning what needs to propagate (new status, resolved custom-data fields) from inside its own transaction, or accepting a caller-supplied "on commit" hook, with the service layer driving both stores against the same transaction.
This touches every call site currently using s.consignmentStore.* in internal/application/store.go (CreateOrUpdate, UpdateStatus, ReviewApplication, UpdateDataAndResetStatus), so it's a dedicated refactor rather than something to fold into an unrelated feature PR.
Version
main (as of #267 / commit a3800d5)
Additional Context
Raised by @sthanikan2000 during review of #267. Two smaller, related review comments from the same review were addressed as immediate fixes on that PR rather than filed separately; this one is structural enough to warrant its own issue and its own PR.
Current Limitation
ApplicationStore(internal/application/store.go) holds a*consignment.Storefield and calls its exported methods (Upsert,UpdateStatus,MergeCustomData) directly, inside its own DB transactions. This crosses a domain boundary at the store layer —internal/application's persistence code has a hard dependency oninternal/consignment's persistence code — rather than at the service layer, where cross-domain orchestration belongs. A store should only need a*gorm.DB/transaction handle, not another domain's store.This isn't new:
ApplicationStorehas held aconsignment.Storeand calledUpsert/UpdateStatuson it since #193 extractedconsignmentinto its own package. #267 (consignment custom data) added a third call site (MergeCustomData) using the same existing pattern, which is what surfaced this in review — see PR #267 review comment.Suggested Improvement
Stores should depend only on the database connection/transaction they're given, not on other domains' store types. The propagation of an application's status (and now custom data fields) onto its parent consignment — currently done by
ApplicationStorereaching intoconsignment.Store— should move up to the service layer, withApplicationServicecoordinating against a consignment-side dependency (service or store) that it's handed explicitly, rather thanApplicationStoreimportinginternal/consignmentat all.The main constraint to design around: today this works inside one DB transaction, so an application row and its parent consignment row are always updated atomically. Whatever replaces this needs to preserve that guarantee — e.g.
ApplicationStorereturning what needs to propagate (new status, resolved custom-data fields) from inside its own transaction, or accepting a caller-supplied "on commit" hook, with the service layer driving both stores against the same transaction.This touches every call site currently using
s.consignmentStore.*ininternal/application/store.go(CreateOrUpdate,UpdateStatus,ReviewApplication,UpdateDataAndResetStatus), so it's a dedicated refactor rather than something to fold into an unrelated feature PR.Version
main (as of #267 / commit a3800d5)
Additional Context
Raised by @sthanikan2000 during review of #267. Two smaller, related review comments from the same review were addressed as immediate fixes on that PR rather than filed separately; this one is structural enough to warrant its own issue and its own PR.