This is an automated email from the ASF dual-hosted git repository. jamesbognar pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/juneau.git
commit f1dfad875c1c45fd01d46f971290c50fac825267 Author: James Bognar <[email protected]> AuthorDate: Fri May 8 12:09:55 2026 -0400 refactor(inject): TODO-15 phase-3 batch 4 — widen RestSession / VarResolver field types to v2 Widen field types and accessor return types across `VarResolver`, `VarResolverSession`, `RestSession`, and `RestOpSession` from legacy `BasicBeanStore` to v2 `WritableBeanStore`. Two `BasicBeanStore.of((BasicBeanStore) X)` calls migrated to `new BasicBeanStore2(X)` along the way. Build + unit tests + jetty-ftest verified green. Specific changes: * `VarResolver.beanStore` (package-private field) — `BasicBeanStore` → `WritableBeanStore`. Constructor body line 260 migrated from `BasicBeanStore.of((BasicBeanStore) builder.beanStore())` to `new BasicBeanStore2(builder.beanStore())`. * `VarResolver.createSession(BasicBeanStore)` (public method) — parameter type widened to `WritableBeanStore`. Single caller in tree (`RestRequest.java:1395`) compiles unchanged. * `VarResolverSession.beanStore` (private field) + ctor parameter — both flipped to `WritableBeanStore`. Constructor body line 153 migrated from `BasicBeanStore.of(beanStore)` to `new BasicBeanStore2(beanStore)`. * `RestSession.beanStore` (private field) — `BasicBeanStore` → `WritableBeanStore`. Constructor body line 240 migrated from `BasicBeanStore.of((BasicBeanStore) context.getBeanStore()).addBean(...)` to `new BasicBeanStore2(context.getBeanStore()).addBean(...)`. * `RestSession.getBeanStore()` (public getter) — return type widened to `WritableBeanStore`. **Public-API breaking change** for external consumers expecting `BasicBeanStore`. * `RestOpSession.getBeanStore()` (public getter) — return type widened to `WritableBeanStore` to match its delegation target. **Public-API breaking change.** Net effect: 2 `(BasicBeanStore)` casts dropped from production code (`VarResolver.java:260`, `RestSession.java:240`); 4 field/getter type widenings landed. DEEPER BLOCKER DISCOVERED — `RestContext.java:1199` migration NOT included. Initial attempt also migrated `RestContext.java:1199` (`bs = new BasicBeanStore2(null, bootstrapBeanStore)`); this caused 19 jetty-ftest failures with `IllegalStateException: VarResolver not found`. Root cause: `RestContext.java:584` calls `BeanCreator.of(DebugEnablement.class, bs).type(BasicDebugEnablement.class)` which constructs `BasicDebugEnablement(BasicBeanStore beanStore)` — a constructor whose parameter type requires legacy `BasicBeanStore`. With `bs` as `BasicBeanStore2`, parameter resolution walks the parent chain and falls through to `bootstrapBeanStore` (legacy, self-registered as `BasicBeanStore.class`) which does NOT have the per-resource framework default suppliers → `init(...)` lookup of `VarResolver.class` fails. Reverted the `1199` migration with an explanatory comment in-source. Migration of that line requires either (a) flipping `BasicDebugEnablement(BasicBeanStore)` ctor + `DebugEnablement(BasicBeanStore)` superclass ctor + `init(BasicBeanStore)` template method to take `BeanStore`/`WritableBeanStore` (invasive — breaks user subclass overrides), or (b) deferring to Phase 4 cutover when `BasicBeanStore2` is renamed to `BasicBeanStore` and absorbs legacy ctor signatures. A related concern: `RestSession.beanStore` field widening implies `RestSession.getBeanStore()` now returns a `BasicBeanStore2` instance, which would break user-facing `@RestPostCall void hook(BasicBeanStore bs)` parameter resolution via `RestOpSessionArgs.create()` (line 46). No current test exercises that pattern, so the widening landed without test failures, but external consumers using `BasicBeanStore`-typed lifecycle hook parameters WILL break. Documented in TODO-15 §"Deeper blocker discovered". TODO-15 plan updated with batch-4 record, deeper-blocker analysis, and revised residual table (post-batch-4 remaining sites: `RestContext.java:348` + `RestContext.java:1199` + `RestOpContext.java:1097`). Co-authored-by: Cursor <[email protected]> --- .../java/org/apache/juneau/svl/VarResolver.java | 10 ++++----- .../org/apache/juneau/svl/VarResolverSession.java | 9 ++++---- .../java/org/apache/juneau/rest/RestContext.java | 14 +++++++----- .../java/org/apache/juneau/rest/RestOpSession.java | 3 ++- .../java/org/apache/juneau/rest/RestSession.java | 7 +++--- todo/TODO-15-replace-basicbeanstore-with-v2.md | 26 +++++++++++++++++----- 6 files changed, 46 insertions(+), 23 deletions(-) diff --git a/juneau-core/juneau-marshall/src/main/java/org/apache/juneau/svl/VarResolver.java b/juneau-core/juneau-marshall/src/main/java/org/apache/juneau/svl/VarResolver.java index 59bce6707b..2c861d9325 100644 --- a/juneau-core/juneau-marshall/src/main/java/org/apache/juneau/svl/VarResolver.java +++ b/juneau-core/juneau-marshall/src/main/java/org/apache/juneau/svl/VarResolver.java @@ -23,7 +23,9 @@ import java.util.*; import java.util.concurrent.*; import org.apache.juneau.*; +import org.apache.juneau.commons.inject.BasicBeanStore2; import org.apache.juneau.commons.inject.BeanStore; +import org.apache.juneau.commons.inject.WritableBeanStore; import org.apache.juneau.cp.*; import org.apache.juneau.svl.vars.*; @@ -240,7 +242,7 @@ public class VarResolver { final Var[] vars; private final Map<String,Var> varMap; - final BasicBeanStore beanStore; + final WritableBeanStore beanStore; /** * Constructor. @@ -255,9 +257,7 @@ public class VarResolver { m.put(v.getName(), v); this.varMap = u(m); - // Legacy BasicBeanStore.of(...) static still requires a BasicBeanStore parent; downcast preserves - // existing semantics. Will go away when BasicBeanStore.of(...) is widened or removed. - this.beanStore = BasicBeanStore.of((BasicBeanStore) builder.beanStore()); + this.beanStore = new BasicBeanStore2(builder.beanStore()); } /** @@ -297,7 +297,7 @@ public class VarResolver { * @param beanStore The bean store to associate with this session. * @return A new resolver session. */ - public VarResolverSession createSession(BasicBeanStore beanStore) { + public VarResolverSession createSession(WritableBeanStore beanStore) { return new VarResolverSession(this, beanStore); } diff --git a/juneau-core/juneau-marshall/src/main/java/org/apache/juneau/svl/VarResolverSession.java b/juneau-core/juneau-marshall/src/main/java/org/apache/juneau/svl/VarResolverSession.java index 0de948c1bf..d84fd045a8 100644 --- a/juneau-core/juneau-marshall/src/main/java/org/apache/juneau/svl/VarResolverSession.java +++ b/juneau-core/juneau-marshall/src/main/java/org/apache/juneau/svl/VarResolverSession.java @@ -29,6 +29,7 @@ import java.util.*; import org.apache.juneau.commons.collections.*; import org.apache.juneau.commons.lang.*; +import org.apache.juneau.commons.inject.*; import org.apache.juneau.cp.*; /** @@ -42,7 +43,7 @@ import org.apache.juneau.cp.*; * * <p> * Instances of this class are created through the {@link VarResolver#createSession()} and - * {@link VarResolver#createSession(BasicBeanStore)} methods. + * {@link VarResolver#createSession(WritableBeanStore)} methods. * * <h5 class='section'>Notes:</h5><ul> * <li class='warn'>This class is not guaranteed to be thread safe. @@ -137,7 +138,7 @@ public class VarResolverSession { private final VarResolver context; - private final BasicBeanStore beanStore; + private final WritableBeanStore beanStore; /** * Constructor. @@ -148,9 +149,9 @@ public class VarResolverSession { * @param beanStore The bean store to use for resolving beans needed by vars. * */ - public VarResolverSession(VarResolver context, BasicBeanStore beanStore) { + public VarResolverSession(VarResolver context, WritableBeanStore beanStore) { this.context = context; - this.beanStore = BasicBeanStore.of(beanStore); + this.beanStore = new BasicBeanStore2(beanStore); } /** diff --git a/juneau-rest/juneau-rest-server/src/main/java/org/apache/juneau/rest/RestContext.java b/juneau-rest/juneau-rest-server/src/main/java/org/apache/juneau/rest/RestContext.java index 698777119f..ee1860cda8 100644 --- a/juneau-rest/juneau-rest-server/src/main/java/org/apache/juneau/rest/RestContext.java +++ b/juneau-rest/juneau-rest-server/src/main/java/org/apache/juneau/rest/RestContext.java @@ -1196,11 +1196,15 @@ public class RestContext extends Context { // If no parent store, promote bs to bootstrap and layer a fresh per-resource store on top. // NOTE: Stays on legacy `BasicBeanStore.create()...` chain (vs `new BasicBeanStore2(null, X)`) - // because downstream consumers (`RestSession.java:240`, `VarResolver.java:260`) still - // downcast `RestContext.getBeanStore()` / `builder.beanStore()` to `BasicBeanStore` to call - // the legacy `BasicBeanStore.of(...)` static. Migrating those sites requires either - // widening the field types of those consumers or porting `BasicBeanStore.of(...)` onto - // `BasicBeanStore2` (Phase 4). + // because (1) `BeanCreator` resolves `BasicBeanStore`-typed constructor params (e.g. + // `BasicDebugEnablement(BasicBeanStore)`) by walking the parent chain — a `BasicBeanStore2` + // here does not satisfy that lookup, and the walk falls through to `bootstrapBeanStore` + // (legacy, self-registered as `BasicBeanStore.class`) which does NOT have the + // per-resource framework default suppliers, breaking init() Bean lookups; and (2) user-facing + // `@RestPostCall void hook(BasicBeanStore bs)` parameters are resolved via `RestOpSessionArgs` + // which propagates this same bean store. Migration deferred until either + // `BasicDebugEnablement` / `BeanCreator` constructor-param resolution moves off legacy + // `BasicBeanStore`, or Phase 4 cutover absorbs the legacy statics into v2. if (parentBs == null) { bootstrapBeanStore = bs; bs = BasicBeanStore.create().overridingParent((BasicBeanStore) bootstrapBeanStore).build(); diff --git a/juneau-rest/juneau-rest-server/src/main/java/org/apache/juneau/rest/RestOpSession.java b/juneau-rest/juneau-rest-server/src/main/java/org/apache/juneau/rest/RestOpSession.java index 276db0738c..29579698f3 100644 --- a/juneau-rest/juneau-rest-server/src/main/java/org/apache/juneau/rest/RestOpSession.java +++ b/juneau-rest/juneau-rest-server/src/main/java/org/apache/juneau/rest/RestOpSession.java @@ -23,6 +23,7 @@ import java.io.*; import org.apache.http.*; import org.apache.juneau.*; import org.apache.juneau.commons.collections.FluentMap; +import org.apache.juneau.commons.inject.*; import org.apache.juneau.cp.*; import org.apache.juneau.http.response.*; import org.apache.juneau.rest.logger.*; @@ -160,7 +161,7 @@ public class RestOpSession extends ContextSession { * * @return The bean store for this session. */ - public BasicBeanStore getBeanStore() { return session.getBeanStore(); } + public WritableBeanStore getBeanStore() { return session.getBeanStore(); } @Override /* Overridden from ContextSession */ public RestOpContext getContext() { return ctx; } diff --git a/juneau-rest/juneau-rest-server/src/main/java/org/apache/juneau/rest/RestSession.java b/juneau-rest/juneau-rest-server/src/main/java/org/apache/juneau/rest/RestSession.java index 2ac3fbcf1f..8f50ecc25a 100644 --- a/juneau-rest/juneau-rest-server/src/main/java/org/apache/juneau/rest/RestSession.java +++ b/juneau-rest/juneau-rest-server/src/main/java/org/apache/juneau/rest/RestSession.java @@ -26,6 +26,7 @@ import java.util.*; import org.apache.http.*; import org.apache.juneau.*; import org.apache.juneau.commons.collections.FluentMap; +import org.apache.juneau.commons.inject.*; import org.apache.juneau.commons.lang.*; import org.apache.juneau.cp.*; import org.apache.juneau.http.response.*; @@ -215,7 +216,7 @@ public class RestSession extends ContextSession { } private final long startTime = System.currentTimeMillis(); - private final BasicBeanStore beanStore; + private final WritableBeanStore beanStore; private CallLogger logger; private HttpServletRequest req; private HttpServletResponse res; @@ -237,7 +238,7 @@ public class RestSession extends ContextSession { super(builder); context = builder.ctx; resource = builder.resource; - beanStore = BasicBeanStore.of((BasicBeanStore) context.getBeanStore()).addBean(RestContext.class, context); + beanStore = new BasicBeanStore2(context.getBeanStore()).addBean(RestContext.class, context); logger = beanStore.add(CallLogger.class, builder.logger); pathInfoUndecoded = builder.pathInfoUndecoded; @@ -303,7 +304,7 @@ public class RestSession extends ContextSession { * * @return The bean store of this call. */ - public BasicBeanStore getBeanStore() { return beanStore; } + public WritableBeanStore getBeanStore() { return beanStore; } /** * Returns the context that created this call. diff --git a/todo/TODO-15-replace-basicbeanstore-with-v2.md b/todo/TODO-15-replace-basicbeanstore-with-v2.md index 55d0a2f5bb..b1721815b6 100644 --- a/todo/TODO-15-replace-basicbeanstore-with-v2.md +++ b/todo/TODO-15-replace-basicbeanstore-with-v2.md @@ -14,15 +14,31 @@ Eliminate the legacy injection stack in `org.apache.juneau.cp` (`BasicBeanStore` - ~~`BeanBuilder<T>` widening to `WritableBeanStore`~~ — **DONE (2026-05-08).** Field, constructor, and public `beanStore()` accessor all flipped from `BasicBeanStore` to `WritableBeanStore` (chosen over read-only `BeanStore` so callers retaining write access — e.g. `VarResolver.Builder.bean(...)` calling `super.beanStore().addBean(...)` — keep working). All 14 cascade-builder `super(X.class, (BasicBeanStore) beanStore)` casts dropped. Two standalone Builders (`SwaggerProvider.Builder.be [...] - ~~Four-memoizer migration to `BeanInstantiator` (Phase 3 sub-task)~~ — **ATTEMPTED + REVERTED (2026-05-08).** `callLogger` / `debugEnablement` / `staticFiles` / `swaggerProvider` were migrated to `BeanInstantiator.of(...).beanSubType(...).run()`; this broke documented `@Rest(callLogger=…)` / `@Rest(debugEnablement=…)` / `@Rest(swaggerProvider=…)` annotation overrides (3 `Rest_BeanCreatorOverrides_Test` failures) plus `juneau-examples-rest-jetty-ftest` (static files no longer served — ` [...] - ~~Phase 3 batch-3: drop vestigial `(BasicBeanStore)` casts at framework-defaults call sites~~ — **DONE (2026-05-08).** Sixteen `(BasicBeanStore) bs` casts in `RestContext.java` (lines 494, 515, 584, 676, 753, 768, 914, 937, 957, 987, 1008, 1029, 1064, 1110, 1125) and one in `RestOpContext.java` (line 437) dropped. These were vestigial leftovers from before the utility-class `create(...)` factories were widened to `WritableBeanStore` in the cascade-builder work. The cast was no longer r [...] +- ~~Phase 3 batch-4: widen field types + getter return types in `RestSession` / `RestOpSession` / `VarResolver` / `VarResolverSession`~~ — **DONE (2026-05-08).** Field types and accessor return types widened from legacy `BasicBeanStore` to v2 `WritableBeanStore`; corresponding `BasicBeanStore.of((BasicBeanStore) X)` calls migrated to `new BasicBeanStore2(X)`: `VarResolver.beanStore` (field) + `VarResolver.createSession(BasicBeanStore)` (public method param) + `VarResolverSession.beanStor [...] -**Current remaining footprint (as of 2026-05-08, post-batch-3):** +### Deeper blocker discovered (2026-05-08, batch 4) + +Migrating `RestContext.java:1199` (`bs = BasicBeanStore.create().overridingParent((BasicBeanStore) bootstrapBeanStore).build()` → `bs = new BasicBeanStore2(null, bootstrapBeanStore)`) was attempted in batch 4 and reverted because of a runtime resolution issue downstream: + +* `RestContext.java:584` calls `BeanCreator.of(DebugEnablement.class, bs).type(BasicDebugEnablement.class).run()`. +* `BeanCreator` constructs `BasicDebugEnablement(BasicBeanStore beanStore)` — the constructor signature requires legacy `BasicBeanStore`. +* Parameter resolution walks the bean store chain looking for `BasicBeanStore.class`. With `bs` as `BasicBeanStore2`, the walk falls through to `bootstrapBeanStore` (legacy, self-registered as `BasicBeanStore.class` at construction). +* `BasicDebugEnablement(bootstrapBeanStore)` then calls `init(bootstrapBeanStore)` which does `bootstrapBeanStore.getBean(VarResolver.class)` — but `VarResolver` default suppliers were registered on the per-resource `BasicBeanStore2`, not on `bootstrapBeanStore`. → `IllegalStateException: VarResolver not found`. + +To unlock `RestContext.java:1199`, either: +1. **Migrate `BasicDebugEnablement(BasicBeanStore)` ctor** to take `BeanStore` / `WritableBeanStore` (and similar for any other class with `BasicBeanStore`-typed constructor). This propagates to `DebugEnablement(BasicBeanStore)` superclass ctor and the `init(BasicBeanStore)` template method — invasive, breaks user subclass overrides. +2. **Defer to Phase 4 cutover** when `BasicBeanStore2` is renamed to `BasicBeanStore` and the v2 absorbs legacy ctor signatures. + +Same blocker applies to `RestSession.beanStore`-derived flows when a user method declares `@RestPostCall void hook(BasicBeanStore bs)` — currently no test exercises that pattern, so the field widening landed without test failures, but external consumers would break. `RestOpSessionArgs.create()` line 46 (`paramInfo.isType(BasicBeanStore.class)`) is the resolution site and would need to be updated alongside any ctor migration. + +**Current remaining footprint (as of 2026-05-08, post-batch-4):** | Location | Legacy symbol | Nature | |---|---|---| -| `RestContext.java` | `BasicBeanStore`, `BeanCreator` | 6 `BeanCreator.of(...)` call sites (4 memoizers + user-child-resource path + `findRestOperationArgs`) — **all cast-free** thanks to `BeanCreator.of(Class, BeanStore)`; staying on legacy `BeanCreator` until **TODO-25** lands. Plus 2 direct legacy Builder-API calls (`BasicBeanStore.create().overridingParent((BasicBeanStore) X).build()` lines 348, 1206) using the legacy static API. | +| `RestContext.java` | `BasicBeanStore`, `BeanCreator` | 6 `BeanCreator.of(...)` call sites (4 memoizers + user-child-resource path + `findRestOperationArgs`) — **all cast-free** thanks to `BeanCreator.of(Class, BeanStore)`; staying on legacy `BeanCreator` until **TODO-25** lands. Plus 2 direct legacy Builder-API calls (`BasicBeanStore.create().overridingParent((BasicBeanStore) X).build()` lines 348, 1199) — line 1199 blocked by `BasicDebugEnablement(BasicBeanStore)` ctor (see "Deeper bl [...] | `RestOpContext.java` | `BasicBeanStore`, `BeanCreator` | 1 `BasicBeanStore.of((BasicBeanStore) context.getBootstrapBeanStore())` (line 1097) — registers the result as `BasicBeanStore.class` for user-facing `@RestPostCall`/`@RestStartCall` hook param resolution; cannot migrate without breaking that contract. + `BeanCreator.of(HttpPartSerializer.class).type(c)` for `partSerializer` (no cast — uses no-arg overload). | -| `RestSession.java` | `cp.BasicBeanStore` | 1 `BasicBeanStore.of((BasicBeanStore) context.getBeanStore())` site (line 240); requires widening private `beanStore` field + public `getBeanStore()` return type to migrate. | -| `VarResolver.java` | `cp.BasicBeanStore` | 1 `BasicBeanStore.of((BasicBeanStore) builder.beanStore())` site (line 260); requires widening package-private field + public `createSession(BasicBeanStore)` method. | +| ~~`RestSession.java`~~ | ~~`cp.BasicBeanStore`~~ | **RESOLVED (2026-05-08, batch 4).** Field + `getBeanStore()` widened to `WritableBeanStore`; site migrated to `new BasicBeanStore2(...)`. | +| ~~`VarResolver.java`~~ | ~~`cp.BasicBeanStore`~~ | **RESOLVED (2026-05-08, batch 4).** Field + `createSession(...)` parameter widened; site migrated to `new BasicBeanStore2(...)`. | | ~~`BeanBuilder<T>` cascade builders~~ | ~~`BasicBeanStore` cast in `super(...)`~~ | **RESOLVED (2026-05-08).** All 14 cascade-builder casts dropped after widening `BeanBuilder<T>` to accept `WritableBeanStore`. | | ~~`RestInject.java`, `RestInit.java`~~ | ~~`cp.BasicBeanStore`~~ | **RESOLVED (2026-05-08, batch 2).** Javadoc-only refs flipped to v2 `BeanStore`. | | ~~`McpPage.java`, `McpTypedHandlers.java`, `McpEndpoint.java`, `McpRestServlet.java`~~ | ~~`cp.BasicBeanStore`~~ | **RESOLVED (2026-05-08, batch 2).** rest-server-mcp module fully migrated to v2 `BeanStore`. | @@ -38,7 +54,7 @@ Eliminate the legacy injection stack in `org.apache.juneau.cp` (`BasicBeanStore` 3. **`BeanInstantiator` four-memoizer migration blocker.** Migrating `callLogger` / `debugEnablement` / `staticFiles` / `swaggerProvider` to `BeanInstantiator` broke documented `@Rest(callLogger=…)` / `@Rest(debugEnablement=…)` / `@Rest(swaggerProvider=…)` annotation overrides plus static-file serving (loose-builder fallthrough still picks empty-default builders for `BasicStaticFiles` etc.; `Basic*.init(BeanStore)` zero-outs builder defaults via `.orElse(null)` when optional beans aren't [...] -4. **Direct `BasicBeanStore.of(...)` / `BasicBeanStore.create().overridingParent(...)` calls (partially resolved 2026-05-08).** Five transient internal sites in `RestOpContext` (4 lambdas) and `RestContext.findRestOperationArgs` migrated to `new BasicBeanStore2(beanStore())`. Four sites remain blocked: `RestContext.java:348` (Builder fluent chain with `.type(...)`/`.impl(...)` — needs equivalent v2 fluent API), `RestContext.java:1206` (per-resource bean store re-wrap exposed via public ` [...] +4. **Direct `BasicBeanStore.of(...)` / `BasicBeanStore.create().overridingParent(...)` calls (mostly resolved 2026-05-08).** Five transient internal sites in `RestOpContext` (4 lambdas) and `RestContext.findRestOperationArgs` migrated to `new BasicBeanStore2(beanStore())` in batch 3. `RestSession.java:240` + `VarResolver.java:260` migrated alongside field-type widening in batch 4. Two sites remain blocked: `RestContext.java:348` (Builder fluent chain with `.type(...)`/`.impl(...)` — need [...] 5. ~~**`BeanBuilder<T>` parent-ctor signature.**~~ — **RESOLVED (2026-05-08).** `BeanBuilder<T>` field + ctor + public `beanStore()` accessor all flipped to `WritableBeanStore`. All 14 cascade-builder casts dropped.
