Fix CSVDataSet filename getter after loading test plan

[e345ee]

Description

This PR fixes CSVDataSet.getFilename() when a test plan is loaded through SaveService.loadTree().

CSVDataSet keeps TestBean values in transient fields during normal execution, while loaded JMX values are restored into the element property map. Because of that, getFilename() could return null right after loading a test plan, even though the filename property was already present.

The change keeps the existing runtime behavior, but makes getFilename() fall back to the stored filename property when the transient field has not been populated yet.

Motivation and Context

Fixes #6451.

I ran into this while working with JMeter test plans programmatically. After loading a JMX file, CSVDataSet already has the filename value in its property map, but getFilename() still returns null until TestBean preparation runs.

That makes it harder to inspect or validate loaded test plans, so this PR makes the getter return the already-loaded value in that case.

How Has This Been Tested?

Tested locally with:

• ./gradlew :src:components:test --tests org.apache.jmeter.config.TestCVSDataSet.testFilenameGetterAfterLoadingTestPlan -Djava.awt.headless=true
• ./gradlew :src:components:test --tests org.apache.jmeter.config.TestCVSDataSet -Djava.awt.headless=true
• ./gradlew :src:components:checkstyleMain :src:components:checkstyleTest
• git diff --check

The new regression test loads a minimal JMX file with SaveService.loadTree(), finds the CSVDataSet, and verifies that both the raw filename property and getFilename() return the expected value.

Screenshots (if appropriate):

Not applicable.

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code follows the code style of this project.
• I have updated the documentation accordingly.

[milamberspace]

Nice, well-scoped fix — thanks @e345ee. The diagnosis is correct: CSVDataSet's TestBean transient fields aren't populated until TestBeanHelper.prepare(), so after SaveService.loadTree() the filename field is still null while the property already holds the value; falling back to getPropertyOrNull("filename") is the right minimal fix and doesn't affect the runtime path (at lines 187/197 the field is already populated). The regression test that loads a real JMX via SaveService.loadTree() is exactly what this needs. 👍

The code looks good to me — one thing to do before merge, which is why I'm leaving this as a comment rather than an approval:

• Add a xdocs/changes.xml entry under Bug fixes, and add yourself under Thanks. Something like:

  <li><pr>6726</pr><issue>6451</issue>Fix CSVDataSet#getFilename() returning null right after loading a test plan via SaveService.loadTree(). Contributed by Sadovoi Grigorii (github.com/e345ee)</li>

  (The PR checklist ticks "I have updated the documentation", but the diff doesn't include a changelog entry.)

Non-blocking follow-up in the inline comment: the sibling transient getters share the same latent behaviour — fine to keep this PR scoped to filename.

---

This review was drafted by an AI-assisted tool (Apache Magpie) and may contain mistakes. An Apache JMeter maintainer has reviewed and confirmed this submission. See CONTRIBUTING.md.

[milamberspace]

Minor / non-blocking (scope note): the sibling getters on this TestBean — getFileEncoding(), getVariableNames(), getDelimiter(), getShareMode(), and the boolean getters — all return their transient field directly and would exhibit the same null-after-loadTree() behaviour this PR fixes for filename. Keeping this PR scoped to filename (the subject of #6451) is perfectly reasonable; just flagging it as a possible follow-up so the pattern is fixed consistently later.

(Tiny aside, not a request: after an explicit setFilename(null) on a loaded element, getFilename() will now return the property value rather than null — an unlikely usage with no real-world impact.)

[e345ee]

Thanks for the review.

I added the required xdocs/changes.xml bug-fix entry and added myself under Thanks.

I also addressed the non-blocking scope note in this PR: the sibling CSVDataSet configuration getters now use the same property-map fallback pattern after SaveService.loadTree(). Explicit setters still take precedence over fallback values, including null for string properties and false for boolean properties.

Tested with:

• ./gradlew --no-daemon --max-workers=2 :src:components:test --tests org.apache.jmeter.config.TestCVSDataSet
• ./gradlew --no-daemon --max-workers=2 :src:components:checkstyleMain :src:components:checkstyleTest
• git diff --check

[vlsi]

Thanks for digging into this — the diagnosis is right, but I'd like to push back on the direction before this lands.

The bug is that CSVDataSet keeps its state in two places (the property map and the transient fields) and the getters read the wrong one. This PR doesn't remove the duplication; it adds a third copy (nine *Set flags) plus a precedence heuristic between the copies. A few consequences:

1. Getters and setters now disagree. The getters read from either source, but setFilename() still writes only to the field. So on a loaded plan, setFilename(x) followed by SaveService.saveTree() silently drops the change — which is the same "inspect and edit a loaded plan" use case the PR is motivated by. The new test encodes that divergence as expected behavior.

2. The fallback ignores the BeanInfo defaults. CSVDataSetBeanInfo declares NOT_UNDEFINED + DEFAULT (delimiter=",", filename="", shareMode=shareMode.all), and TestBeanHelper.unwrapProperty() applies them for a NullProperty. getStringProperty() returns null instead. For a JMX without a delimiter property, getDelimiter() now returns null and delim.isEmpty() in iterationStart() NPEs. That's a second set of defaults diverging from the first.

3. fieldSet || fieldValue != defaultValue — the second condition only fires if a field differs from its default without a setter call, which the transient modifier rules out for every property except ignoreFirstLine. Either it's dead code or it needs a comment naming the case.

4. The constants are a third copy — FILENAME, DELIMITER, SHAREMODE and friends already exist verbatim in CSVDataSetBeanInfo.

The transient fields exist to cache the evaluated property value, but FunctionProperty already does exactly that, with better semantics: it returns the raw string when !isRunningVersion() and evaluates with per-iteration caching once sampling starts. So the fields are a redundant layer, and the fix is to delete them rather than to arbitrate between them:

• add a CSVDataSetSchema (the infrastructure is in org.apache.jmeter.testelement.schema) carrying the property names and the defaults;
• back the getters/setters with the property map — Argument.java is the precedent: return get(getSchema().getArgumentName());
• drop the transient config fields, the *Set flags, and readResolve() (it only exists to restore the recycle default);
• have CSVDataSetBeanInfo read DEFAULT from the schema descriptors, so the defaults live in one place.

One thing that needs care: with property-backed setters, TestBeanHelper.prepare() would write the evaluated value back and collapse a FunctionProperty into a literal. For CSVDataSet it happens once at compile time (TestCompiler#trackIterationListeners), so behavior wouldn't change, but relying on that is the same implicit invariant that caused this bug. A marker interface that makes prepare() skip property-backed beans — in the spirit of NoConfigMerge / NoThreadClone — keeps it honest and lets the rest of the TestBeans migrate one at a time.

That's roughly the same diff size as this PR, but the state shrinks to zero instead of growing to 27 fields, and #6451 closes together with the symmetric write-side problem.

Worth adding to the test: a save/reload round-trip after setFilename(), assertEquals(",", new CSVDataSet().getDelimiter()), and getFilename() on a ${__P(csv)} value.

[e345ee]

Understand, I will rework, I need some time

[vlsi]

@e345ee first, an apology. You've now done two rounds of substantial work on this, and neither the earlier review nor I flagged the thing that should have been said before you started: CSVDataSet is a TestBean, and the split you set out to fix is the TestBean design rather than a defect in this class.

TestBeans keep their persistent state in the element's property map and mirror it into plain Java fields; TestBeanHelper.prepare() is the sync point, and it runs on the per-thread clone. That's why the fields are transient and why TestBeanGUI never touches them — it reads and writes the property map directly. So a getter returning null on a freshly loaded tree is the contract, and #6451 was reading a runtime accessor at design time. TestBeanHelper.prepare(element) or getPropertyAsString("filename") gives the reporter what they wanted. I've replied there and I'm closing it as works-as-designed.

That's on me for not saying so two rounds ago. It's the kind of thing a maintainer is supposed to catch on the first pass.

What this PR actually is now

Not a bug fix — a design proposal: move a TestBean from "POJO fields + prepare()" to property-backed storage, with PropertyBackedTestBean as the opt-out marker. That's a legitimate proposal, and the current revision is a good implementation of it. It's just a much bigger question than #6451, and it shouldn't be settled in a thread about getFilename().

Worth separating two things that TestBean bundles together:

• BeanInfo-driven GUI generation. You write a BeanInfo, you get a working GUI with no Swing code. This is TestBean's real value and nobody wants to lose it.
• Field-based storage with a prepare() sync step. This is what causes the reported behavior.

Your PR shows these are separable: CSVDataSet still implements TestBean, the GUI still works, only the storage changed. So the question isn't "TestBean or schemas" — it's whether the storage half should go.

The case for migrating

• It removes an entire bug class rather than one instance. All 32 TestBean elements have the same trap; JSR223 elements and ConstantThroughputTimer behave identically today.
• prepare() is reflective and runs per sample for samplers (JMeterThread#doSampling), invoking a write method for every declared property whether or not the sampler reads it. Property-backed access does a map lookup only for properties actually read. This plausibly nets out faster, though that claim needs a benchmark before anyone leans on it.
• Schemas have existed since 5.6 and are where the codebase is going. Argument and ConfigTestElement are already property-backed.
• The new property editors (JBooleanPropertyEditor and friends) bind to schema descriptors, so TestBean elements can't currently use them or the modified-value gutter.
• Defaults stop living in two places. Your CSVDataSetBeanInfo change, reading DEFAULT from the schema descriptors, is a good demonstration.
• 32 elements is a tractable number, and most are smaller than CSVDataSet.

The case against, or at least for caution

• Third-party plugins implement TestBean. The contract has to keep working, so any migration must stay additive. PropertyBackedTestBean is additive, which is right — but a codebase with two storage models half-migrated for two years is worse than either model.
• Function evaluation timing changes. Today prepare() freezes ${...} into a field; property-backed access re-evaluates per read, with FunctionProperty's per-iteration cache. That is arguably more correct, but it's an observable semantic change and some element may depend on the freeze.
• Getters start returning schema defaults where they returned null. Your PR already had to update three existing assertions from assertNull to assertEquals("").
• Hot-path cost moves from "one reflective sync per sample" to "a map lookup per read". Direction depends on access patterns; measure rather than assume.

Suggested path

If there's appetite for this, I'd rather we decide the semantics once, up front, than rediscover them per element:

1. Settle null vs schema default for unset properties, and the function-evaluation timing, as project-wide decisions.
2. Land PropertyBackedTestBean on its own, with @API(EXPERIMENTAL) and javadoc stating the contract. Consider requiring getSchema() rather than leaving it a bare marker, so the invariant is enforced.
3. Migrate elements in small batches, CSVDataSet first as the reference.

I'd like to hear from other maintainers before we commit to any of that.

For this PR meanwhile

Two things I'd change regardless of the outcome:

• The setProperty fast path skips normalization for values without spaces. shareMode.all renders as 所有线程 in the Chinese bundle, which has no space, so old plans saved under a CJK locale would no longer migrate to the canonical key. The existing comment does describe a space-based shortcut, but that comment describes an optimization that was never implemented and doesn't hold for languages without word spacing. Comparing against the three canonical enum values is both cheaper and locale-safe.
• With CSVDataSet.getFilename() returns null while filename exists in propMap #6451 closing as works-as-designed, the changes.xml entry needs rewording — this is a behavior change, not a bug fix.

To be clear about the outcome I'd like: I don't want this to end with your work discarded. The implementation is sound and it's a good reference point for the design discussion. I'd just rather it land as a deliberate change with maintainer buy-in than as a fix for an issue that turned out not to be a bug.

[vlsi]

Follow-up to my previous comment, and please read this before you start reworking.

I no longer think PropertyBackedTestBean is the right shape, so ignore step 2 of the path I sketched above. The marker names a storage strategy but its actual effect is "skip the field sync", and it doesn't extend TestBean despite the name.

The better decomposition, suggested by looking at where TestBean is actually checked: it bundles two unrelated jobs. Five call sites use it to mean "generate my GUI from BeanInfo" (TestBeanGUI, GuiPackage, MenuFactory, JMeterTreeNode), and three use it to mean "copy my properties into my setters" (TestBeanHelper, StandardJMeterEngine). No call site needs both. Splitting it into two interfaces, with TestBean extending both, keeps every existing instanceof result identical for all 32 elements and every third-party plugin, while letting new elements opt into only the half they want.

I've opened #6728 to work that out separately. It doesn't belong in a thread about getFilename(), and it needs input from other maintainers.

So: nothing to rework here yet. This PR is blocked on a design decision that isn't yours to make, and I don't want you spending a third round on a shape we might change again. I'll ping you once there's a conclusion.

[e345ee]

Understood, thank you for the clarification and feedback.
🫠🫠🫠🫠🫠🫠