#getComponentParametersInfo(request) is very brittle in case an hst component gets extended

--------------------------------------------------
Explanation of the problem :
--------------------------------------------------

today I extended the built in EssentialsMenuComponent as follows:

public class MyEssentialsMenuComponent extends EssentialsMenuComponent {
@Override
public void doBeforeRender(final HstRequest request, final
HstResponse response)

{ super.doBeforeRender(request, response); }

}

To my surprise, this results in an NPE when referring to
MyEssentialsMenuComponent in my menu hst component instead of
EssentialsMenuComponent

Why?

Because in EssentialsMenuComponent there is:

final EssentialsMenuComponentInfo paramInfo =
getComponentParametersInfo(request);

but getComponentParametersInfo scans MyEssentialsMenuComponent for the
annotation 'ParametersInfo' which it doesn't have.

--------------------------------------------------
Solution :
--------------------------------------------------
Deprecate #getComponentParametersInfo(request) and add #getComponentParametersInfo(Class componentClazz, request)

--------------------------------------------------
We need to check:
--------------------------------------------------
that this MyEssentialsMenuComponentInfo interface can extend
EssentialsMenuComponentInfo, and then also inherit the annotated
methods from EssentialsMenuComponentInfo, right? Perhaps this already
works because the code is:

for (Method method : classType.getMethods()) {
if (method.isAnnotationPresent(Parameter.class)) {

THREAD:

On Thu, Feb 4, 2016 at 10:25 PM, Woonsan Ko <w.ko@onehippo.com> wrote:
> On Thu, Feb 4, 2016 at 4:23 PM, Ard Schrijvers
> <a.schrijvers@onehippo.com> wrote:
>> On Thu, Feb 4, 2016 at 10:20 PM, Ard Schrijvers
>> <a.schrijvers@onehippo.com> wrote:
>>> On Thu, Feb 4, 2016 at 10:13 PM, Woonsan Ko <w.ko@onehippo.com> wrote:
>>>> On Thu, Feb 4, 2016 at 4:06 PM, Ard Schrijvers
>>>> <a.schrijvers@onehippo.com> wrote:
>>>>> On Thu, Feb 4, 2016 at 8:59 PM, Woonsan Ko <w.ko@onehippo.com> wrote:
>>>>>> In addition to what Niels pointed out, HST ParametersInfo annotation
>>>>>> supports two use cases, in my understanding: a) easier access to hst
>>>>>> component parameters via interface, b) channel manager configuration
>>>>>> support as Niels pointed out.
>>>>>>
>>>>>> In use case a), I would like to consider another situation in mind: a
>>>>>> component extends another component to reuse some functionality, but
>>>>>> the component doesn't want to use the parameters annotation as-is. No
>>>>>> parameters or different annotation possibly.
>>>>>> For this, I would like to suggest improving the API,
>>>>>> #getComponentParametersInfo(request), to
>>>>>> #getComponentParametersInfo(Class componentClazz, request). So, the
>>>>>> base class in Ard's example, EssentialsMenuComponent, could pass its
>>>>>> own class EssentialsMenuComponent.class to read its own parametersInfo
>>>>>> annotation safely, regardless of how it was extended and used.
>>>>>> ParameterUtils#getParametersInfo(HstComponent, ..) should have been
>>>>>> #getParametersInfo(Class hstComponentClazz, ..), I guess.
>>>>>
>>>>> Yes, I think this is a good idea! Note that in the channel manager, we
>>>>> should show all properties for all superclasses that are annotated
>>>>> with a ParametersInfo annotation, right? So in channel manager, we
>>>>> show an augmented version, in code, we start using
>>>>> #getComponentParametersInfo(Class componentClazz, request).
>>>>>
>>>>>>
>>>>>> In use case b), I agree with Niels. I think the extending component
>>>>>> class should explicitly define it for parameterization in channel
>>>>>> manager.
>>>>>
>>>>> Imho, the channel manager should show an augmented parameterization of
>>>>> the ParametersInfo annotation properties of the current component
>>>>> and its super classes ParametersInfo annotation properties.
>>>>> Otherwise, you can not configure properties from
>>>>> EssentialsMenuComponent if you extend it, right?
>>>>>
>>>>> WDYT?
>>>>
>>>> So, it's either augmenting of annotations or augmenting of interfaces, IMHO.
>>>> I guess the latter one is a lot simpler and less complex. One thing
>>>> is, in the latter approach, that developers should still put an
>>>> annotation explicitly with a ParametersInfo interface in child
>>>> component class. But I think that's a reasonable approach, with giving
>>>> more flexibility. If a developer wants to show everything in channel
>>>> manager, then she or he can simply add annotation with the interface
>>>> of super class or a new interface extending super's ParametersInfo
>>>> interface. Then channel manager can simply list all the getters a lot
>>>> simpler (comparing with annotation searching..)
>>>
>> Ah you are right I think (if I understand you correctly).
>>
>> We need to support that if you have
>>
>> @ParametersInfo(type = MyEssentialsMenuComponentInfo.class)
>>
>> that this MyEssentialsMenuComponentInfo interface can extend
>> EssentialsMenuComponentInfo, and then also inherit the annotated
>> methods from EssentialsMenuComponentInfo, right? Perhaps this already
>> works because the code is:
>>
>> for (Method method : classType.getMethods()) {
>> if (method.isAnnotationPresent(Parameter.class)) {
>>
>> Perhaps this also just works for superclass methods : I think so!!!!
>
> Me, too!!!!!
>
>>
>> If that works, then the only thing we need to change is deprecate
>> #getComponentParametersInfo(request), and replace it with
>> #getComponentParametersInfo(Class componentClazz, request),
>>
>> WDYT?
>
> Yea, sounds really cool!
>
>>
>> Note that since essentials has a different release cycle than cms/hst,
>> I will keep the deprecation of #getComponentParametersInfo(request) at
>> least until CMS 12
>>
>> WDYT? Does this make sense?
>
> Makes sense!

Great, thanks for your valuable feedback Woonsan and Niels, and thanks
for the #getComponentParametersInfo(Class componentClazz, request)
idea! I'll create an hst issue for this