You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by "e345ee (via GitHub)" <gi...@apache.org> on 2026/07/13 07:14:16 UTC
[PR] Fix CSVDataSet filename getter after loading test plan [jmeter]
e345ee opened a new pull request, #6726:
URL: https://github.com/apache/jmeter/pull/6726
## 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:
- [x] My code follows the [code style][style-guide] of this project.
- [x] I have updated the documentation accordingly.
[style-guide]: https://wiki.apache.org/jmeter/CodeStyleGuidelines
--
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: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix CSVDataSet filename getter after loading test plan [jmeter]
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on PR #6726:
URL: https://github.com/apache/jmeter/pull/6726#issuecomment-5011504588
@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 #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.
--
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: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix CSVDataSet filename getter after loading test plan [jmeter]
Posted by "e345ee (via GitHub)" <gi...@apache.org>.
e345ee commented on PR #6726:
URL: https://github.com/apache/jmeter/pull/6726#issuecomment-5011695361
Understood, thank you for the clarification and feedback.
--
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: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix CSVDataSet filename getter after loading test plan [jmeter]
Posted by "e345ee (via GitHub)" <gi...@apache.org>.
e345ee commented on code in PR #6726:
URL: https://github.com/apache/jmeter/pull/6726#discussion_r3603224753
##########
src/components/src/main/java/org/apache/jmeter/config/CSVDataSet.java:
##########
@@ -239,7 +239,11 @@ private static void trimVarNames(String[] varsNames) {
* @return Returns the filename.
*/
public String getFilename() {
- return filename;
+ if (filename != null) {
+ return filename;
+ }
+ JMeterProperty property = getPropertyOrNull("filename");
Review Comment:
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`
--
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: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix CSVDataSet filename getter after loading test plan [jmeter]
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on PR #6726:
URL: https://github.com/apache/jmeter/pull/6726#issuecomment-5011640858
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.
--
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: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix CSVDataSet filename getter after loading test plan [jmeter]
Posted by "milamberspace (via GitHub)" <gi...@apache.org>.
milamberspace commented on code in PR #6726:
URL: https://github.com/apache/jmeter/pull/6726#discussion_r3602560068
##########
src/components/src/main/java/org/apache/jmeter/config/CSVDataSet.java:
##########
@@ -239,7 +239,11 @@ private static void trimVarNames(String[] varsNames) {
* @return Returns the filename.
*/
public String getFilename() {
- return filename;
+ if (filename != null) {
+ return filename;
+ }
+ JMeterProperty property = getPropertyOrNull("filename");
Review Comment:
**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.)
--
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: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix CSVDataSet filename getter after loading test plan [jmeter]
Posted by "e345ee (via GitHub)" <gi...@apache.org>.
e345ee commented on PR #6726:
URL: https://github.com/apache/jmeter/pull/6726#issuecomment-5003726607
Understand, I will rework, I need some time
--
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: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org