vlsi opened a new issue, #6728:
URL: https://github.com/apache/jmeter/issues/6728
## Summary
`TestBean` is a marker interface that means two unrelated things at once.
Elements can't opt into one without the other, which is what turned a simple
bug report (#6451) into an open-ended design question in #6726. This issue
proposes splitting it, and asks for a decision on how existing elements opt out
of the storage half.
## Background
Implementing `TestBean` currently buys an element two separate things:
1. **A generated GUI.** Write a `BeanInfo` and `TestBeanGUI` builds the
editor by introspection, with no Swing code in the element.
2. **Field-backed storage with a sync step.** `TestBeanHelper.prepare()`
copies the property map into the element's setters, so the element's plain Java
fields hold the values during a run.
The second is the source of a recurring surprise: the fields are only
populated after `prepare()`, so every TestBean getter returns `null` (or a
field initializer) on a tree that was just loaded from JMX. #6451 hit this with
`CSVDataSet.getFilename()`. It's working as designed, but the design is a trap,
and it applies to all 32 elements that implement `TestBean` — JSR223 elements,
`ConstantThroughputTimer`, `DataSourceElement`, and the rest.
The two jobs have nothing to do with each other. An element that stores its
configuration in the property map still wants a generated GUI; it just doesn't
want the field sync.
## The split is already there in the code
Every place the framework tests for `TestBean` uses it for exactly one of
the two jobs, and no call site needs both:
**GUI generation (5)**
- `TestBeanGUI:159` — guard that the class is a TestBean
- `GuiPackage:368` — instantiate `TestBeanGUI` for the element
- `MenuFactory:138`, `MenuFactory:209` — menu registration
- `JMeterTreeNode:145` — icon and label resolution
**Field-backed storage (3)**
- `TestBeanHelper:129` — entry guard for `prepare()`
- `TestBeanHelper:180` — recursion into nested `TestElementProperty`
- `StandardJMeterEngine:243` — prepare test listeners
## Proposal
Split the marker in two and have `TestBean` extend both. Names are open to
bikeshedding:
```java
public interface BeanInfoGui {} // "generate my GUI from my
BeanInfo"
public interface FieldBackedProperties {} // "copy my properties into my
setters before use"
public interface TestBean extends BeanInfoGui, FieldBackedProperties {}
```
Then narrow the eight call sites above to whichever half they actually mean.
## This is a no-op for existing code
Adding superinterfaces to an existing interface is binary compatible, and
because `TestBean` extends both, **every `instanceof` result stays identical**:
- `el instanceof TestBean` at `TestBeanHelper:129` becomes `el instanceof
FieldBackedProperties`. Any element implementing `TestBean` inherits it, so the
result is unchanged.
- `TestBean.class.isAssignableFrom(c)` at `GuiPackage:368` becomes
`BeanInfoGui.class.isAssignableFrom(c)`. Same.
All 32 in-tree elements and every third-party plugin keep working with no
source change. Both interfaces must stay method-free markers for this to hold.
So the split can land on its own merits, independently of anything else
discussed here.
## What it enables
New elements implement only `BeanInfoGui`: they get the generated GUI, store
their configuration in the property map, and their getters are valid at any
point in the lifecycle. No marker, no opt-out, no `prepare()` on the sampling
path.
That also unblocks the newer property editors (`JBooleanPropertyEditor` and
friends) and the modified-value gutter for these elements, since those bind to
schema property descriptors rather than to bean fields.
## The unresolved part: existing elements can't un-implement
An element like `CSVDataSet` has to keep implementing `TestBean` for
compatibility — user code may cast to it or test for it. Since `TestBean
extends FieldBackedProperties`, it inherits the storage half and Java gives us
no way to drop it. So migrating an existing element to property-backed storage
still needs an explicit opt-out. Options:
**A. A negative marker.** `interface NotFieldBacked {}`, checked alongside
the positive one in `TestBeanHelper.prepare()`. Visible in the element's
declaration, and its javadoc can scope it honestly as a legacy-compatibility
wart that new elements should never need. Cost: a double negative in the type
hierarchy.
**B. A BeanInfo attribute.** `beanDescriptor.setValue("propertyBacked",
TRUE)`, read once inside the existing `GOOD_PROPS` `ClassValue` in
`TestBeanHelper`, so zero per-call cost. The "cannot un-implement" problem
disappears because this is metadata rather than a type, and the decision sits
with the element's other bean metadata. Cost: stringly-typed and less visible
when reading the element class.
**C. Neither** — accept that existing elements stay field-backed forever,
and property-backed storage is only for new elements. Cheapest, but leaves the
#6451 trap in place for all 32.
I lean toward B, precisely because "cannot un-implement an interface" is a
permanent Java constraint and metadata sidesteps it entirely. I don't feel
strongly.
## Decisions requested
These are independent; the first can land without settling the second.
1. Do we split `TestBean` as described?
2. If we do, which opt-out mechanism for existing elements — A, B, or C?
And if the answer to 2 is A or B, two semantics questions need settling
once, project-wide, rather than being rediscovered per element:
- Should a getter return `null` or the schema default for an unset property?
Property-backed getters return the default, which differs from today's field
behavior before `prepare()`.
- Function properties: today `prepare()` freezes `${...}` into a field at
compile time; property-backed access re-evaluates per read, using
`FunctionProperty`'s per-iteration cache. The latter looks more correct, but it
is an observable change.
## Origin
Came out of #6451 (reported as a `CSVDataSet.getFilename()` bug, closed as
works-as-designed) and #6726, where @e345ee implemented property-backed storage
for `CSVDataSet` with a `PropertyBackedTestBean` marker. That PR is a working
reference implementation of the migration; it's on hold pending this decision.
--
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]
