Streamline binding of UI elements to TestElement properties #6199
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Previously (e.g. in 5.6.2),
TestElement.set(PropertyDescriptor, String?)
was removing the property if the value was an empty string. I thought it was convenient for the UI, however now I realize that was a mistake. If the user callsport = "whatever"
they expect the property should be set no matter if the value is empty string or if it accidentally matches a default one.This PR changes
.set
behaviour so onlynull
value would cause property removal.TODO
JTextFieldBindEditor
toGui
classes that useTestElementSchema
so they remove property when the field is emptyorg.apache.jmeter.gui.JMeterGUIComponent#modifyTestElement
that does not clear the element state. For most elements,modifyTestElement
should reset the test element state, and then populate properties based on the UI state. However, if UI is composed from several sub-UI blocks, then we need to apply sub-block "UI to properties" transformation without clearing the properties. For instance,HttpDefaultsGui
hasUrlConfigGui
block, and we would like to call something like.modifyTestElement(element)
fromHttpDefaultsGui.modifyTestElement
, however, ifurlConfigGui.modifyTestElement
clears the properties, then we can't compose UI blocks.Result: "Gui extensions" can be a separate sub-class of
AbstractJMeterGuiComponent
orJMeterGUIComponent
that does not callsuper.modifyTestElement..
, so they do not reset the test element state.interface Binding
(wasinterface PropertyDescriptorBinder
) (it includesupdateElement
andupdateUi
methods)class JTextComponentBinding
(wasclass JTextComponentBindEditor
),class JCheckBoxBinding
(wasclass JCheckBoxBindEditor
): they wrap regularJTextField
and link them withPropertyDescriptors
class BindingGroup
(wasclass PropertyEditorCollection
) maintains a collection ofBinding
(wasPropertyDescriptorBinder
)org.apache.jmeter.gui.JMeterGUIComponent#makeTestElement
. It is intended to "just create empty test element" when creating from UIorg.apache.jmeter.gui.JMeterGUIComponent#assignDefaultValues
. It should assign default values after creating the test element. By default it assigns name, guiClass, etc. Subclasses could set their defaults. For instance, GraphQL sampler inherits from HTTP Request, and it setsmethod=post
inassignDefaultValues
.Setting properties with API vs UI
TestElement
properties no matter if the value is empty or if the value is the same as default one. For instance,HTTP Request Defaults
(and other "config" elements) override non-set properties only, so we might want to explicitly have a value inHTTP Request
to avoid it being overridden byHTTP Request Defaults
JTextField
) is empty (blank) we assume the value is unset. Currently, we have no way to distinguish between "value is empty string" vs "value is unset", so we just assume "blank" values to be the same as "unset". It means UI elements should convert "empty string text field" and "empty checkbox" to "property removal". Note that we can't integrate "remove empty property" into setter methods due to1.
I factor UI converters into UI classes to make it happen without much duplication. For instance,
JEditableCheckBox
calls intoremoveProperty
for unset checkboxes.I added
JTextFieldBindEditor
to make it easier to teach the existing UI classes to "remove property on empty string"Creating elements with UI
The way UI creates test elements is
org.apache.jmeter.gui.JMeterGUIComponent#createTestElement
. Historically, the recommendation was as follows:modifyTestElement
is a method that assigns the state of UI controls toTestElement's
properties.The drawback is that UI controls (e.g.
JTextField
) must be initialized to the appropriate default values and thenmodifyTestElement
grabs those defaults and puts them toTestElementXYZ
.I suggest we should drift away from using
modifyTestElement
when creating test elements, and we should just set the property values as property values. Then we have a workableTestElement
which we can display withJMeterGUIComponent#configure
(it extracts properties and populates UI accordingly).I think the change is backward compatible, and there's no rush in editing every component at once, however, it makes code simpler when "set default values in
JTextField
" calls are removed.Before
JMeterGUIComponent#createTestElement
creates test element and populates values based on UI controls (modifyTestElement
).JMeterGUIComponent#clearGui
sets all controls to their default valuesJMeterGUIComponent#modifyTestElement
extracts data from UI and populatesTestElement's
propertiesJMeterGUIComponent#configureTestElement
somewhat duplicatesmodifyTestElement
JMeterGUIComponent#configure
extracts data fromTestElement's
and populates UI elementsAfter
JMeterGUIComponent#createTestElement
is adefault
methodJMeterGUIComponent
:JMeterGUIComponent#makeTestElement
is a method plugin authors should implement to instantiate theTestElement
instanceJMeterGUIComponent#assignDefaultValues
assigns default property values likename
,gui_class
,test_class
. Plugin authors could override it to add their defaults. For instance,HTTP Request
wants to setmethod=GET
,use_keepalive=true
, and the implementation could be as follows:JMeterGUIComponent#clearGui
. In most (all?) cases, the method should not alter simple fields. When JMeter wants to displayTestElement
it callsJMeterGUIComponent#configure
, and the expectation is thatconfigure
updates all the UI fields. That means there's no need to reset the fields. However, the UI might have extra state like "selected basic/advanced tab", and it might make sense to reset that state inclearGui
.JMeterGUIComponent#configure
. No changes. It should update all UI controls based on the properties of the test element even in the case the property is absent. Note: for now, "unset properties that have defaults" must be displayed as empty string to avoid confusion. For instance,HTTP charset
has default of UTF-8, however, we do not want to display it as if the user put UTF-8 explicitly.JMeterGUIComponent#modifyTestElement
should extract data from UI and pass toTestElement
. It should treat empty string as "absence of a value", so it should remove the corresponding property. Classes likeJTextFieldBindEditor
make it easier to retrofit the existing code and make the behaviour consistent across all fields.JMeterGUIComponent#configureTestElement
. I suggest we deprecateconfigureTestElement
. Of course, we can keep it in the source code, however, we should not use it. We should just add a default implementation ofmodifyTestElement
and ask users to follow the regular pattern like:org.apache.jmeter.gui.AbstractJMeterGuiComponent#editors
isPropertyEditorCollection
which helps to implement bothmodifyTestElement
andconfigure
at the same time. The idea is that we can linkJTextField
with a correspondingPropertyDescriptor
, then it would know how to populate the field based on the givenTestElement
, and it would know how to get the UI value into theTestElement
Here's a sample for
HTTP Request Sampler
UI.concurrentDwn
,useMD5
are "editable checkboxes" which already were linked to the properties, so they are passed as is. The rest (e.g.concurrentPool
) are regularJTextField
instances, so we link them with a property, and remove the boilerplate frommodifyTestElement
andconfigure