Skip to content

Commit 25a0084

Browse files
MarketDataDev03MarketDataDev03
committed
adds ADR-007 dicuss
1 parent 326e19d commit 25a0084

1 file changed

Lines changed: 314 additions & 0 deletions

File tree

‎docs/adr/ADR-007-internal-api-encapsulation.md‎

Lines changed: 314 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,314 @@
1+
# ADR-007: Encapsulation of Internal Types on the Public API Surface
2+
3+
## Status
4+
5+
Proposed.
6+
7+
## Context
8+
9+
Resource façades like `MarketsResource` are part of the public API
10+
(`com.marketdata.sdk.markets`) — consumers reach them via
11+
`client.markets()`. They are also constructed by
12+
`MarketDataClient`, which lives in a *different* package
13+
(`com.marketdata.sdk`). Java's visibility levels are `private`,
14+
package-private, `protected`, and `public`; there is no
15+
"module-internal cross-package" level in the language base.
16+
Cross-package construction therefore requires the constructor to be
17+
`public`, even when conceptually "this should not be called from
18+
outside the SDK".
19+
20+
The current `MarketsResource(HttpTransport transport)` constructor
21+
illustrates the problem:
22+
23+
```java
24+
// src/main/java/com/marketdata/sdk/markets/MarketsResource.java
25+
/**
26+
* Package-private: only {@link com.marketdata.sdk.MarketDataClient} constructs
27+
* resources, so consumers get one via {@code client.markets()}.
28+
*/
29+
public MarketsResource(HttpTransport transport) { // ← Javadoc lies; ctor is public
30+
this.transport = transport;
31+
}
32+
```
33+
34+
`HttpTransport` lives in `com.marketdata.sdk.internal.http`. The
35+
package is named `internal` *by convention* — the language gives no
36+
enforcement. Direct consequences:
37+
38+
- A consumer can write
39+
`new MarketsResource(new HttpTransport("https://...", "v1", "ua", "token"))`
40+
and bypass `MarketDataClient` and everything it enforces (configuration
41+
cascade per §4, demo mode, future startup validation, retry
42+
policy, telemetry).
43+
- Generated Javadoc lists the constructor as part of the public API,
44+
blurring the contract.
45+
- Any change to `HttpTransport`'s constructor signature is a
46+
technically SemVer-breaking change because that signature appears
47+
in a public-API position — even though the type *lives in* a package
48+
named `internal`. SemVer-strict tooling (`revapi`, `japicmp`) will
49+
flag it.
50+
51+
The leak is the same kind every non-modular Java SDK has (Stripe Java,
52+
AWS SDK v1, Twilio). It is moderate severity rather than critical, but
53+
it directly contradicts the Kotlin-interop and clean-API goals of
54+
ADRs 001 and 006.
55+
56+
A related observation: the same problem appears for *every* resource
57+
that lands in subsequent endpoint work (`stocks`, `options`, `funds`,
58+
`utilities`). Whatever we choose, it must scale to roughly five resource
59+
façades by v1.
60+
61+
## Options Considered
62+
63+
### Option A — JPMS (`module-info.java`)
64+
65+
Add a Java module descriptor at `src/main/java/module-info.java` that
66+
exports only the public-API packages and leaves `internal.*` unexported.
67+
Consumers using the modulepath will find `HttpTransport` literally
68+
unreachable: the import line will fail to compile.
69+
70+
```java
71+
module com.marketdata.sdk {
72+
requires java.net.http;
73+
requires com.fasterxml.jackson.databind;
74+
requires static org.jspecify; // compile-time only
75+
76+
exports com.marketdata.sdk;
77+
exports com.marketdata.sdk.markets;
78+
exports com.marketdata.sdk.exception;
79+
// com.marketdata.sdk.internal.* — intentionally NOT exported
80+
}
81+
```
82+
83+
**Pros**
84+
85+
- Zero refactor of existing source. The current package layout is
86+
preserved verbatim — `internal/http`, `internal/wire/markets`, the
87+
whole tree stays where it is.
88+
- Compile-time enforcement at the *language* level for modulepath
89+
consumers: the consumer's compiler refuses
90+
`import com.marketdata.sdk.internal.http.HttpTransport`.
91+
- IDEs surface the unexported package as a warning or error even when
92+
the consumer is on classpath.
93+
- SemVer-strict tooling (`revapi`, `japicmp`) reads `module-info` and
94+
knows precisely what the API surface is; spurious "breaking
95+
changes" on `internal.*` types disappear.
96+
- This is what AWS SDK v2, Spring Boot (modular), Hibernate ORM
97+
(modular) do.
98+
99+
**Cons**
100+
101+
- Enforcement is full only on **modulepath** consumers. Classpath
102+
consumers see `internal.*` as part of the unnamed module and can
103+
still reference it (with IDE warnings).
104+
- Adds module-system reasoning to the build (transitive `requires`,
105+
test patching). Gradle 9 + `java-library` handles this transparently
106+
for our case, but it is one more thing to know.
107+
- Reflection still works the same way as today: callers using
108+
`setAccessible(true)` can reach private members. To block that, the
109+
consumer would need to be denied `--add-opens` for our packages —
110+
which is the JVM default for non-exported packages.
111+
112+
#### End-user impact (special focus per the request)
113+
114+
How JPMS affects each consumer cohort:
115+
116+
- **Production consumer using Maven/Gradle classpath (most common today):**
117+
Library appears on the classpath, the JVM treats it as part of the
118+
unnamed module, and `exports` directives are *not enforced at runtime*.
119+
Such a consumer can still write
120+
`import com.marketdata.sdk.internal.http.HttpTransport;`. **However**,
121+
IntelliJ, Eclipse, and recent VS Code Java extensions display a
122+
visible warning ("module com.marketdata.sdk does not export
123+
com.marketdata.sdk.internal.http") — strong social signal, no hard
124+
block.
125+
126+
- **Production consumer using modulepath (`requires com.marketdata.sdk;`
127+
in their own `module-info.java`):**
128+
The import simply does not compile. The consumer's build fails with
129+
"package com.marketdata.sdk.internal.http is declared in module
130+
com.marketdata.sdk, which does not export it." This is the cohort the
131+
ADR primarily protects.
132+
133+
- **Consumer running tests against the SDK:**
134+
Their test code, when it lives in their own module, is governed by
135+
the same rules as their production code. Tests in our SDK's own
136+
module continue to access internals via the test source set
137+
(Gradle handles `--patch-module` for test classpaths automatically).
138+
139+
- **Reflective consumer (advanced):**
140+
Default behaviour for non-exported packages is "deep reflection
141+
blocked." The consumer must add `--add-opens
142+
com.marketdata.sdk/com.marketdata.sdk.internal.http=ALL-UNNAMED` to
143+
the JVM args of their application — a deliberate choice that is now
144+
visible at deploy time, not buried in code.
145+
146+
- **Consumer of the published Javadoc:**
147+
Standard Javadoc for a modular JAR omits the unexported packages.
148+
`com.marketdata.sdk.internal.*` no longer appears in the rendered
149+
docs, eliminating the "Javadoc lists the constructor as public API"
150+
symptom.
151+
152+
- **Consumer running a SemVer-comparison tool (`revapi`, `japicmp`):**
153+
Tools that read `module-info` correctly classify `internal.*` as
154+
non-API. Changes to `HttpTransport`'s signature stop registering as
155+
breaking — they were never API.
156+
157+
- **Migration cost on the consumer side:**
158+
Zero for classpath consumers (they keep working as before). Modulepath
159+
consumers add `requires com.marketdata.sdk;` to their `module-info` —
160+
a one-line change, standard for any modular dependency.
161+
162+
The asterisk to be honest about: a determined classpath consumer can
163+
*technically* still use `internal.*` types — but they get IDE
164+
warnings, lose Javadoc visibility, and accept that any future change
165+
to those types is by definition not a SemVer break (we can change them
166+
freely). The contract is now legible.
167+
168+
### Option B — Single-package infra (collapse the directory tree)
169+
170+
Move `MarketsResource` (and every future resource façade), `HttpTransport`,
171+
`AsyncSemaphore`, `RequestSpec`, `HttpStatusMapper`, `RateLimitHeaders`,
172+
`Configuration`, `EnvVars`, `Tokens`, `Version`, plus every wire-format
173+
deserializer — all into the same package as `MarketDataClient`
174+
(`com.marketdata.sdk`). Drop the `public` modifier from every
175+
"internal" class so it becomes package-private. The compiler then
176+
refuses to compile a consumer's reference to those types because the
177+
type itself is not visible.
178+
179+
```
180+
com.marketdata.sdk/
181+
├── MarketDataClient.java public
182+
├── RateLimits.java public
183+
├── MarketsResource.java public final, ctor package-private
184+
├── HttpTransport.java package-private
185+
├── AsyncSemaphore.java package-private
186+
├── RequestSpec.java package-private
187+
├── HttpStatusMapper.java package-private
188+
├── RateLimitHeaders.java package-private
189+
├── Configuration.java package-private
190+
├── MarketStatusDeserializer.java package-private
191+
├── SdkJsonModule.java package-private
192+
└── ... ~25 files in a single root package
193+
194+
com.marketdata.sdk.markets/ ← only public response DTOs
195+
├── MarketStatus.java
196+
└── DailyStatus.java
197+
198+
com.marketdata.sdk.exception/ ← only public exceptions
199+
└── ...
200+
```
201+
202+
The consumer cannot import a type that is not `public`, so:
203+
204+
```java
205+
// Does NOT compile:
206+
import com.marketdata.sdk.HttpTransport; // package-private
207+
new MarketsResource(/*...*/); // package-private ctor
208+
```
209+
210+
**Pros**
211+
212+
- Pure Java visibility. No JPMS, no module reasoning, no tooling
213+
asterisk. Compile-time enforcement everywhere — classpath and
214+
modulepath alike.
215+
- Reflection still gated by `setAccessible(true)` + `--add-opens` in
216+
modular runtimes; for non-modular runtimes the bar is the same as
217+
the rest of the JDK.
218+
- Industry precedent: Stripe Java's pre-modular layout uses a similar
219+
structure (one root package, response DTOs in subpackages).
220+
221+
**Cons**
222+
223+
- Refactor scope is real: ~25 files change package + the `internal/`
224+
subdirectory disappears as a visual cue (replaced by the
225+
`package-private` modifier as the "do not touch" signal).
226+
- The `@JsonDeserialize(using = MarketStatusDeserializer.class)`
227+
annotation on response records can no longer reference a
228+
package-private class from another package. Deserializers must move
229+
to programmatic registration via a `SimpleModule` on
230+
`HttpTransport`'s `ObjectMapper`. This is a real architectural change
231+
on top of the move (decoupling response models from their wire-format
232+
logic — arguably an improvement, but additional work).
233+
- Test sources for any package-private type must live in the matching
234+
test package, which already happens — but every test that previously
235+
imported `com.marketdata.sdk.internal.http.HttpTransport` now imports
236+
`com.marketdata.sdk.HttpTransport`. Mechanical, not zero.
237+
- Naming feels slightly off: `com.marketdata.sdk.MarketsResource` instead
238+
of `com.marketdata.sdk.markets.MarketsResource`. Consumers rarely
239+
import the resource class by name (they go through
240+
`client.markets()`), but it is a smell at first glance.
241+
- A single package with ~25 classes is fine technically but
242+
organisationally noisier than the current layered tree.
243+
244+
## Claude's Recommendation
245+
246+
**Option A (JPMS), with Option B as a follow-on if and when the team
247+
decides classpath enforcement is required.**
248+
249+
Reasoning:
250+
251+
1. JPMS is the **lowest-risk** option that produces real enforcement
252+
for the most relevant consumer cohorts. ~1 file added (`module-info.java`),
253+
no source moves, no API renames, no annotation refactor.
254+
2. Modulepath adoption among Java consumers is increasing every year.
255+
AWS SDK v2 and Spring Boot have already crossed the bar; Java
256+
consumers building anything modular today already write
257+
`module-info` files. By the time this SDK has been in the field for
258+
v1.x lifetime, modulepath enforcement will cover the majority of
259+
serious consumers.
260+
3. The classpath asterisk is real but mitigated by IDE warnings and
261+
SemVer tooling that already reads `module-info` correctly. The
262+
consumer who *deliberately* imports `internal.*` from classpath has
263+
accepted a one-sided contract — we can change those types freely.
264+
4. Option B genuinely closes the gap for classpath consumers but at
265+
substantial refactor cost (~25 file moves + deserializer
266+
registration refactor). Worthwhile only if the threat model is
267+
"adversarial classpath consumer". For "respectful classpath consumer
268+
+ adversarial modulepath consumer", Option A is sufficient and B is
269+
over-engineering.
270+
271+
The strongest counter-argument to Option A is "it is not enforcement,
272+
only a strong signal, for classpath consumers." That is correct. The
273+
counter-counter-argument is "every Java SDK in our space has the same
274+
property; the alternative (Option B) costs more than the gap is
275+
worth." If the team disagrees on the threat model, Option B is the
276+
escape hatch — but it should be a deliberate choice, not a default.
277+
278+
## Decision
279+
280+
*Pending team review.*
281+
282+
## Consequences
283+
284+
Follow-on work implied by each option. The chosen option will be
285+
marked when the decision is made.
286+
287+
- **A (JPMS, recommended):** Add `src/main/java/module-info.java` with
288+
the exports listed in the option above. Add `requires` clauses for
289+
Jackson and JSpecify (`requires static org.jspecify` for compile-time
290+
only). Update CLAUDE.md to record the modulepath/classpath
291+
enforcement contract. No source moves, no refactor of resource code.
292+
Tests continue to access internals via Gradle's standard test source
293+
set wiring. Verify that `./gradlew build` still succeeds end-to-end
294+
and that `javadoc` no longer surfaces `internal.*` packages.
295+
296+
- **B (single-package infra):** Move every infra and resource façade
297+
class to `com.marketdata.sdk`; drop `public` from every type that
298+
was internal. Refactor `@JsonDeserialize` annotations off response
299+
records and into a programmatic `SimpleModule` registered on
300+
`HttpTransport`'s `ObjectMapper`. Update every test that imports
301+
formerly-internal types to the new package. Update CLAUDE.md to
302+
describe the single-package convention and the package-private
303+
default for non-DTO classes.
304+
305+
## References
306+
307+
- [Java Language Specification §7 — Packages](https://docs.oracle.com/javase/specs/jls/se17/html/jls-7.html)
308+
- [JEP 261: Module System (JPMS)](https://openjdk.org/jeps/261)
309+
- [State of the Module System — Mark Reinhold](https://openjdk.org/projects/jigsaw/spec/sotms/)
310+
- [Stripe Java SDK — package layout](https://github.com/stripe/stripe-java)
311+
- [AWS SDK for Java v2 — modular layout](https://github.com/aws/aws-sdk-java-v2)
312+
- [`revapi` — API change tracking with module-info awareness](https://revapi.org/)
313+
- [ADR-001 — Java Only vs. Multi-Language SDK](./ADR-001-java-only-vs-multi-language-sdk.md)
314+
- [ADR-002 — Minimum JDK Version](./ADR-002-minimum-jdk-version.md)

0 commit comments

Comments
 (0)