docs: Layered Architecture for API Separation of Concerns RFC BED-7036 #2172
Conversation
WalkthroughA new RFC file Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (1)
rfc/bh-rfc-5.md (1)
53-75: Consider adding migration and backwards compatibility guidance.While section 3.3 mentions incremental implementation (one service at a time), the RFC doesn't address how to maintain external API stability during the transition. This could be important for teams implementing this architecture:
- How should external API contracts be versioned while services migrate to the new architecture?
- Are there breaking change concerns as services transition?
- Should there be a deprecation period for the old architecture?
This could be a separate migration RFC or a brief appendix to this one, but it may be worth addressing before this moves from DRAFT status.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rfc/bh-rfc-5.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1820
File: packages/go/stbernard/command/audit/audit.go:133-136
Timestamp: 2025-08-26T21:25:33.480Z
Learning: superlinkx prefers to receive feedback and identify issues as early as possible in the review process rather than later.
🪛 LanguageTool
rfc/bh-rfc-5.md
[grammar] ~164-~164: Ensure spelling is correct
Context: ... Can be monolithic like the existing DB struct, or broken up along useful boundaries. -...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (3)
rfc/bh-rfc-5.md (3)
168-170: Expand transaction handling guidance.The transaction handling section is too sparse. Engineers implementing this will need concrete guidance on:
- How does the service layer request a transaction from the data layer? (Does it call a method that returns an opaque transaction object/interface?)
- How are nested data layer calls executed within a transaction?
- What is the public interface the service layer depends on?
This is a critical implementation detail that should be made explicit so services and data layers implement this pattern consistently.
Consider adding a subsection with a concrete example, such as:
// Service layer requests transaction txn, err := dataLayer.BeginTxn(ctx) if err != nil { /* handle */ } // Service passes transaction to nested calls user, err := dataLayer.GetUser(txn, userID) // ... // Service commits/rollbacks err = txn.Commit()Or clarify if the pattern differs (e.g., callback-based transaction handling).
113-153: Clarify sentinel error matching for callers.The error handling section defines how layers should wrap errors, but doesn't explain how callers distinguish between different sentinel error types. Add guidance on:
- How should the calling code check which sentinel error type was returned? (using
errors.Is(), type assertion, etc.)- Should sentinels be comparable with
errors.Is()?- How should this work across layer boundaries?
An example of error matching at the view layer would be valuable:
// How should handlers determine which app error occurred? _, err := service.GetUser(ctx, userID) if errors.Is(err, app.ErrNotFound) { // return 404 } else if errors.Is(err, app.ErrUnauthorized) { // return 403 }This clarification will ensure consistent error handling across all layers.
1-256: Comprehensive RFC with strong architectural foundation.This RFC provides clear guidance on layered architecture with well-defined responsibilities, practical model translation patterns, and sensible error boundaries. The document is well-structured, uses helpful diagrams, and establishes principles that will reduce architectural debt.
The three review comments above flag areas where additional implementation detail (transactions, error matching, backwards compatibility) would strengthen the RFC before it advances from DRAFT status. These are refinements rather than fundamental issues.
|
This first commit was hastily thrown through an LLM from a handwritten design document I wrote. I'm still validating the details that are captured for a proper first pass, but it looked reasonably correct at first glance and I'd like to start sharing early. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
rfc/bh-rfc-5.md (4)
108-140: Error handling strategy is practical with good Go example.The three-step error boundary rule (convert to string, wrap with sentinel, don't leak dependency errors) is clear. The code example (lines 120-129) effectively shows the pattern. The Mermaid flow at lines 131-136 correctly illustrates private-to-public error wrapping.
However, the "shared application registry of sentinel errors" (line 140) is vague. Specify or provide a guideline: where should this separate errors package live (repo root, internal package, etc.), what should it contain, and should it be versioned with the API or separately? This helps implementers avoid ad-hoc error definitions.
142-157: Data Layer responsibilities are well-defined.The design principles are sound: packages provide methods (not their own interfaces), internal conversion between app and data-specific types, and transaction-aware patterns. However, Section 6.3 (Transaction Handling) lacks a concrete example. Provide a brief Go code snippet showing how a transaction object should be passed/returned to illustrate the abstraction boundary. This helps clarify whether the transaction type is public, how errors from transactional calls are handled, etc.
159-195: Service Layer dependency injection guidance is strong and practical.The Mermaid diagram (lines 169-188) clearly shows how interfaces bridge the service and data layer. Focusing interfaces on "only required methods" rather than exposing all data layer methods is a great constraint that keeps surface area small and mock generation scoped per-service.
Recommendation: Add a dedicated Testing section (or expand Section 7) to cover unit testing strategy for services. Key questions: Should services be tested in isolation with mocked data layers? Are integration tests with real data layers expected? Should there be test fixtures for common app models? Currently, testing is only mentioned in passing (line 195), but service layer testing is critical to the architecture's success.
1-241: Overall: Strong architectural foundation with clear guidance—approve for early sharing.This RFC successfully defines a coherent layered architecture with practical separation of concerns, error boundary rules, and dependency injection patterns. The diagrams are clear, the motivation is sound, and the "incremental refactoring" approach is realistic. The "no broken windows" philosophy balances pragmatism with architecture.
Before merge, address:
- Clarify whether this is Go-specific or language-agnostic (or explicitly state Go-first with plans for other languages).
- Expand error registry guidance to specify location and structure of the shared errors package.
- Add a brief transaction handling code example in Section 6.3.
- Consider a dedicated Testing section or expand Section 7 with unit/integration testing strategy for services.
The author's note about LLM generation and ongoing validation is appropriate for a DRAFT. Given superlinkx's preference for early feedback, these recommendations can be incorporated iteratively before finalizing. The architectural vision is sound and will serve the codebase well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rfc/bh-rfc-5.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1820
File: packages/go/stbernard/command/audit/audit.go:133-136
Timestamp: 2025-08-26T21:25:33.480Z
Learning: superlinkx prefers to receive feedback and identify issues as early as possible in the review process rather than later.
🪛 LanguageTool
rfc/bh-rfc-5.md
[grammar] ~151-~151: Ensure spelling is correct
Context: ... Can be monolithic like the existing DB struct, or broken up along useful boundaries. -...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (5)
rfc/bh-rfc-5.md (5)
1-11: RFC frontmatter and metadata look good.Well-structured YAML frontmatter with clear audience definition. Status as DRAFT is appropriate given the author's note about ongoing validation.
13-47: Architecture diagrams are clear and effectively communicate layering.The Mermaid diagram clearly shows the four-layer structure (View, Service, Data, Models) and unidirectional flow. The dotted lines appropriately show how all layers depend on shared models and errors.
49-75: Clear motivation and thoughtful approach to addressing architectural debt.The "no broken windows" philosophy aligns well with pragmatic refactoring. The incremental migration strategy (one service at a time, not wholesale refactor) is practical and realistic for an active codebase.
However, clarify whether this RFC is Go-specific or intended to be language-agnostic. The examples, imports, and package structure references assume Go, but the architectural principles could apply more broadly. Either make language-agnostic statements explicit or clarify in the Overview/Motivation that this guidance is Go-specific.
77-106: Model translation approach is sound and well-visualized.The layer-specific model ownership (Database/View) and shared Application Models is a clean separation. The sequence diagram at lines 89-102 clearly shows the translation flow and conversion points. This prevents API contract drift effectively.
197-241: View Layer handler flow is clear with excellent sequence diagram.The sequence diagram (lines 215-234) effectively illustrates the handler's responsibility: validation → conversion → service call → result conversion. The separation of Views (types with JSON tags) from application models is a clean way to prevent API contract drift. Future extensibility (HTMX, Wails, etc.) is thoughtfully acknowledged.
Description
This RFC provides the guidelines for our backend services architecture proposal that has been years in the making. The draft will help guide decisions on current OpenGraph work, but the final document will be used as the design foundation for upcoming restructure work to help refactor our API services to a more maintainable and robust architecture.
Motivation and Context
Resolves BED-7036
Solves a long standing pain with our codebase organically growing without much proper separation of concerns, and refactors becoming increasingly complex and costly. The goal is to provide ourselves with a lightweight and extensible architecture for the next chapter of BloodHound.
How Has This Been Tested?
Documentation only, no code to test
Types of changes
Checklist:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.