Architectural Analysis

Architecture Overview

This codebase implements a layered client library for the SMART on FHIR healthcare interoperability protocol. The architecture follows a three-layer pattern: a presentation/demo layer (Flask web app), a client orchestration layer (FHIRClient), and a server communication layer (FHIRServer + FHIRAuth). The client layer delegates all HTTP and OAuth2 concerns to the server layer, which in turn delegates authorization to a pluggable auth subsystem using a Registry pattern. The models layer (fhirclient.models.*) is a separate, auto-generated FHIR resource model library that the client layer imports lazily to avoid circular dependencies — a notable architectural smell. Data flows are primarily synchronous request-response, with OAuth2 redirect flows handled via callback URLs.

Zones of Pain

1. fhirclient/_utils.py — Circular Import Workaround (HIGH)

Evidence: fhirclient/_utils.py:90 performs a local import inside _execute_pagination_request:

from fhirclient.models.bundle import Bundle

This is explicitly documented as "to avoid circular imports" (fhirclient/_utils.py:76-92 claim). The fact that a utility module cannot import models at the top level indicates a circular dependency cycle between fhirclient.server → fhirclient.models.* → fhirclient.server. This is a structural red flag — it means the models layer depends on the server layer, violating the typical layered architecture where models should be dependency-free.

2. fhirclient/client.py — Lazy Model Imports (MEDIUM)

Evidence: fhirclient/client.py:169 performs a local import:

from fhirclient.models.patient import Patient

This is inside the patient property getter. While functional, this pattern obscures the true dependency graph and makes static analysis unreliable. The dependency graph tool reports zero imports for client.py, which is clearly incorrect — the real imports are hidden inside method bodies.

3. fhirclient/server.py — Tight Coupling to FHIRClient (MEDIUM)

Evidence: fhirclient/server.py:37-38 — FHIRServer.__init__ takes a client parameter and stores it as self.client. The server holds a reference back to the client that created it, creating a bidirectional reference between FHIRClient and FHIRServer. This is not a circular import (they're in separate files), but it creates a tight bidirectional coupling that makes either class hard to test or reuse independently.

Coupling Analysis

Key finding: The dependency graph tool reports zero imports for client.py, server.py, and auth.py because all imports are either relative (from .server import ...) or lazy (inside method bodies). The true in-degree is higher than reported — every file that uses FHIRClient or FHIRServer depends on these modules.

Design Pattern Inventory

1. Registry Pattern — FHIRAuth (CONSISTENT)

Evidence: fhirclient/auth.py:16-30 — FHIRAuth maintains a class-level auth_classes dict and a register() classmethod. Subclasses like FHIROAuth2Auth (line 13, auth_type = "oauth2") register themselves. The create() factory method (line 20-30 claim) instantiates the correct subclass by auth_type. This is a well-implemented Registry pattern — clean, extensible, and testable.

2. State Serialization Pattern — All Core Classes (INCONSISTENT)

Evidence: FHIRClient.state (client.py:215-226), FHIRServer.state (server.py:302-308), FHIRAuth.state (auth.py:401-419) all implement state properties and from_state methods. This is a consistent pattern across all three layers for serializing/deserializing session state. However, the pattern is not formalized — there's no abstract interface or protocol, so each class implements it slightly differently (e.g., FHIRAuth.from_state uses state.get() with fallback to self.app_id, while FHIRServer.from_state creates a new FHIRAuth instance).

3. Factory Method — FHIRAuth.create() (CONSISTENT)

Evidence: fhirclient/auth.py:20-30 claim — create() is a classmethod factory that returns the registered subclass instance. This is clean and follows the Registry pattern naturally.

4. Adapter Pattern — FHIRServer wrapping requests.Session (INFORMAL)

Evidence: fhirclient/server.py:44 — self.session = requests.Session(). The FHIRServer class wraps the requests library with FHIR-specific headers, error handling, and auth signing. This is an informal Adapter — it works but isn't abstracted behind an interface, making it hard to swap out the HTTP library.

Refactoring Priorities

1. 🔴 Break the Circular Dependency Between server and models (HIGH)

Problem: _utils.py cannot import Bundle at module level because models.bundle likely imports FHIRServer or something that depends on it. This forces lazy imports that hide the true dependency graph. Solution: Extract an abstract HTTP interface (e.g., HttpClient protocol) from FHIRServer. Have FHIRServer implement it, and have models depend only on the abstract interface. This breaks the cycle and follows Dependency Inversion. Expected benefit: Eliminates all lazy imports, makes static analysis accurate, enables HTTP mocking without subclassing FHIRServer.

2. 🟡 Formalize the State Serialization Pattern (MEDIUM)

Problem: state/from_state is implemented ad-hoc across three classes with subtle inconsistencies. No type safety, no validation. Solution: Define a Stateful protocol or abstract base class with state: dict and from_state(state: dict) -> None. Implement it consistently across FHIRClient, FHIRServer, and FHIRAuth. Add type annotations. Expected benefit: Reduces bugs from state deserialization mismatches, improves IDE support, makes the serialization contract explicit.

3. 🟡 Extract FHIRClient into Separate Concerns (MEDIUM)

Problem: FHIRClient (lines 16-226) handles: (a) client initialization from settings/state, (b) OAuth2 flow delegation, (c) patient management, (d) state persistence, (e) human name formatting. This violates the Single Responsibility Principle. Solution: Extract patient management into a PatientManager class, and name formatting into a utility function. Keep FHIRClient as a thin orchestrator. Expected benefit: Each class becomes testable in isolation. The patient property's lazy import of Patient becomes a top-level import in PatientManager.

4. 🟢 Remove Duplicate MockServer Definitions (LOW)

Problem: tests/server_test.py:178-181 and tests/fhirreference_test.py:133-139 both define a MockServer class extending FHIRServer with nearly identical logic (override request_json to read local files). This is duplicated test infrastructure. Solution: Extract a shared MockServer into a test helper module. Expected benefit: Reduces maintenance burden when the FHIRServer interface changes.