Improvements to the xml/PropertyHelper.java class

[NikithaUdeshana]

Description:
The following break statement in the PropertyHelper class[1] in the synapse-commons module is missing compared to the synapse-core PropertyHelper class[5]. Even though both of these classes serve the same purpose, the difference in the above line led them to behave differently.

In the case of Class Mediator it passes propertyName, extracted value and the class mediator implementation object to this method and the behaviour should be looping through each method of class mediator implementation object and see whether the corresponding Setter for propertyName is there and invoke it with the value and after this, the loop should break. So adding break to the end of if (mName.equals(method1.getName())){ } condition should be the correct behaviour.

[1] https://github.com/wso2/wso2-synapse/blob/5debdfd925874ad568f0ecec7dc778996ddebae2/modules/commons/src/main/java/org/apache/synapse/commons/util/PropertyHelper.java#L95
[2] https://github.com/wso2/wso2-synapse/blob/5debdfd925874ad568f0ecec7dc778996ddebae2/modules/core/src/main/java/org/apache/synapse/config/xml/PropertyHelper.java#L95

Affected Product Version:
WSO2 Micro Integrator 1.2.0

[0pq76r]

The "missing" break statement is only noticed when multiple Setters for "propertyName" are implemented for the object.
In your class[1] (with the break), it will "randomly" execute one Setter; in class[2], it will execute all Setter for "propertyName" but in an undefined order.

The array of mediators you iterate over is defined as Method[] methods = obj.getClass().getMethods();. According to Class (Java Platform SE 8 ): "The elements in the returned array are not sorted and are not in any particular order.".

Adding the break probably fixes the broken implementation for now. OpenJDK 8 and 11 are likely to place inherited methods at the end of the array (observed behaviour on Linux and Windows). Thus, the Setter declared in the object (which probably is the intended Setter by the author of the mediator) is likely to be called first.

The proper solution would be to sort the methods according to some criteria (e.g. declared before inherited, number of arguments, primitive types first, ...). Then it would be clear which Setter is called (and in which order if no break is added).

The second option would be to reject any object where multiple Setters for a "propertyName" are defined. However, this would make it impossible to add new properties to non-final mediator classes. A derived class possibly already has a Setter for "propertyName" of a different type, adding a new Setter to a parent class turns the existing Setter into an overloaded method. As a result, the derived mediator could no longer be initialized.

[dnwick]

Fixed in wso2/wso2-synapse#1886

[chanikag]

Still issues could happen when both methods use String as an input parameter. Then, Method[] methods = obj.getClass().getMethods() will return multiple setter methods with same parameters. For example, PayloadFactoryMediator.setTemplateType() method takes a String. If we have the same method name in a class with different method signature it fails.

We can improve the logic to give better user experience. We can print warn logs to improve the user experience.

wso2/wso2-synapse@5aa1895