Change the policy of ComponentManager `#getInstanceList` to log instead of failing the whole list in case of failing component

Development
#1

Hi everyone,

The title contains the proposal.

Currently, when you inject a List of components or use ComponentManager#getInstanceList you are stuck when any of the elements in the list fail to initialize. The point back then was to be on the safe side in case for use case where any missing element from the list would make the logic broken.

Problem is that I don’t really have many examples of such use cases, but I have plenty where a bad custom component (bad extension, bad wiki component) can break everything.

So I would suggest, starting with 15.0, to change a bit to implementation of #getInstanceList to log an error when failing to initialize any element and continue instead of throwing an exception.

WDYT ?

Here is my +1

#2

WDYT of having two methods:

  1. the existing one ComponentManager#getInstanceList, with its current semantic
  2. a new ComponentManager#getUnsafeInstanceList (name to be discussed) either:
    2.1. logging the failing components
    2.2 returning a structure with two fields, the list of successfully returned components, a map of failed components mapped with the corresponding exceptions.

Also, do you think it could be useful to do the same for getInstanceMap?

#3

I’m not sure to see how it improves current situation as we’d need to replace all the calls of getInstanceList in existing code to the unsafe one to have some benefits.
Now I’ve also the feeling that in some specific cases we might want to get the exception, so I’d propose the contrary: to change the policy of the existing one as proposed by Thomas and to have a new one throwing indeed an exception.

#4

I’m fine with that option. My point was mainly that the existing semantics shouldn’t be lost since, as you said, it could still be useful in some use cases.

#5

Yes, sorry I only mentioned getInstanceList but the idea is of course to change the logic of all the APIs that return several components.

The whole point here is to bulletproof all the existing code and not to scare people so that you don’t use the new behavior :slight_smile:

I thought about proposing that, but I’m not really sure it’s really needed in practice.

#6

To be honest, I’m not sure I agree with this. Do you have any examples where throwing the exception causes issues?

One example I have in mind are raw block filters: They use getInstanceList to get a list of components. If, for example, XWikiHTMLRawBlockFilter would fail to initialize for whatever reason, this must throw an exception as otherwise we have a security issue.

Similar arguments can be made for event listeners. If any event listener that has the job to ensure safety by, e.g., preventing saving a document or preventing the execution of a script macro is failing to initialize for whatever reason, XWiki must fail to initialize or we’ll have a huge security issue.

Extensions can also easily override any component and thus cause arbitrary components that inject other components fail to initialize so the argument that list dependencies should be treated differently is not that strong imho.

#7

There are plenty of examples where you gather a list of totally independent components either to call them all or to provide a list of choices and if one of those components fail to initialize the whole feature, and in some cases pretty much the whole XWki, is dead.

Here are some examples but the list is very long:

  • any bad listener will make XWiki quite unusable
  • if a bad macro is registered, the macro picker is dead
  • any bad ui extension will make all ui extensions registered for the same point disappear

Another aspect of this example is that any extension or wiki component providing a broken implementation of RawBlockFilter will make the html macro unusable.

#8

I think we need to cover both cases:

  • injected components are optional => log exception
  • injected components are mandatory => throw exception

I agree with @tmortagne that most of the time the components in the list / map are independent and the fact that the list / map is dynamic makes those components optional by default. If you want to make sure a specific component is used / called you should probably inject it explicitly / directly. For the cases where this is not possible / easy we should provide an alternative getInstanceList that throws the exception, but I’d use a parameter rather than a new method name: getInstanceList(Type role, boolean failSilently).

Thanks,
Marius

#9

The way I’d do this is by returning a ComponentResult object with methods to know which instance had errors and the exception raised and methods to iterate over the list or map. Then, it would be the caller’s decision to decide how to handle errors, depending on its context. In some cases, the caller will just log the problems and continue, in others it’ll fail what it’s doing by throwing another exception, etc.

Since we have support for List<...> and Map<...> in @Inject fields, we’d need to change that in favor of something like ComponentListResult<...> and ComponentMapResult<...>.

#10

I still don’t understand why you would consider a listener that checks rights for script macros as optional. Such listeners are important safety checks and if any of these fails to initialize we need to do more than just log an error that might not be noticed at all. Maybe we could add an annotation to mark components that aren’t optional as they contain important safety checks?

#11

You are assuming that it’s common for such a listener to fail to initialize. It generally means you are doing something dangerous in the initialize and you always have ways to make a component init bulletproof if needed.

Sure, that’s easy, and I much prefer that than having several APIs. The component knows better if it’s mandatory than what calls getInstanceList (and if getInstanceList should fail for any component of a given role all you have to do is put the annotation on the role). Even if we were getting a detailed list of what happen with each component, we would not really know what do it with it.

#12

So here is an updated proposal:

  • change the default behavior of ComponentManager#getInstanceList (and #getInstanceMap) to log instead of throw in case one of the extensions fail to initialize
  • introduce a @ComponentMandatory annotation (itself annotated with @Inherited) that you use to control this behavior: @ComponentMandatory would change the behavior of getInstanceList to fail if that specific component init fail (and I would also probably add a boolean value to that annotation to change the behavior of a @ComponentMandatory located in a super class/interface).

Here is my +1

#13

+1 for this updated proposal.

#14

+1 Thanks

#15

+1,

Thanks,
Marius

#16

(sorry to be a pain and answering so late)

-1 (unless I’m missing something) as this is a big breaking API change. Calling code will have a try/catch and will break when upgrading XWiki at runtime. They’ll break from an API POV but also in term of their logical handling of errors.

I like the proposal but not to change the default behavior. IMO we need to add @OptionalComponent in places where we want them to be optional. It’s also safer by default.

#17

The whole point of the proposal is to make getInstanceList not fail by default (which is also why it’s done only in master) because 99% of the components are optional.

#18

I understand that but it’s a breakage for any well-coded extension using getInstanceList (since they should manage error conditions).

#19

For example: Sign in to GitHub · GitHub (that’s only a subset since theres the injections through @Injecty Provider<List<...>> ...).

Example: the following would return an empty list now but will return something different afterwards (and the code won’t compile, not sure what happens at runtime): application-scripting-documentation/DefaultScriptBindingsFinder.java at f329f8d8a594de5bada8c11211a6365637ac07b0 · xwiki-contrib/application-scripting-documentation · GitHub

#20

Of course not, nothing changed in term of signature.

In this use case, having one missing ScriptBindingsFinder because it failed to initialize does not matter much, you will just have some missing bindings instead of nothing. The code used to return an empty list because it could not really do much more, but the intent is clearly to never fail, so that’s typically a use case which is going to be improved by this change.