Streamline binding of UI elements to TestElement properties

[vlsi]

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 calls port = "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 only null value would cause property removal.

TODO

• Ensure tests pass
• Apply apply JTextFieldBindEditor to Gui classes that use TestElementSchema so they remove property when the field is empty
• Add javadocs for the new classes, interfaces, and methods
• Check if this PR somehow resolves Downloading of embedded resources via HTTP Request Default not working on v5.6.2 #6076. The PR does resolve 6076
• Analyze if we need an alternative to org.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 has UrlConfigGui block, and we would like to call something like .modifyTestElement(element) from HttpDefaultsGui.modifyTestElement, however, if urlConfigGui.modifyTestElement clears the properties, then we can't compose UI blocks.
  Result: "Gui extensions" can be a separate sub-class of AbstractJMeterGuiComponent or JMeterGUIComponent that does not call super.modifyTestElement.., so they do not reset the test element state.
• Check naming
  • interface Binding (was interface PropertyDescriptorBinder) (it includes updateElement and updateUi methods)
  • class JTextComponentBinding (was class JTextComponentBindEditor), class JCheckBoxBinding (was class JCheckBoxBindEditor): they wrap regular JTextField and link them with PropertyDescriptors
  • class BindingGroup (was class PropertyEditorCollection) maintains a collection of Binding (was PropertyDescriptorBinder)
  • org.apache.jmeter.gui.JMeterGUIComponent#makeTestElement. It is intended to "just create empty test element" when creating from UI
  • org.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 sets method=post in assignDefaultValues.

Setting properties with API vs UI

1. If a property is set from code (e.g. from Java code), then we should store it in 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 in HTTP Request to avoid it being overridden by HTTP Request Defaults
2. If a UI element (e.g. 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 to 1.

I factor UI converters into UI classes to make it happen without much duplication. For instance, JEditableCheckBox calls into removeProperty 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:

public TestElement createTestElement() {
    TestElementXYZ el = new TestElementXYZ();
    modifyTestElement(el);
    return el;
}

modifyTestElement is a method that assigns the state of UI controls to TestElement's properties.
The drawback is that UI controls (e.g. JTextField) must be initialized to the appropriate default values and then modifyTestElement grabs those defaults and puts them to TestElementXYZ.

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 workable TestElement which we can display with JMeterGUIComponent#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 values
• JMeterGUIComponent#modifyTestElement extracts data from UI and populates TestElement's properties
• JMeterGUIComponent#configureTestElement somewhat duplicates modifyTestElement
• JMeterGUIComponent#configure extracts data from TestElement's and populates UI elements

After

• JMeterGUIComponent#createTestElement is a default method JMeterGUIComponent:

      default TestElement createTestElement() {
          TestElement element = makeTestElement();
          assignDefaultValues(element);
          return element;
      }

• JMeterGUIComponent#makeTestElement is a method plugin authors should implement to instantiate the TestElement instance

      @Override
      public TestElement makeTestElement() {
          return new HTTPSamplerProxy();
      }

• JMeterGUIComponent#assignDefaultValues assigns default property values like name, gui_class, test_class. Plugin authors could override it to add their defaults. For instance, HTTP Request wants to set method=GET, use_keepalive=true, and the implementation could be as follows:

      @Override
      public void assignDefaultValues(TestElement element) {
          super.assignDefaultValues(element);
          HTTPSamplerBaseSchema schema = HTTPSamplerBaseSchema.INSTANCE;
          // It probably does not make much sense overriding HTTP method with HTTP Request Defaults, so we set it here
          element.set(schema.getMethod(), HTTPConstants.GET);
          element.set(schema.getFollowRedirects(), true);
          element.set(schema.getUseKeepalive(), true);
          urlConfigGui.assignDefaultValues(element);
      }

• JMeterGUIComponent#clearGui. In most (all?) cases, the method should not alter simple fields. When JMeter wants to display TestElement it calls JMeterGUIComponent#configure, and the expectation is that configure 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 in clearGui.

• 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 to TestElement. It should treat empty string as "absence of a value", so it should remove the corresponding property. Classes like JTextFieldBindEditor make it easier to retrofit the existing code and make the behaviour consistent across all fields.

• JMeterGUIComponent#configureTestElement. I suggest we deprecate configureTestElement. Of course, we can keep it in the source code, however, we should not use it. We should just add a default implementation of modifyTestElement and ask users to follow the regular pattern like:

      @Override
      public void modifyTestElement(TestElement element) {
          super.modifyTestElement(element); // populate base properties like name, comments
          // populate custom properties

• org.apache.jmeter.gui.AbstractJMeterGuiComponent#editors is PropertyEditorCollection which helps to implement both modifyTestElement and configure at the same time. The idea is that we can link JTextField with a corresponding PropertyDescriptor, then it would know how to populate the field based on the given TestElement, and it would know how to get the UI value into the TestElement

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 regular JTextField instances, so we link them with a property, and remove the boilerplate from modifyTestElement and configure

    protected HttpTestSampleGui(boolean ajp) {
        isAJP = ajp;
        init();
        HTTPSamplerBaseSchema schema = HTTPSamplerBaseSchema.INSTANCE;
        editors.addAll(
                Arrays.asList(
                        retrieveEmbeddedResources,
                        concurrentDwn,
                        new JTextComponentBinding(concurrentPool, schema.getConcurrentDownloadPoolSize()),
                        useMD5,
                        new JTextComponentBinding(embeddedAllowRE, schema.getEmbeddedUrlAllowRegex()),
                        new JTextComponentBinding(embeddedExcludeRE, schema.getEmbeddedUrlExcludeRegex())
                )
        );
        if (!isAJP) {
            editors.addAll(
                    Arrays.asList(
                            new JTextComponentBinding(sourceIpAddr, schema.getIpSource()),
                            // TODO: sourceIpType
                            new JTextComponentBinding(proxyScheme, schema.getProxy().getScheme()),
                            new JTextComponentBinding(proxyHost, schema.getProxy().getHost()),
                            new JTextComponentBinding(proxyPort, schema.getProxy().getPort()),
                            new JTextComponentBinding(proxyUser, schema.getProxy().getUsername()),
                            new JTextComponentBinding(proxyPass, schema.getProxy().getPassword()),
                            // TODO: httpImplementation
                            new JTextComponentBinding(connectTimeOut, schema.getConnectTimeout()),
                            new JTextComponentBinding(responseTimeOut, schema.getResponseTimeout())
                    )
            );
        }
    }

[vlsi]

I'm happy with the changes, except there might be better names for the added classes and methods.