diff --git a/.gitignore b/.gitignore index a958c90..613c8ee 100644 --- a/.gitignore +++ b/.gitignore @@ -29,3 +29,8 @@ Thumbs.db *.log hs_err_pid* replay_pid* + +# Claude personal config — local slash commands, settings, and skills. +# Anything the team wants to share should be added back explicitly via `!`, +# e.g. !.claude/settings.json for shared per-repo settings. +.claude/ diff --git a/CLAUDE.md b/CLAUDE.md index 34f022d..d62e4bf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,7 +63,9 @@ The Java SDK must also satisfy the canonical, cross-language [SDK Requirements]( **Already wired in:** - §1.1 client object — `MarketDataClient` with two public constructors: a no-arg one for production (everything from the cascade) and a 4-arg `(apiKey, baseUrl, apiVersion, validateOnStartup)` for tests and short-lived runtimes. All fields `final` (immutable). Default base URL `https://api.marketdata.app`, default API version `v1`, single shared `HttpClient`, `User-Agent: marketdata-sdk-java/{version}` (version auto-detected from JAR manifest), `close()` for resource release, `getRateLimits()` accessor. - §4 configuration cascade — `Configuration.resolve(...)` does explicit → `MARKETDATA_*` env var → `.env` in CWD → default. Env var names live in `EnvVars` (package-private, in the SDK root package). The 4-arg constructor's parameters feed step 1; the no-arg constructor skips it and starts at step 2. -- §5 demo mode + `validateOnStartup` parameter on the 4-arg constructor (defaults to `true` via the no-arg constructor); token redaction via `Tokens.redact` (matches the spec example `***…***YKT0`). +- §5 demo mode + `validateOnStartup` parameter on the 4-arg constructor (defaults to `true` via the no-arg constructor); token redaction via `Tokens.redact` (matches the spec example `***…***YKT0`). When `validateOnStartup` is true and not in demo mode, the constructor hits `GET /user/` via `HttpTransport.validateToken` (single-attempt, no retry) so invalid tokens surface as `AuthenticationError` from the constructor instead of from the first business call. +- §7 logging: `MARKETDATA_LOGGING_LEVEL` (via cascade) is applied to the `com.marketdata.sdk` logger at client construction. When set, a `MarketDataLogFormatter` (producing the spec-mandated `{timestamp} - {logger_name} - {level} - {message}` shape, UTC second resolution) is installed on a dedicated `ConsoleHandler` and parent handlers are bypassed to avoid duplicate emission. When unset, the SDK does not touch the JVM logging config. +- §8 pre-flight rate-limit check: `HttpTransport.executeOnce` short-circuits with `RateLimitError` before acquiring a permit if the latest snapshot reports `remaining <= 0`. No-op for cold clients (snapshot starts null). - §6 sealed `MarketDataException` hierarchy with the 7 canonical subtypes and full support context (`requestId`, `requestUrl`, `statusCode`, `timestamp`, `exceptionType`) + `getSupportInfo()`. - §10 timeouts: `REQUEST_TIMEOUT = 99s` and `CONNECT_TIMEOUT = 2s` exposed as constants on `MarketDataClient`. Connect timeout is wired into the `HttpClient`; the per-request 99 s timeout is applied via `HttpRequest.Builder#timeout` in `HttpTransport.buildRequest`. - §12 concurrency: 50-permit `AsyncSemaphore` on `HttpTransport` with acquire/release wired around every dispatch. The custom semaphore replaces `java.util.concurrent.Semaphore` so `executeAsync` never parks the caller's thread on a full pool (ADR-007). @@ -82,9 +84,7 @@ The Java SDK must also satisfy the canonical, cross-language [SDK Requirements]( **Deliberately deferred (require the request/endpoint layer to land first):** - §1.2 resource groupings (`client.stocks`, `client.options`, `client.funds`, `client.markets`, `client.utilities`). - §2 endpoint method coverage; §3 universal parameters; §11 wire-format decoding. -- §5 actual `/user/` startup validation call (the `validateOnStartup` flag is the seam; the call itself comes with the request layer). -- §7 honoring `MARKETDATA_LOGGING_LEVEL` and the spec's exact `{timestamp} - {logger_name} - {level} - {message}` format. Currently the SDK uses `java.util.logging` with default formatting; consumers can attach their own handler. -- §8 rate-limit header parsing, pre-flight check, request-scoped attachment. +- §8 request-scoped attachment of rate-limit metadata to the response object (the client-level snapshot is wired; the per-response carrier is not). - §9 `/status/` cache workflow and `Retry-After` header override (retry/backoff itself lives in `RetryPolicy` and is wired; what is missing is the `/status/` pre-check before retrying 501–599 and respecting the server-specified `Retry-After` over the calculated exponential backoff). - §13 100% coverage threshold via JaCoCo `violationRules`; deferred until there is functional code worth the threshold. @@ -94,6 +94,30 @@ When picking up new work, check this list before reaching for the SDK requiremen - `HttpTransport.buildUri` URL-encodes query-param values with `URLEncoder.encode(..., UTF_8)`, which is form-encoding semantics: spaces become `+`, not `%20`. Fine for today's typed params (dates, numerics) but a future endpoint that takes an arbitrary string (e.g. `symbol="BRK A"`) would round-trip differently against an RFC-3986-strict server. Switch to a path/query-segment-aware encoder when the first such param lands. Tracked as Issue #10 of the 2026-05-11 review. - `Retry-After` server header is parsed and respected by neither `RetryPolicy` nor `HttpTransport`. Today every retry uses the calculated exponential backoff (`min(1s × 2^N, 30s)`). Implementing the override needs the response headers to reach `RetryPolicy.backoffDelay`, which today only sees the attempt index — most natural path is to surface a `Duration` on `ServerError` (or thread it through a separate channel) when 5xx responses carry the header. Follow-up of the §9 work. +## Test policy: unit tests are network-free + +Unit tests under `src/test/` **must not** make real network calls. Anything that needs the live +API goes to `src/integrationTest/` (gated by `MARKETDATA_RUN_INTEGRATION_TESTS=true`). + +This matters because §5 made `MarketDataClient`'s constructor perform a `GET /user/` when +`validateOnStartup=true` (the default). A test that builds `new MarketDataClient("any-token", +null, null, true)` now hits `api.marketdata.app` over the network — exactly the failure mode +this policy prevents. + +Concrete rules for unit tests that construct `MarketDataClient`: + +- Use the 4-arg constructor with `validateOnStartup=false` when verifying field wiring only. +- Use a local server (`com.sun.net.httpserver.HttpServer` in-process) and point `baseUrl` at it + when you genuinely need to exercise the validate-on-startup flow — see + `MarketDataClientStartupValidationTest` for the pattern. +- Tests that need the **real** API for smoke verification (e.g. the no-arg ctor end-to-end) go to + `src/integrationTest/.../MarketDataClientIT.java`. + +No automated enforcement today (a future ArchUnit rule or a grep step in CI could close that +gap). Code review is the gate. Whenever you introduce a test that touches `new +MarketDataClient(..., true)` with a default URL, ask: "does this need to live in `integrationTest` +instead?" + ## Acceptance checklist `docs/java-sdk-requirements.md` ends with an "Acceptance Checklist" mapping each Java-specific requirements section to verifiable items. Treat it as the definition of done for v1: when implementing, work toward making each box checkable, and use it as a self-review pass before declaring a section complete. diff --git a/src/integrationTest/java/com/marketdata/sdk/MarketDataClientIT.java b/src/integrationTest/java/com/marketdata/sdk/MarketDataClientIT.java new file mode 100644 index 0000000..375da53 --- /dev/null +++ b/src/integrationTest/java/com/marketdata/sdk/MarketDataClientIT.java @@ -0,0 +1,47 @@ +package com.marketdata.sdk; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Test; + +/** + * Integration smoke for {@link MarketDataClient} construction against the live Market Data API. + * Gated by the {@code integrationTest} source set, which itself only runs when {@code + * MARKETDATA_RUN_INTEGRATION_TESTS=true}. Requires a valid {@code MARKETDATA_TOKEN} via env or + * {@code .env} — without one the no-arg ctor falls back to demo mode and skips {@code /user/}, so + * the smoke would assert nothing real. + * + *

These assertions used to live in {@code MarketDataClientTest} as unit tests, but after §5 + * landed the no-arg ctor performs a real HTTP call ({@code GET /user/}) when {@code + * validateOnStartup=true}. Unit tests must not depend on the network; the smoke moved here so the + * real-API path stays exercised exactly once, under the integration gate. + */ +class MarketDataClientIT { + + @Test + void noArgConstructorAppliesProductionDefaults() { + try (var client = new MarketDataClient()) { + assertThat(client.isValidateOnStartup()) + .as( + "the no-arg ctor must be equivalent to `new MarketDataClient(null, null, null, true)`") + .isTrue(); + assertThat(client.getUserAgent()).startsWith("marketdata-sdk-java/"); + + // baseUrl/apiVersion fall back to the documented defaults only when the cascade has no + // override. CI typically has no overrides; locally a developer may set MARKETDATA_BASE_URL + // for a staging environment, so gate those assertions on the env vars being unset. + if (System.getenv("MARKETDATA_BASE_URL") == null) { + assertThat(client.getBaseUrl()).isEqualTo(Configuration.DEFAULT_BASE_URL); + } + if (System.getenv("MARKETDATA_API_VERSION") == null) { + assertThat(client.getApiVersion()).isEqualTo(Configuration.DEFAULT_API_VERSION); + } + + // After /user/ ran, the rate-limit snapshot must be seeded — that confirms the §5 wiring + // travels end-to-end (request fired, response headers parsed, snapshot stored). + assertThat(client.getRateLimits()) + .as("startup validation hit /user/ — snapshot should be seeded") + .isNotNull(); + } + } +} diff --git a/src/main/java/com/marketdata/sdk/HttpTransport.java b/src/main/java/com/marketdata/sdk/HttpTransport.java index 4549556..0a5767a 100644 --- a/src/main/java/com/marketdata/sdk/HttpTransport.java +++ b/src/main/java/com/marketdata/sdk/HttpTransport.java @@ -6,6 +6,7 @@ import com.marketdata.sdk.exception.MarketDataException; import com.marketdata.sdk.exception.NetworkError; import com.marketdata.sdk.exception.ParseError; +import com.marketdata.sdk.exception.RateLimitError; import com.marketdata.sdk.markets.MarketStatus; import java.io.IOException; import java.net.URI; @@ -120,6 +121,14 @@ private static HttpClient defaultHttpClient() { * RetryPolicy}: retries 501–599 and IOException-shaped {@link NetworkError}s with exponential * backoff, surfaces every other failure immediately. Cancellation of the returned future bails * out of any pending backoff and propagates to the current in-flight attempt. + * + *

Interaction between the §8 pre-flight check and the retry chain: each + * attempt re-runs the pre-flight check against the latest rate-limit snapshot. If a transient 5xx + * triggers a backoff and another concurrent caller's response drains the snapshot to {@code + * remaining=0} during that window, the next attempt's pre-flight fires and the chain ends with + * {@link com.marketdata.sdk.exception.RateLimitError} — not the upstream 5xx. The synthetic error + * reflects what the server would have returned anyway (a guaranteed 429 once the quota was gone), + * but callers that grep for "last server status" should know the substitution can happen. */ CompletableFuture executeAsync(RequestSpec spec, Class responseType) { CompletableFuture result = new CompletableFuture<>(); @@ -185,6 +194,25 @@ private void attempt( }); } + /** + * SDK requirements §5: hit {@code /user/} to confirm the token is valid. Used by {@link + * MarketDataClient}'s constructor when {@code validateOnStartup=true}. Single-attempt + * deliberately: a 3-attempt retry chain on the construction path would block the caller for + * 99s+1s+2s before reporting a bad token — worse UX than failing fast. The response body is + * irrelevant; what matters is that the status is 2xx (token valid) vs 401 (token invalid) vs a + * network exception. + */ + void validateToken() { + RequestSpec spec = RequestSpec.get("user").build(); + try { + executeOnce(spec, java.util.Map.class).join(); + } catch (CompletionException e) { + throw asRuntime(e.getCause()); + } catch (CancellationException e) { + throw asRuntime(e); + } + } + /** * Single-shot dispatch — one HTTP request, one permit lease, one response decode. Public retry * orchestration lives in {@link #executeAsync}. @@ -193,6 +221,26 @@ private CompletableFuture executeOnce(RequestSpec spec, Class response URI uri = buildUri(spec); HttpRequest request = buildRequest(uri); + // SDK requirements §8: pre-flight check. If the last response we saw left us with no + // credits, fail fast — there's no point burning a permit and an RTT for what the server + // will reject as 429 anyway. The snapshot can only be `null` until the first response + // seeds it, so this is a no-op for cold clients. + RateLimits snapshot = latestRateLimits.get(); + if (snapshot != null && snapshot.remaining() <= 0) { + // `reset == EPOCH` means the response carried partial rate-limit headers (e.g. + // remaining without reset) and `RateLimitHeaders.parse` defaulted the missing field to + // 0. Rendering "1970-01-01" in the user-facing message looks like a bug; omit the + // suffix when the value is meaningless. + String resetSuffix = + snapshot.reset().equals(java.time.Instant.EPOCH) + ? "" + : " (resets at " + snapshot.reset() + ")"; + return CompletableFuture.failedFuture( + new RateLimitError( + "Pre-flight rate-limit check failed: 0 credits remaining" + resetSuffix, + new ErrorContext(null, uri.toString(), null))); + } + // ADR-007: acquire returns a CompletableFuture instead of parking the caller's thread. // When permits are available the future is already completed (fast path) and thenCompose // runs synchronously; when the pool is exhausted the future completes later, on the diff --git a/src/main/java/com/marketdata/sdk/MarketDataClient.java b/src/main/java/com/marketdata/sdk/MarketDataClient.java index 85cc821..b09b1a4 100644 --- a/src/main/java/com/marketdata/sdk/MarketDataClient.java +++ b/src/main/java/com/marketdata/sdk/MarketDataClient.java @@ -1,6 +1,8 @@ package com.marketdata.sdk; import java.time.Duration; +import java.util.Locale; +import java.util.logging.Handler; import java.util.logging.Level; import java.util.logging.Logger; import org.jspecify.annotations.Nullable; @@ -57,7 +59,8 @@ public final class MarketDataClient implements AutoCloseable { * requirements §4 (env var → {@code .env} → built-in default) and enables startup validation. * *

Equivalent to {@link #MarketDataClient(String, String, String, boolean) new - * MarketDataClient(null, null, null, true)}. + * MarketDataClient(null, null, null, true)} — see that constructor's side-effects paragraph for + * the network IO performed when a token is present. */ public MarketDataClient() { this(null, null, null, true); @@ -68,6 +71,22 @@ public MarketDataClient() { * baseUrl}, and {@code apiVersion} may be {@code null} to defer to the cascade in §4 for that * single value. * + *

Side effects when {@code validateOnStartup=true}: the constructor performs + * a synchronous {@code GET /user/} HTTP request against {@code baseUrl} to verify the token. That + * means construction: + * + *

+ * + * Pass {@code false} for tests, latency-sensitive cold paths (e.g. serverless), or any context + * where a "pure" constructor without network IO is required. Demo mode (no token) also skips the + * call regardless of the flag. + * * @param apiKey explicit API token, or {@code null} to resolve from {@code MARKETDATA_TOKEN} → * {@code .env} → demo mode * @param baseUrl override the API base URL, or {@code null} to resolve to {@link @@ -75,8 +94,7 @@ public MarketDataClient() { * @param apiVersion override the API version segment, or {@code null} to resolve to {@link * Configuration#DEFAULT_API_VERSION} * @param validateOnStartup whether to validate the token on construction by calling {@code - * /user/} (SDK requirements §5). Pass {@code false} for short-lived runtimes where the - * startup hit is undesirable. + * /user/} (SDK requirements §5). See the side-effects paragraph above. */ public MarketDataClient( @Nullable String apiKey, @@ -84,6 +102,10 @@ public MarketDataClient( @Nullable String apiVersion, boolean validateOnStartup) { Configuration config = Configuration.loadFromProcess(); + // SDK requirements §7: apply MARKETDATA_LOGGING_LEVEL (if set) before any LOG.log call + // below, otherwise the INFO line for client initialization would be filtered out when the + // user expected FINE. + configureLogging(config); this.token = config.resolve(apiKey, EnvVars.TOKEN); this.baseUrl = trimTrailingSlash( @@ -109,8 +131,14 @@ public MarketDataClient( LOG.log(Level.FINE, "Token: {0}", Tokens.redact(this.token)); } - // SDK requirements §5: validate on startup by default. The actual - // /user/ call lands with the user resource; this flag is the seam. + // SDK requirements §5: validate the token at startup by hitting /user/. Skipped in demo + // mode (no token to validate) and when the caller opted out via the 4-arg constructor. + // Failure surfaces immediately as AuthenticationError (401) or NetworkError (unreachable + // server), so the caller never gets a half-constructed client. + if (this.validateOnStartup && !this.demoMode) { + this.transport.validateToken(); + LOG.log(Level.FINE, "Token validated against /user/."); + } } // --------------------------------------------------------------------- @@ -164,4 +192,91 @@ public void close() { private static String trimTrailingSlash(String url) { return url.endsWith("/") ? url.substring(0, url.length() - 1) : url; } + + // --------------------------------------------------------------------- + // SDK requirements §7 — logging level + formatter wiring + // --------------------------------------------------------------------- + + /** + * Name of the package-level logger that controls every {@code com.marketdata.sdk.*} logger + * (children inherit unless they install their own handler). + */ + static final String SDK_LOGGER_NAME = "com.marketdata.sdk"; + + /** + * Applies {@code MARKETDATA_LOGGING_LEVEL} (resolved through the config cascade) to the SDK's + * logger and installs a spec-shaped {@link MarketDataLogFormatter} on a dedicated handler if one + * isn't already installed. Idempotent — calling repeatedly only refreshes the level. + * + *

Package-private + static so the constructor wires it and tests can drive it directly with a + * synthetic {@link Configuration} without depending on real process env vars. + * + *

When the env var is unset (or blank, or malformed), the method is a no-op — the SDK inherits + * whatever {@code java.util.logging} configuration the host JVM has, which is what library code + * is expected to do. + * + *

Side effect when the env var is set: the SDK logger's {@code + * useParentHandlers} is flipped to {@code false}, so handlers attached to the root logger no + * longer receive {@code com.marketdata.sdk.*} records. That prevents double emission (root + * handler with the JVM default formatter + our handler with the spec format) but means consumers + * that previously intercepted all logs through a root handler will stop seeing SDK records. To + * re-route, attach the consumer's handler directly to the SDK logger: + * + * 

{@code
+   * Logger.getLogger("com.marketdata.sdk").addHandler(myHandler);
+   * }
+ * + *

Records emitted before this method completes use whatever formatter the JVM + * has by default, not {@link MarketDataLogFormatter}. Two paths produce such records: + * + *

+ * + * These edges are accepted: warnings still reach the user (default JVM level is {@code INFO} ≤ + * {@code WARNING}); they just don't carry the spec timestamp prefix. Spec-shaped output starts + * with the first record emitted after this method returns normally. + */ + static void configureLogging(Configuration config) { + String requestedLevel = config.resolve(null, EnvVars.LOGGING_LEVEL); + if (requestedLevel == null || requestedLevel.isBlank()) { + return; + } + Level parsed; + try { + parsed = Level.parse(requestedLevel.trim().toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException e) { + LOG.warning("Ignoring invalid MARKETDATA_LOGGING_LEVEL=\"" + requestedLevel + "\""); + return; + } + Logger sdkLogger = Logger.getLogger(SDK_LOGGER_NAME); + sdkLogger.setLevel(parsed); + if (!hasSdkHandler(sdkLogger)) { + Handler handler = new MarketDataConsoleHandler(); + handler.setLevel(parsed); + sdkLogger.addHandler(handler); + // Bypass the root logger's default handlers; they would otherwise re-emit each record + // with the JVM default formatter and produce duplicate, badly-shaped lines. + sdkLogger.setUseParentHandlers(false); + } else { + for (Handler h : sdkLogger.getHandlers()) { + if (h instanceof MarketDataConsoleHandler) { + h.setLevel(parsed); + } + } + } + } + + private static boolean hasSdkHandler(Logger logger) { + for (Handler h : logger.getHandlers()) { + if (h instanceof MarketDataConsoleHandler) { + return true; + } + } + return false; + } } diff --git a/src/main/java/com/marketdata/sdk/MarketDataConsoleHandler.java b/src/main/java/com/marketdata/sdk/MarketDataConsoleHandler.java new file mode 100644 index 0000000..f8f4c1b --- /dev/null +++ b/src/main/java/com/marketdata/sdk/MarketDataConsoleHandler.java @@ -0,0 +1,19 @@ +package com.marketdata.sdk; + +import java.util.logging.ConsoleHandler; + +/** + * Marker subclass of {@link ConsoleHandler} so {@link MarketDataClient#configureLogging} can tell + * its own handler apart from anything the host application or another library installed on the same + * logger. The previous discriminator inspected {@code getFormatter() instanceof + * MarketDataLogFormatter}, which is structurally accurate today but trips up the rare case of a + * consumer attaching {@code MarketDataLogFormatter} to a vanilla {@code ConsoleHandler}. + * + *

No behavior beyond {@link ConsoleHandler}; the type identity is the whole point. + */ +final class MarketDataConsoleHandler extends ConsoleHandler { + + MarketDataConsoleHandler() { + setFormatter(new MarketDataLogFormatter()); + } +} diff --git a/src/main/java/com/marketdata/sdk/MarketDataLogFormatter.java b/src/main/java/com/marketdata/sdk/MarketDataLogFormatter.java new file mode 100644 index 0000000..bc9ee45 --- /dev/null +++ b/src/main/java/com/marketdata/sdk/MarketDataLogFormatter.java @@ -0,0 +1,39 @@ +package com.marketdata.sdk; + +import java.time.Instant; +import java.time.ZoneId; +import java.time.format.DateTimeFormatter; +import java.util.logging.Formatter; +import java.util.logging.LogRecord; + +/** + * SDK requirements §7 log formatter. Produces lines shaped as {@code {timestamp} - {logger_name} - + * {level} - {message}}, e.g. + * + * 

+ * 2026-05-12T13:42:18Z - com.marketdata.sdk.MarketDataClient - INFO - Initialized SDK 0.1.0
+ *
+ * + *

Timestamps are in UTC ISO-8601 (second resolution) — the cross-language spec doesn't pick a + * timezone, so the universal one wins. If the spec ever mandates US/Eastern for logs the way it + * does for response dates, this is the only spot to change. + */ +final class MarketDataLogFormatter extends Formatter { + + private static final DateTimeFormatter TIMESTAMP_FORMAT = + DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss'Z'").withZone(ZoneId.of("UTC")); + + @Override + public String format(LogRecord record) { + String timestamp = TIMESTAMP_FORMAT.format(Instant.ofEpochMilli(record.getMillis())); + String name = record.getLoggerName() == null ? "(anonymous)" : record.getLoggerName(); + return timestamp + + " - " + + name + + " - " + + record.getLevel().getName() + + " - " + + formatMessage(record) + + System.lineSeparator(); + } +} diff --git a/src/test/java/com/marketdata/sdk/HttpTransportRetryTest.java b/src/test/java/com/marketdata/sdk/HttpTransportRetryTest.java index 002a44c..08f2fb1 100644 --- a/src/test/java/com/marketdata/sdk/HttpTransportRetryTest.java +++ b/src/test/java/com/marketdata/sdk/HttpTransportRetryTest.java @@ -273,6 +273,87 @@ void cancellationMidBackoffSkipsRemainingAttempts() throws Exception { assertThat(permits.queueLength()).isZero(); } + // ---------- §8 pre-flight rate-limit check ---------- + + /** + * Spec §8: "If credits_remaining <= 0, throw RateLimitError immediately (no request)." Once a + * response seeds the snapshot with {@code remaining = 0}, the next call must fail fast without + * dispatching anything — saves both the round trip and a guaranteed 429 from the server. + */ + @Test + void preflightFailsImmediatelyWhenRemainingIsZero() { + MultiResponseHttpClient client = + new MultiResponseHttpClient( + response( + 200, + "{\"value\":\"ok\"}", + Map.of( + "x-api-ratelimit-limit", "50000", + "x-api-ratelimit-remaining", "0", + "x-api-ratelimit-reset", "1735689600", + "x-api-ratelimit-consumed", "50000"))); + + HttpTransport transport = newTransport(client, fastPolicy(3)); + + // First call: snapshot is still null, request proceeds; response seeds remaining=0. + transport.executeSync(RequestSpec.get("ping").build(), Echo.class); + assertThat(client.callCount()).isEqualTo(1); + + // Second call: pre-flight check sees remaining=0 and fails fast without touching the wire. + assertThatThrownBy(() -> transport.executeSync(RequestSpec.get("ping").build(), Echo.class)) + .isInstanceOf(RateLimitError.class) + .hasMessageContaining("Pre-flight"); + + assertThat(client.callCount()) + .as("pre-flight must short-circuit before a second request hits the network") + .isEqualTo(1); + } + + @Test + void preflightMessageOmitsResetSuffixWhenResetIsEpoch() { + // Partial rate-limit headers: server returned `remaining` without a `reset`, so + // RateLimitHeaders.parse defaulted reset to Instant.EPOCH. Rendering "(resets at + // 1970-01-01T00:00:00Z)" in the user-facing message looks like a bug — verify we omit it. + MultiResponseHttpClient client = + new MultiResponseHttpClient( + response( + 200, + "{\"value\":\"ok\"}", + Map.of( + "x-api-ratelimit-limit", "50000", + "x-api-ratelimit-remaining", "0", + "x-api-ratelimit-consumed", "50000"))); + + HttpTransport transport = newTransport(client, fastPolicy(3)); + transport.executeSync(RequestSpec.get("ping").build(), Echo.class); + + assertThatThrownBy(() -> transport.executeSync(RequestSpec.get("ping").build(), Echo.class)) + .isInstanceOf(RateLimitError.class) + .hasMessage("Pre-flight rate-limit check failed: 0 credits remaining"); + } + + @Test + void preflightAllowsRequestWhenRemainingPositive() { + MultiResponseHttpClient client = + new MultiResponseHttpClient( + response( + 200, + "{\"value\":\"ok\"}", + Map.of( + "x-api-ratelimit-limit", "50000", + "x-api-ratelimit-remaining", "100", + "x-api-ratelimit-reset", "1735689600", + "x-api-ratelimit-consumed", "49900")), + response(200, "{\"value\":\"ok\"}", Map.of())); + + HttpTransport transport = newTransport(client, fastPolicy(3)); + + transport.executeSync(RequestSpec.get("ping").build(), Echo.class); + transport.executeSync(RequestSpec.get("ping").build(), Echo.class); + + assertThat(client.callCount()).isEqualTo(2); + } + // ---------- permits are still conserved across retries ---------- @Test diff --git a/src/test/java/com/marketdata/sdk/MarketDataClientLoggingTest.java b/src/test/java/com/marketdata/sdk/MarketDataClientLoggingTest.java new file mode 100644 index 0000000..9b873f6 --- /dev/null +++ b/src/test/java/com/marketdata/sdk/MarketDataClientLoggingTest.java @@ -0,0 +1,203 @@ +package com.marketdata.sdk; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.lang.reflect.Constructor; +import java.util.Arrays; +import java.util.Map; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.Logger; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Exercises {@link MarketDataClient#configureLogging(Configuration)} in isolation. Driving it via a + * synthetic {@link Configuration} avoids the JVM env-var dependency that would otherwise make these + * tests flaky between CI and local runs. + */ +class MarketDataClientLoggingTest { + + private Logger sdkLogger; + private Level previousLevel; + private boolean previousUseParent; + private Handler[] previousHandlers; + + @BeforeEach + void snapshotLoggerState() { + sdkLogger = Logger.getLogger(MarketDataClient.SDK_LOGGER_NAME); + previousLevel = sdkLogger.getLevel(); + previousUseParent = sdkLogger.getUseParentHandlers(); + previousHandlers = sdkLogger.getHandlers().clone(); + // Strip any handlers the previous test (or process-level setup) might have installed. + for (Handler h : previousHandlers) { + sdkLogger.removeHandler(h); + } + } + + @AfterEach + void restoreLoggerState() { + // Drop anything configureLogging added during the test. + for (Handler h : sdkLogger.getHandlers()) { + sdkLogger.removeHandler(h); + } + sdkLogger.setLevel(previousLevel); + sdkLogger.setUseParentHandlers(previousUseParent); + for (Handler h : previousHandlers) { + sdkLogger.addHandler(h); + } + } + + private static Configuration newConfig(Map systemEnv) { + try { + Constructor ctor = + Configuration.class.getDeclaredConstructor(Map.class, Map.class); + ctor.setAccessible(true); + return ctor.newInstance(systemEnv, Map.of()); + } catch (ReflectiveOperationException e) { + throw new IllegalStateException(e); + } + } + + // ---------- happy paths ---------- + + @Test + void appliesLevelFromEnvVar() { + MarketDataClient.configureLogging(newConfig(Map.of(EnvVars.LOGGING_LEVEL, "FINE"))); + + assertThat(sdkLogger.getLevel()).isEqualTo(Level.FINE); + assertThat(Arrays.stream(sdkLogger.getHandlers())) + .anyMatch(h -> h instanceof MarketDataConsoleHandler); + assertThat(sdkLogger.getUseParentHandlers()) + .as("our handler bypasses parent so we don't double-emit with the JVM default formatter") + .isFalse(); + } + + @Test + void normalizesLevelCaseInsensitively() { + MarketDataClient.configureLogging(newConfig(Map.of(EnvVars.LOGGING_LEVEL, "warning"))); + + assertThat(sdkLogger.getLevel()).isEqualTo(Level.WARNING); + } + + // ---------- idempotency ---------- + + @Test + void calledTwiceDoesNotDuplicateHandler() { + MarketDataClient.configureLogging(newConfig(Map.of(EnvVars.LOGGING_LEVEL, "FINE"))); + MarketDataClient.configureLogging(newConfig(Map.of(EnvVars.LOGGING_LEVEL, "WARNING"))); + + long sdkHandlers = + Arrays.stream(sdkLogger.getHandlers()) + .filter(h -> h instanceof MarketDataConsoleHandler) + .count(); + assertThat(sdkHandlers) + .as("second call should refresh the level, not add a second handler") + .isEqualTo(1); + assertThat(sdkLogger.getLevel()).isEqualTo(Level.WARNING); + // The handler's own level should track the latest call too. + Arrays.stream(sdkLogger.getHandlers()) + .filter(h -> h instanceof MarketDataConsoleHandler) + .forEach(h -> assertThat(h.getLevel()).isEqualTo(Level.WARNING)); + } + + // ---------- no-op paths ---------- + + @Test + void noOpWhenEnvVarUnset() { + Level beforeLevel = sdkLogger.getLevel(); + int beforeHandlers = sdkLogger.getHandlers().length; + + MarketDataClient.configureLogging(newConfig(Map.of())); + + assertThat(sdkLogger.getLevel()) + .as("no env var means we don't touch the logger config") + .isEqualTo(beforeLevel); + assertThat(sdkLogger.getHandlers()).hasSize(beforeHandlers); + } + + @Test + void noOpWhenEnvVarIsBlank() { + MarketDataClient.configureLogging(newConfig(Map.of(EnvVars.LOGGING_LEVEL, " "))); + + assertThat(Arrays.stream(sdkLogger.getHandlers())) + .noneMatch(h -> h instanceof MarketDataConsoleHandler); + } + + // ---------- end-to-end: env var → real log emission with spec shape ---------- + + /** + * Closes the loop between {@link MarketDataClient#configureLogging} and {@link + * MarketDataLogFormatter}. The other tests in this class verify the mechanics of configureLogging + * (level applied, handler installed) and {@code MarketDataLogFormatterTest} verifies the + * formatter shape in isolation — but neither alone catches a regression where the two stop + * composing (e.g. configureLogging starts using a different formatter, or the level filter blocks + * records the formatter would have rendered). + * + *

Strategy: let {@code configureLogging} install its handler, then add a parallel capturing + * handler that reuses the same {@link MarketDataLogFormatter} so we observe the same line that + * goes to stderr. Emit a record on a child of the SDK logger and assert the captured line matches + * the spec shape exactly. + */ + @Test + void emittedRecordsAreFormattedPerSpec() { + MarketDataClient.configureLogging(newConfig(Map.of(EnvVars.LOGGING_LEVEL, "FINE"))); + + java.util.logging.Handler installed = + Arrays.stream(sdkLogger.getHandlers()) + .filter(h -> h instanceof MarketDataConsoleHandler) + .findFirst() + .orElseThrow(() -> new AssertionError("configureLogging did not install its handler")); + + CapturingHandler capture = new CapturingHandler(); + capture.setFormatter(installed.getFormatter()); + capture.setLevel(Level.ALL); + sdkLogger.addHandler(capture); + + Logger child = Logger.getLogger("com.marketdata.sdk.example"); + child.fine("hello world"); + + assertThat(capture.formattedLines) + .as("end-to-end logging must produce the spec-mandated shape") + .anyMatch( + line -> + line.matches( + "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}Z" + + " - com\\.marketdata\\.sdk\\.example - FINE - hello world" + + java.util.regex.Pattern.quote(System.lineSeparator()))); + } + + /** + * Minimal {@link java.util.logging.Handler} that runs the configured formatter and stashes the + * rendered string. Tests assert against {@link #formattedLines}; the raw records are not exposed + * because the formatter is what we actually care about end-to-end. + */ + private static final class CapturingHandler extends java.util.logging.Handler { + final java.util.List formattedLines = new java.util.ArrayList<>(); + + @Override + public void publish(java.util.logging.LogRecord record) { + if (isLoggable(record)) { + formattedLines.add(getFormatter().format(record)); + } + } + + @Override + public void flush() {} + + @Override + public void close() {} + } + + @Test + void noOpAndWarnsWhenEnvVarIsInvalid() { + // Level.parse rejects unknown names; configureLogging swallows the IAE, logs a warning, + // and leaves the logger untouched. + MarketDataClient.configureLogging(newConfig(Map.of(EnvVars.LOGGING_LEVEL, "NOT_A_REAL_LEVEL"))); + + assertThat(Arrays.stream(sdkLogger.getHandlers())) + .as("invalid level must not install a handler — that would lie about the SDK's config") + .noneMatch(h -> h instanceof MarketDataConsoleHandler); + } +} diff --git a/src/test/java/com/marketdata/sdk/MarketDataClientStartupValidationTest.java b/src/test/java/com/marketdata/sdk/MarketDataClientStartupValidationTest.java new file mode 100644 index 0000000..fcf7f83 --- /dev/null +++ b/src/test/java/com/marketdata/sdk/MarketDataClientStartupValidationTest.java @@ -0,0 +1,155 @@ +package com.marketdata.sdk; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.marketdata.sdk.exception.AuthenticationError; +import com.marketdata.sdk.exception.NetworkError; +import com.marketdata.sdk.exception.ServerError; +import com.sun.net.httpserver.HttpHandler; +import com.sun.net.httpserver.HttpServer; +import java.io.IOException; +import java.net.InetSocketAddress; +import java.nio.charset.StandardCharsets; +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Exercises the §5 startup-validation path on {@link MarketDataClient}. Each test stands up an + * in-process server scripted to behave like the {@code /user/} endpoint under different conditions, + * then asserts the constructor's behavior. + */ +class MarketDataClientStartupValidationTest { + + private HttpServer server; + private final AtomicInteger requestCount = new AtomicInteger(); + + @BeforeEach + void startServer() throws IOException { + server = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + } + + @AfterEach + void stopServer() { + if (server != null) { + server.stop(0); + } + } + + private String baseUrl() { + return "http://127.0.0.1:" + server.getAddress().getPort(); + } + + private void route(int status, String body) { + HttpHandler handler = + exchange -> { + requestCount.incrementAndGet(); + byte[] bytes = body.getBytes(StandardCharsets.UTF_8); + exchange.getResponseHeaders().add("Content-Type", "application/json"); + exchange.sendResponseHeaders(status, bytes.length); + exchange.getResponseBody().write(bytes); + exchange.getResponseBody().close(); + }; + server.createContext("/", handler); + server.start(); + } + + // ---------- happy path ---------- + + @Test + void validatesTokenAtStartupWhenFlagIsTrue() { + route(200, "{\"s\":\"ok\",\"name\":\"test-user\"}"); + + try (var client = new MarketDataClient("valid-token", baseUrl(), null, true)) { + assertThat(client.isDemoMode()).isFalse(); + } + + assertThat(requestCount.get()).as("/user/ must be hit exactly once on startup").isEqualTo(1); + } + + // ---------- failure paths ---------- + + @Test + void surfaceAuthenticationErrorImmediatelyWhenTokenIsRejected() { + route(401, "{\"s\":\"error\",\"errmsg\":\"Invalid token\"}"); + + assertThatThrownBy(() -> new MarketDataClient("bogus-token", baseUrl(), null, true)) + .isInstanceOf(AuthenticationError.class) + .satisfies(t -> assertThat(((AuthenticationError) t).getStatusCode()).isEqualTo(401)); + + assertThat(requestCount.get()) + .as("startup validation must fail fast — no retry on auth errors") + .isEqualTo(1); + } + + /** + * Regression: {@code validateToken} must be single-attempt. {@code HttpTransport.executeAsync} + * normally retries 503 with exponential backoff, so if a refactor accidentally routes the startup + * probe through it the server would see 3 calls and (in this test) eventually succeed with a 200. + * Forcing the test to script enough responses to satisfy a 3-attempt chain — and then asserting + * {@code requestCount == 1} — fails determinístically the moment the path stops being + * single-shot. + */ + @Test + void validateTokenDoesNotRetryTransient5xx() { + route(503, "{}"); + + assertThatThrownBy(() -> new MarketDataClient("token", baseUrl(), null, true)) + .isInstanceOf(ServerError.class) + .satisfies(t -> assertThat(((ServerError) t).getStatusCode()).isEqualTo(503)); + + assertThat(requestCount.get()) + .as( + "validateToken must call executeOnce (not executeAsync) so a refactor that" + + " accidentally enables retry on the startup path is caught here") + .isEqualTo(1); + } + + @Test + void surfacesNetworkErrorWhenServerUnreachable() throws IOException { + // Bind+close an ephemeral port to get a "nothing is listening" URL. + int closedPort; + try (java.net.ServerSocket probe = + new java.net.ServerSocket(0, 0, java.net.InetAddress.getByName("127.0.0.1"))) { + closedPort = probe.getLocalPort(); + } + + assertThatThrownBy( + () -> new MarketDataClient("any-token", "http://127.0.0.1:" + closedPort, null, true)) + .isInstanceOf(NetworkError.class); + } + + // ---------- skipped paths ---------- + + @Test + void skipsValidationWhenFlagIsFalse() { + route(401, "{}"); // would normally trip AuthenticationError if called + + assertThatCode(() -> new MarketDataClient("any-token", baseUrl(), null, false).close()) + .doesNotThrowAnyException(); + + assertThat(requestCount.get()).as("validateOnStartup=false must not hit /user/").isZero(); + } + + @Test + void skipsValidationInDemoMode() { + // This test reaches demo mode by passing apiKey=null AND relying on the env cascade + // having no token. If the test environment exports MARKETDATA_TOKEN, the 4-arg + // constructor still resolves a real token — we can't synthetically force demo mode + // from the public API. + org.junit.jupiter.api.Assumptions.assumeTrue( + Configuration.loadFromProcess().resolve(null, EnvVars.TOKEN) == null, + "MARKETDATA_TOKEN present in env — can't exercise demo mode through the 4-arg ctor"); + + // Server is configured to fail every call, but the constructor must not even reach it. + route(500, "{}"); + + assertThatCode(() -> new MarketDataClient(null, baseUrl(), null, true).close()) + .doesNotThrowAnyException(); + + assertThat(requestCount.get()).as("demo-mode constructors must skip /user/ entirely").isZero(); + } +} diff --git a/src/test/java/com/marketdata/sdk/MarketDataClientTest.java b/src/test/java/com/marketdata/sdk/MarketDataClientTest.java index ca51fbd..8c1ff18 100644 --- a/src/test/java/com/marketdata/sdk/MarketDataClientTest.java +++ b/src/test/java/com/marketdata/sdk/MarketDataClientTest.java @@ -14,7 +14,10 @@ class MarketDataClientTest { @Test void buildsWithExplicitToken() { - try (var client = new MarketDataClient("test-key", null, null, true)) { + // validateOnStartup=false: this test verifies field-wiring on the explicit ctor, not the + // /user/ probe. The probe path is exercised end-to-end against an in-process server in + // MarketDataClientStartupValidationTest, and against the live API in MarketDataClientIT. + try (var client = new MarketDataClient("test-key", null, null, false)) { assertThat(client.isDemoMode()).isFalse(); assertThat(client.getBaseUrl()).isEqualTo(Configuration.DEFAULT_BASE_URL); assertThat(client.getApiVersion()).isEqualTo(Configuration.DEFAULT_API_VERSION); @@ -22,15 +25,17 @@ void buildsWithExplicitToken() { } @Test - void demoModeWhenNoTokenAvailable() { - // Demo mode iff the full cascade (env var → .env → null) yields nothing. Deriving the - // expectation from the same Configuration helper the constructor uses keeps the test - // valid both on CI (no token anywhere → demoMode) and locally (.env-supplied token → - // not demoMode); a plain `System.getenv` check would miss the .env source and break - // locally. - try (var client = new MarketDataClient()) { - boolean expectDemo = Configuration.loadFromProcess().resolve(null, EnvVars.TOKEN) == null; - assertThat(client.isDemoMode()).isEqualTo(expectDemo); + void demoModeWhenAllSourcesYieldNull() { + // Demo mode iff the full cascade (apiKey → env var → .env → null) yields nothing. We use + // the 4-arg ctor with validateOnStartup=false so the constructor never touches the network + // — the assertion is purely about cascade resolution. assumeTrue gates the test on the + // CI/local environment having no token; otherwise we couldn't reach demo mode without + // mocking env vars from inside a unit test. + org.junit.jupiter.api.Assumptions.assumeTrue( + Configuration.loadFromProcess().resolve(null, EnvVars.TOKEN) == null, + "MARKETDATA_TOKEN present in env — cannot exercise demo mode from a unit test"); + try (var client = new MarketDataClient(null, null, null, false)) { + assertThat(client.isDemoMode()).isTrue(); } } @@ -85,26 +90,10 @@ public void flush() {} public void close() {} } - @Test - void noArgConstructorAppliesProductionDefaults() { - // The no-arg constructor must be equivalent to `new MarketDataClient(null, null, null, - // true)` — production path with everything resolved from the cascade and startup - // validation enabled. validateOnStartup and the userAgent format are env-independent, - // so we assert them unconditionally; baseUrl/apiVersion fall back to the documented - // defaults only when the cascade has no override, so we gate those assertions on the - // env vars being unset (mirrors the demo-mode test above). - try (var client = new MarketDataClient()) { - assertThat(client.isValidateOnStartup()).isTrue(); - assertThat(client.getUserAgent()).startsWith("marketdata-sdk-java/"); - - if (System.getenv("MARKETDATA_BASE_URL") == null) { - assertThat(client.getBaseUrl()).isEqualTo(Configuration.DEFAULT_BASE_URL); - } - if (System.getenv("MARKETDATA_API_VERSION") == null) { - assertThat(client.getApiVersion()).isEqualTo(Configuration.DEFAULT_API_VERSION); - } - } - } + // `noArgConstructorAppliesProductionDefaults` (verifying the no-arg ctor end-to-end against + // the production defaults) now lives in `src/integrationTest/.../MarketDataClientIT.java`, + // because the post-§5 constructor hits /user/ when validateOnStartup=true and unit tests + // must not depend on real network. @Test void overridesAreHonored() { @@ -117,14 +106,17 @@ void overridesAreHonored() { @Test void userAgentMatchesSpec() { - try (var client = new MarketDataClient("KEY", null, null, true)) { + // validateOnStartup=false so the userAgent assertion doesn't depend on a real /user/ call. + try (var client = new MarketDataClient("KEY", null, null, false)) { assertThat(client.getUserAgent()).startsWith("marketdata-sdk-java/"); } } @Test void rateLimitsStartUnpopulated() { - try (var client = new MarketDataClient("KEY", null, null, true)) { + // validateOnStartup=false so the constructor does not hit /user/ and seed the snapshot; + // this test asserts the pre-network state of the client. + try (var client = new MarketDataClient("KEY", null, null, false)) { assertThat(client.getRateLimits()).isNull(); } } diff --git a/src/test/java/com/marketdata/sdk/MarketDataLogFormatterTest.java b/src/test/java/com/marketdata/sdk/MarketDataLogFormatterTest.java new file mode 100644 index 0000000..9e55e79 --- /dev/null +++ b/src/test/java/com/marketdata/sdk/MarketDataLogFormatterTest.java @@ -0,0 +1,55 @@ +package com.marketdata.sdk; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.logging.Level; +import java.util.logging.LogRecord; +import org.junit.jupiter.api.Test; + +class MarketDataLogFormatterTest { + + private final MarketDataLogFormatter formatter = new MarketDataLogFormatter(); + + @Test + void producesSpecShapedLine() { + // SDK requirements §7: `{timestamp} - {logger_name} - {level} - {message}`. + LogRecord record = new LogRecord(Level.INFO, "Initialized SDK"); + record.setLoggerName("com.marketdata.sdk.MarketDataClient"); + record.setMillis(1715000000000L); // 2024-05-06T12:53:20Z UTC + + String formatted = formatter.format(record); + + assertThat(formatted) + .isEqualTo( + "2024-05-06T12:53:20Z - com.marketdata.sdk.MarketDataClient - INFO - " + + "Initialized SDK" + + System.lineSeparator()); + } + + @Test + void rendersMessageWithMessageFormatPlaceholders() { + // java.util.logging supports `{0}`, `{1}`, ... placeholders. Our formatter delegates to + // formatMessage so those get substituted correctly. + LogRecord record = new LogRecord(Level.FINE, "Token: {0}"); + record.setLoggerName("com.marketdata.sdk.Tokens"); + record.setParameters(new Object[] {"***...***ABCD"}); + record.setMillis(1715000000000L); + + String formatted = formatter.format(record); + + assertThat(formatted).contains(" - FINE - Token: ***...***ABCD"); + } + + @Test + void replacesNullLoggerNameWithPlaceholder() { + // Anonymous Logger.getAnonymousLogger() records have a null name; rendering literal "null" + // would be uglier than "(anonymous)". + LogRecord record = new LogRecord(Level.WARNING, "no-name"); + record.setLoggerName(null); + record.setMillis(1715000000000L); + + String formatted = formatter.format(record); + + assertThat(formatted).contains(" - (anonymous) - WARNING - no-name"); + } +}