Copilot commented on code in PR #6315: URL: https://github.com/apache/fineract/pull/6315#discussion_r3841448177
########## tools/archmetrics_to_vega.py: ########## @@ -0,0 +1,234 @@ +#!/usr/bin/env python3 +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Turn the java-architecture-metrics JSON report into Vega-Lite specs. + +The plugin reports, per component: classCount, abstractTypeCount, +afferentCoupling (Ca), efferentCoupling (Ce), abstractness (A), +instability (I), distance (D), plus the directed incoming/outgoingComponents +sets. Everything below is derived from that one file. + + python3 tools/archmetrics_to_vega.py <report.json> [--outdir DIR] [--exclude external,platform] +""" +import argparse, json, pathlib, sys + +MAIN_SEQUENCE = "main-sequence.vl.json" +DISTANCE_RANK = "distance-ranking.vl.json" +COUPLING_MTX = "cross-feature-matrix.vl.json" + + +def load(path, exclude): + report = json.loads(pathlib.Path(path).read_text()) + rows = [] + for c in report.get("components", []): + if c.get("component") in exclude: + continue + out = [d for d in c.get("outgoingComponents", []) if d not in exclude] + inc = [d for d in c.get("incomingComponents", []) if d not in exclude] + rows.append({ + "component": c.get("component"), + "classCount": c.get("classCount", 0), + "abstractTypeCount": c.get("abstractTypeCount", 0), + "Ca": c.get("afferentCoupling", 0), + "Ce": c.get("efferentCoupling", 0), + "A": round(c.get("abstractness", 0.0), 4), + "I": round(c.get("instability", 0.0), 4), + "D": round(c.get("distance", 0.0), 4), + "CFV": len(out), + "outgoing": sorted(out), + "incoming": sorted(inc), + }) + return sorted(rows, key=lambda r: -r["CFV"]) + + +def main_sequence(rows): + return { + "$schema": "https://vega.github.io/schema/vega-lite/v5.json";, + "title": "Abstractness vs Instability (distance from the main sequence)", + "width": 520, "height": 420, + "data": {"values": [{k: r[k] for k in ("component", "A", "I", "D", "classCount")} for r in rows]}, + "layer": [ + { # the main sequence itself: A + I = 1 + "data": {"values": [{"I": 0, "A": 1}, {"I": 1, "A": 0}]}, + "mark": {"type": "line", "strokeDash": [6, 4], "color": "#888"}, + "encoding": { + "x": {"field": "I", "type": "quantitative"}, + "y": {"field": "A", "type": "quantitative"}, + }, + }, + { + "mark": {"type": "circle", "opacity": 0.8}, + "encoding": { + "x": {"field": "I", "type": "quantitative", "title": "Instability (I = Ce / (Ca + Ce))", + "scale": {"domain": [0, 1]}}, + "y": {"field": "A", "type": "quantitative", "title": "Abstractness (A)", + "scale": {"domain": [0, 1]}}, + "size": {"field": "classCount", "type": "quantitative", "title": "classes"}, + "color": {"field": "D", "type": "quantitative", "title": "Distance", + "scale": {"scheme": "orangered"}}, + "tooltip": [{"field": c, "type": t} for c, t in + (("component", "nominal"), ("A", "quantitative"), ("I", "quantitative"), + ("D", "quantitative"), ("classCount", "quantitative"))], + }, + }, + { + "mark": {"type": "text", "dy": -10, "fontSize": 9}, + "encoding": { + "x": {"field": "I", "type": "quantitative"}, + "y": {"field": "A", "type": "quantitative"}, + "text": {"field": "component", "type": "nominal"}, + }, + }, + ], + } + + +def distance_ranking(rows): + return { + "$schema": "https://vega.github.io/schema/vega-lite/v5.json";, + "title": "Distance from the main sequence, by component", + "width": 520, "height": {"step": 14}, + "data": {"values": [{k: r[k] for k in ("component", "D", "Ce", "Ca")} for r in rows]}, + "mark": "bar", + "encoding": { + "y": {"field": "component", "type": "nominal", "sort": "-x", "title": None}, + "x": {"field": "D", "type": "quantitative", "title": "Distance (D = |A + I - 1|)"}, + "color": {"field": "D", "type": "quantitative", "legend": None, + "scale": {"scheme": "orangered"}}, + "tooltip": [{"field": c, "type": t} for c, t in + (("component", "nominal"), ("D", "quantitative"), + ("Ce", "quantitative"), ("Ca", "quantitative"))], + }, + } + + +def coupling_matrix(rows): + cells = [{"from": r["component"], "to": t, "v": 1} for r in rows for t in r["outgoing"]] + return { + "$schema": "https://vega.github.io/schema/vega-lite/v5.json";, + "title": "Cross-feature dependencies (row depends on column)", + "width": 520, "height": 520, + "data": {"values": cells}, + "mark": "rect", + "encoding": { + "y": {"field": "from", "type": "nominal", "title": "depends on ->"}, + "x": {"field": "to", "type": "nominal", "title": None, + "axis": {"labelAngle": -45, "orient": "top"}}, + "color": {"value": "#d1495b"}, + "tooltip": [{"field": "from", "type": "nominal"}, {"field": "to", "type": "nominal"}], + }, + } + + +_NAME_LIMIT = 12 + + +def _names(items): + """Render a dependency list, truncated so a section stays readable.""" + if not items: + return "_nothing_" + shown = ", ".join(f"`{d}`" for d in items[:_NAME_LIMIT]) + if len(items) > _NAME_LIMIT: + shown += f", … and {len(items) - _NAME_LIMIT} more" + return shown + + +def skeleton(rows, min_cfv): + """Emit one AsciiDoc section per component, for future authors to fill in. + + Every component present in the report gets a heading, its measured numbers and its + dependency list. The analysis prose is left as TODO markers so that the structure + exists before anyone writes a word. + """ + out = [ + "// Generated by tools/archmetrics_to_vega.py --skeleton. Do not edit by hand:", + "// regenerate with ./gradlew architectureMetricsReport. Fill in the TODO", + "// sections in the chapter that includes this file, not here.", + "", + "// Components with at least %d outgoing cross-component dependency, ordered by" % min_cfv, + "// outgoing dependency count.", + "", + ] + for r in rows: + out += [ + f"=== {r['component']}", + "", + '[cols="^1,^1,^1,^1,^1,^1,^1", options="header"]', + "|===", + "| Classes | Abstract | Ce | Ca | I | A | D", + "", + f"| {r['classCount']} | {r['abstractTypeCount']} | {r['Ce']} | {r['Ca']} " + f"| {r['I']:.2f} | {r['A']:.2f} | {r['D']:.2f}", + "|===", + "", + f"*Depends on ({len(r['outgoing'])}):* " + _names(r["outgoing"]), + "", + f"*Depended on by ({len(r['incoming'])}):* " + _names(r["incoming"]), + "", + "// TODO: what this component is responsible for", + "// TODO: which of the dependencies above are genuine and which are violations", + "// TODO: proposed fix, estimated effort, impact on the source code", + "", + ] + return "\n".join(out) + + +def main(): + ap = argparse.ArgumentParser() + ap.add_argument("report", type=pathlib.Path) + ap.add_argument("--outdir", type=pathlib.Path, + default=pathlib.Path("fineract/fineract-doc/src/docs/en/diagrams")) Review Comment: `--outdir` default points to `fineract/fineract-doc/...`, but this repo uses `fineract-doc/...` at the root. Running the script without `--outdir` will write to an unexpected path (and may create a stray `fineract/` directory). Consider defaulting relative to the repo root derived from `__file__` (e.g., `Path(__file__).resolve().parents[1] / 'fineract-doc/src/docs/en/diagrams'`) or at least changing the literal to `fineract-doc/src/docs/en/diagrams`. ########## fineract-doc/src/docs/en/chapters/architecture/cross-feature-boundary-violations.adoc: ########## @@ -0,0 +1,1426 @@ += Cross-Feature Boundary Violations + +Fineract is built from more than thirty Gradle modules, but the feature code inside +them still behaves largely as a single unit. A class in the accounting feature holds +a direct reference to a charge entity; a client service imports types from loan, +savings, collateral and holidays. The module structure describes an intent that the +code does not yet honour. + +This chapter measures how far the code is from that intent, explains which parts of +the gap are worth closing, and sets out the criteria used to decide which packages to +decouple first. + +== Introduction + +=== What a Feature Boundary Means in Fineract + +A *feature* is a business capability that owns its own data and rules — loan, savings, +charge, accounting, client, group. In the target architecture each feature is a Gradle +module (`fineract-loan`, `fineract-savings`, `fineract-charge`) that publishes a small, +deliberate set of types and keeps everything else to itself. + +A feature's *boundary* is that published set. Code inside the feature may use anything +it owns. Code outside it may use only what the feature publishes, and only through +contracts rather than concrete implementations. + +Two shared modules sit beneath every feature and are deliberately outside this rule: + +* `fineract-core` — shared domain primitives, contracts and abstractions that any + feature may depend on. +* `fineract-command` — the command/handler infrastructure through which state changes flow. + +A dependency on either is not a violation. It is the mechanism by which features are +meant to reach one another. + +One complication has to be stated up front, because it shapes everything that follows. +Fineract's features are Gradle modules, but Java code is organised by *package*, and the +two do not line up: 159 of 1023 main-source packages are split across two to four +modules. `org.apache.fineract.portfolio.loanaccount.service`, for example, has classes +in `fineract-loan`, `fineract-provider`, `fineract-progressive-loan` and `fineract-core` +simultaneously. A boundary is therefore a property of *modules*, not of package names, +and any measurement keyed on package names alone will misattribute code. + +=== What Counts as a Boundary Violation + +A *cross-feature violation* (CFV) is a compile-time reference from a class owned by one +feature to an implementation type owned by another feature. + +Counted as a violation: + +* Importing a concrete class, entity or service implementation from another feature. +* A JPA association that maps directly onto another feature's entity — for example + `ProductToGLAccountMapping` in the accounting module holding a `Charge` field + (see `ProductToGLAccountMapping.java`), which binds the accounting object model to + the charge object model at compile time. +* Injecting another feature's `…ReadPlatformService` implementation rather than a + contract published for the purpose. +* Reading another feature's tables directly in hand-written SQL. + +Not counted as a violation: + +* Any dependency on `fineract-core` or `fineract-command`. +* A feature *implementing* a contract that another feature owns. This is the inversion + that fixes a violation rather than an instance of one, and it is the shape every fix in + <<_how_a_violation_is_fixed>> takes. +* Dependencies on platform infrastructure — security, event framework, jobs, Liquibase. + +The distinction between the second and third bullets of the two lists matters more than +it may appear. Accounting depending on charge is a violation. Charge implementing an +interface that accounting owns is not — the compile-time arrow now points from charge +into accounting-owned contracts, and accounting no longer needs to know charge exists. +That single reversal is the core move used throughout this chapter. + +=== What Kind of Entanglement Are We Trying to Solve + +"Coupling" covers several distinct problems in Fineract, and they do not have the same +cost, the same fix, or the same urgency. Separating them prevents both overstating the +problem and fixing the wrong layer. + +==== Compile-Time Entanglement + +One feature's classes refer to another feature's classes, so the two cannot be compiled, +tested or released apart. This is the entanglement this chapter measures and the one the +CFV count describes. + +It is also the only one of the four that can be fixed purely by moving and re-shaping +Java code, which is why it is addressed first. + +==== Database Entanglement + +All features share a single tenant schema, and that schema contains foreign keys that +cross feature lines. The accounting-to-charge relationship again serves as the example: + +[source,xml] +---- +<addForeignKeyConstraint baseColumnNames="charge_id" baseTableName="acc_product_mapping" + constraintName="FK_acc_product_mapping_m_charge" + onDelete="RESTRICT" onUpdate="RESTRICT" + referencedColumnNames="id" referencedTableName="m_charge"/> +---- + +`acc_product_mapping` is an accounting table. `m_charge` is a charge table. The database +enforces a hard reference between them independently of anything the Java code does. + +This is the single most important qualification in the chapter: *removing a compile-time +dependency does not remove the corresponding data dependency.* Replacing the `Charge` +association with a `Long chargeId` decouples the object model while the foreign key +constraint remains exactly as it was. That is a real and worthwhile gain — the modules +compile apart — but it is not database separation, and the chapter does not claim it as +such. + +==== Infrastructure Entanglement + +Every feature uses the same command bus, event framework, security context, tenant +resolution and Liquibase master changelog. Features are entangled through this shared +substrate in the sense that they all depend on it. + +This is by design and is not a target. Shared infrastructure is what makes features +interoperable at all; the aim is for it to be the *only* thing they share. + +==== Runtime and Transactional Entanglement + +Cross-feature calls run in-process and typically inside one database transaction, so a +failure in one feature can roll back work in another. Decoupling at compile time does +not change this, and this chapter does not attempt to. + +It is recorded here because it is the boundary of what the work achieves: the outcome is +modules that build and test independently, not services that fail independently. + +=== Why Cross-Feature Dependencies Matter + +The measured position — 115 cross-feature dependency pairs across 13,887 class-level +edges, with only 2 of 20 features clean — has concrete consequences: + +* *Change amplification.* A change to a charge entity can force recompilation and + retesting of accounting, loan, savings and shareaccounts, because all four reference it + directly. +* *No independent testing.* A feature cannot be exercised without standing up most of the + platform, which pushes work into slow integration tests. +* *Dependency cycles.* Mutual dependencies between features mean there is no ordering in + which they can be built or reasoned about separately. +* *Onboarding cost.* A contributor cannot form a working mental model of one feature + without also learning several others. +* *Modularisation is blocked.* Extracting any feature into its own module is impossible + while its compile-time reach is unbounded, so the module structure cannot advance. + +=== Scope of This Analysis + +Twenty feature packages were analysed, covering the portfolio and accounting features +that make up the functional core of the platform. The unit of measurement is the +feature-to-feature dependency pair; supporting evidence is drawn from class-level +metrics because package-level metrics cannot be attributed reliably (see +<<_what_a_feature_boundary_means_in_fineract>>). + +==== Which Database + +Fineract runs on MariaDB and PostgreSQL, and the analysis is *engine-independent* — no +measurement in this chapter varies by database vendor. + +What matters is the schema layout, which is identical on both engines: + +* A single *tenant schema* holds the tables of every feature. There is no per-feature + schema, and no per-feature database. +* Schema evolution is driven by one Liquibase master changelog, with the bulk of the + feature tables created in `0001_initial_schema.xml`. Some newer modules + (`fineract-savings`, `fineract-progressive-loan`, `fineract-rates`, + `fineract-working-capital-loan`) carry their own changelog directories, so migration + ownership is partially distributed already. +* Cross-feature foreign keys exist throughout that shared schema. + +In scope: identifying the foreign keys and shared tables that constrain a proposed +extraction, and reporting them as part of each case study's impact assessment. + +Out of scope: splitting the schema, removing cross-feature foreign keys, or introducing +per-feature datasources. Those are separate pieces of work with their own migration and +operational risk, and none of the changes proposed here depend on them. + +==== Infrastructure in Scope + +The build, test and enforcement infrastructure is in scope, because decoupling that is +not enforced regresses: + +* Gradle module structure and inter-module dependency declarations. +* The contract-hosting role of `fineract-core`. +* Automated boundary verification in the test suite. + +Application runtime infrastructure — deployment topology, messaging, caching, connection +pooling — is not in scope and is unaffected by this work. + +==== What This Chapter Does Not Cover + +* *Microservice extraction.* The target is a modular monolith. Nothing here proposes + separate deployables. +* *Runtime or transactional isolation*, as set out above. +* *A complete directed dependency graph from MetricsTree alone.* MetricsTree measures + coupling magnitude, never direction: `Ce = 11` says a package reaches out to eleven + outside classes, never which ones. Directed feature-to-feature edges are derived from + the import graph; MetricsTree supplies magnitude and feature-envy evidence. + +=== How to Read This Chapter + +<<_measuring_boundary_violations>> covers tooling, metric definitions and the measured +baseline. Readers who want to know *what to do* rather than *how it was measured* can +begin at <<_deciding_whether_a_package_is_worth_decoupling>>, which sets out the +selection criteria, and then read the three case studies. + +Each case study follows the same six-part structure — the package, its metric profile, +the violation, the proposed fix, the estimated effort, and the impact on the source code +— so that the same slot can be compared across all three severities in +<<_comparative_analysis>>. + +== Complexity + +=== What Complexity We Are Solving + +The complexity being removed is *accidental*, not *essential*. + +Essential complexity is inherent in the domain and will survive any refactoring. A loan +genuinely has charges. Posting a repayment genuinely produces accounting entries. A +client genuinely has loans, savings accounts and collateral. No amount of module +restructuring makes these relationships go away, and none of the work proposed here +attempts to. + +Accidental complexity is how those relationships are currently *expressed in code*: + +* Accounting knows the `Charge` *class* when all it requires is a charge *identifier* + and a small amount of charge metadata. +* The client feature imports concrete types from loan, savings, charge, collateral, + note, accounting, staff, working days, holidays and data queries in order to assemble + what is ultimately a read-only view. +* A feature that needs one field from a neighbour takes a compile-time dependency on + that neighbour's entire object graph. + +In each case the *dependency is real* but the *coupling is far stronger than the +dependency requires*. That gap — between what a feature genuinely needs and what it +currently imports — is the complexity this work removes. + +=== Where the Complexity Shows Up + +* *Build and test cycles.* Touching a widely-referenced entity triggers rebuilds far + beyond the feature that owns it. +* *Cyclic reasoning.* With 115 cross-feature pairs among 20 features, dependency cycles + are the norm rather than the exception, and no feature can be understood in isolation. +* *Misleading module structure.* Gradle modules imply boundaries that the code does not + respect, so the build files actively mislead newcomers. +* *Hidden coupling.* Because 159 packages are split across modules, some cross-module + dependencies appear as ordinary intra-package references and are invisible to + package-level metrics — the coupling is worse than a naive `Ce` reading suggests. + +=== What Decoupling Will Not Simplify + +Stated plainly, so the expected benefit is not oversold: + +* *Domain complexity is unchanged.* The same business rules, in the same number, after + the work as before. +* *Class count rises.* Interfaces and DTOs in `fineract-core` are added, not removed. + Decoupling trades a smaller number of tightly-bound types for a larger number of + loosely-bound ones. +* *Indirection increases.* Navigating from a caller to the implementation now passes + through a contract, which is a real cost to readability at the call site. +* *The database is untouched.* Cross-feature foreign keys remain. +* *Runtime behaviour is unchanged.* The same calls happen in the same order in the same + transaction. + +The trade is deliberate: more types, more indirection, and in exchange a module +structure that can be enforced automatically and evolved independently. + +== Measuring Boundary Violations + +=== Tooling and Method + +The platform was measured twice, with independent tooling, because no single tool +produces everything the analysis needs. + +The *first pass* used the MetricsTree plugin inside IntelliJ IDEA. It supplies coupling +magnitude and the feature-envy metrics that identify individual misplaced classes, but it +measures magnitude only — never direction. An efferent coupling of 11 states that a +package reaches eleven classes it does not own and says nothing about which ones, so the +directed feature-to-feature edges had to be derived separately from the import graph. + +The *second pass* uses a Gradle plugin, `io.github.usekylis.java-architecture-metrics`, +applied to the build itself. It computes the same Robert C. Martin set and additionally +reports, for every component, the set of components it depends on and the set that depend +on it. That is the directed matrix, produced by the build rather than reconstructed +afterwards, which makes it the authoritative source for the violation counts. + +The two passes agree on 11 of the 17 comparable features. Where they differ the cause is +taxonomy rather than measurement — most of it because fixed and recurring deposits have +no package of their own and roll up into savings in the build-generated view. + +==== MetricsTree Setup + +[cols="1,3", options="header"] +|=== +| Component | Version and state + +| IDE | IntelliJ IDEA Community Edition 2025.2 +| Plugin | MetricsTree 2026.0.0 (`org.b333vv.metricstree`) +| Gradle sync | Complete — 141 module descriptors in external module storage +| Module scoping | Available; the plugin's module selector lists every module containing classes +|=== + +Open the project, let the Gradle import finish, then open the *MetricsTree* tool window +from the bottom dock. Metrics are calculated from the *Project Metrics* tab; the *Class +Metrics* tab only reports on the file currently open in the editor. + +One structural caveat governs how the output can be read. MetricsTree aggregates by +*package*, while Fineract's features are *Gradle modules*, and the two do not line up: + +> 159 of 1023 main-source packages are split across two to four modules. + +---- +org.apache.fineract.portfolio.loanaccount.service fineract-loan=50, fineract-provider=47, fineract-progressive-loan=20, fineract-core=1 +org.apache.fineract.portfolio.loanaccount.domain fineract-loan=75, fineract-progressive-loan=3, fineract-core=2, fineract-provider=2 +org.apache.fineract.infrastructure.security.service fineract-security=16, fineract-core=9, fineract-provider=6 +org.apache.fineract.portfolio.charge.exception fineract-charge=16, fineract-savings=3, fineract-core=1 +---- + +A dependency between two classes in different modules that happen to share a package name +is *intra-package*, so `Ce` does not count it. Cross-module coupling hidden inside a shared +namespace is invisible to the package-level metric, and scoping runs per module does not +fix it. Only class-level export avoids the problem, because its rows are keyed by fully +qualified class name and each name resolves to exactly one module. + +==== How Measurements Were Collected + +*Build-generated pass (authoritative).* The Gradle plugin reads compiled bytecode rather +than source: for every class it extracts the package, the abstract and interface flags, +and the set of types referenced from the constant pool. Each class is then assigned to a +component by a classifier that collapses packages into features, and the results are +aggregated per component. + +The critical property is how a dependency's target is resolved. The analyzer looks the +target class up in a map built *only from the classes it scanned*; anything outside that +set is discarded. A run scoped to one module therefore computes that module's coupling as +though the rest of the platform did not exist, and every cross-module edge silently +disappears. The measurement consequently runs as a single aggregate scan over every Java +module at once. + +*Corroborating pass.* MetricsTree's class-level export was aggregated per module to +provide the feature-envy view — `ATFD`, `FDP` and `LAA` — which the build-generated +report does not compute. Measured against this checkout, 5336 top-level classes across 36 +modules resolve with zero name collisions, so attribution is exact rather than +best-effort. + +==== Reproducing These Results + +The build-generated figures are reproduced with a single command: + +[source,bash] +---- +./gradlew architectureMetricsReport +---- + +It runs two tasks in sequence. `architectureMetrics` scans every module's compiled classes +and writes `fineract-feature-metrics.json` and `.csv` under `build/reports/architecture`. +`architectureMetricsDiagrams` then regenerates the Vega-Lite specifications in +`fineract-doc/src/docs/en/diagrams` from that report, so the diagrams in this chapter +cannot drift from the numbers they are drawn from. Review Comment: The reproduction section appears out of sync with the Gradle tasks added in this PR: it states that `architectureMetrics` writes `fineract-feature-metrics.json` under `build/reports/architecture`, but the build config writes `fineract-package-metrics.*` under `build/reports/architecture/packages/` and `fineract-feature-metrics.*` under `build/reports/architecture/features/` via `architectureMetricsFeatures`. Aligning the task names and paths here will make the docs reproducible. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
