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