| After some investigation, I've found the root cause of this issue. I guess it's a partial responsibility between the configuration-as-code plugin, and the p4 plugin.
- Additionally, in that same base class, a method named isSsl is defined. This method returns a boolean (and, importantly, not a TrustImpl object).
- The method the configuration-as-code plugin uses to describe a data-bound type (as found in DataBoundConfigurator::describe) involves iterating over each argument of a class' constructor (which are assumed to be part of the description), and obtaining their current value on the given instance.
- Obtaining said valid is done by calling the Attribute::locateGetter method (via Attribute::_getValue, passing it the class definition, and the name of the field to locate. locateGetter proceeds to iterate over all defined methods in the class, looking for a method taking no parameter named after the capitalized field name prefixed by either get or is.
- In our case, the constructor parameter name is ssl, so locateGetter attempts to find a method named either getSsl or isSsl. Unfortunately for us, P4BaseCredential, the base class of our credential types, defines an isSsl method, as we've seen above. This is the method locateGetter returns, which is then used to obtain the value of our ssl field, which ends up being a boolean (since that's the return type of isSsl). This boolean, in DataBoundConfigurator::describe, gets added to the args array that is then used to construct a new instance of the class, by calling its data-bound constructor.
- This is where the exception is raised: the type of the 5th element of args is boolean, but the 5th parameter of our constructors is of type TrustImpl. This throws an IllegalArgumentException which ends up being displayed as-is in the configuration export.
There are two fixes that I can see:
- First, it seems odd that the configuration-as-code plugin would consider methods with an incompatible return type in Attribute::locateGetter. If that method was provided with the type of the field alongside its name, it could use that information to discard accessors whose name matches one of the two expected patterns, but whose type doesn't match the field. This would've caused locateGetter to return null, and _getValue would've then fallen back to locating a field matching the expected name (note that the comment there is misleading, forceAccess is true so it'll still find private fields), which would've been found. Note that in this execution path too, ExtraFieldUtils.GetField does not check for the field type at all – that might be worth looking at as well. In our specific case, though, the type would've been a match.
- Second, the p4 plugin should probably consider using the convention around naming getters for data-bound constructors that the configuration-as-code plugin relies on. This would mean replacing the isSsl method with a getSsl one returning a TrustImpl. Note that just adding another valid getter without removing isSsl might or might not work, depending on the order the Java's reflection engine would place them: if isSsl is listed before getSsl is, without the aforementioned change to the configuration-as-code plugin, this bug would still happen.
|
