[InjectingExtension] Refactoring suggestions

[kriegfrj]

Having reviewed the InjectingExtension in more detail, I believe that we've made some mistakes in the pattern that we should consider addressing:

1. It became apparent in my work on [ServiceExtension] Type compatibility check for @InjectService.service() #495 and [ServiceExtension] Tweak wildcard type inferencing for upper bounds #494 that supportsType() should have taken the resolved annotation as a parameter. We can add this as a new method and delegate to the old.
2. InjectingExtension implements AfterEachCallback and AfterAllCallback. However, the implementations do nothing. There is therefore no reason for the base class to implement these methods - if a specific implementation wishes to add this functionality, it can do so by directly implementing them.
3. There is no reason for supportsType() to return a boolean. supportsParameter()and supportsField() both do a check to see if the field/parameter is annotated by the annotation type that is "owned" by the extension. If it is not owned by the extension, then it should return false so that Jupiter can find the rightful owner. But if if it is owned by the extension, then supportsField()/supportsParameter() must either return true (if all is well) or else throw an informative exception if there is some kind of misconfiguration. Simply returning false at this stage would be uninformative and implementations should be discouraged from doing so. boolean supportsType() should have perhaps been void checkField().

Were it not for the fact that InjectingExtension has already found its way into a release in its current form, I would simply make these changes. However, I'm looking at the versioning consequences.

The first change would introduce a minor or a major change depending on whether we simply change the signature or else add a new method that delegates to the old. The second would be a major change. The third - we could simply ignore the boolean return value from supportsType() and avoid any formal change, though the practical change would still be there so it might be misleading to not bump the version in that case (I think it would be confusing to implementers have a boolean-valued override whose return value is ignored).

Thoughts?

[bjhargrave]

2. InjectingExtension implements AfterEachCallback and AfterAllCallback. However, the implementations do nothing.

This was added by me on purpose and should not be removed. The reason it is exists is for future proofing. If a subclass implements these interfaces, their method implementations should call the super methods in case they have important behavior. But if we do not now implement these methods, subclasses cannot call them.

https://github.com/osgi/osgi-test/blob/0426c0b48202ed9c9de49e90ea04741ee995c431/org.osgi.test.junit5.cm/src/main/java/org/osgi/test/junit5/cm/ConfigurationExtension.java#L98

So we need to keep this to allow for the future were we want to implement these methods in the base class.

[bjhargrave]

3. There is no reason for supportsType() to return a boolean. supportsParameter()and supportsField() both do a check to see if the field/parameter is annotated by the annotation type that is "owned" by the extension.

That is not enough. There are other reasons why the subclass may know that it cannot handle the type and needs to communicate this with the caller in the base class. So I don't agree this is wrong.

[bjhargrave]

1. that supportsType() should have taken the resolved annotation as a parameter. We can add this as a new method and delegate to the old.

I am not sure what useful information the annotation has for supportsType? The TargetType parameter is the proper place (abstraction) to pass any necessary information to the method. If we need more information, we should improve TargetType and use it to pass information.

[kriegfrj]

2. InjectingExtension implements AfterEachCallback and AfterAllCallback. However, the implementations do nothing.

This was added by me on purpose and should not be removed. The reason it is exists is for future proofing. If a subclass implements these interfaces, their method implementations should call the super methods in case they have important behavior. But if we do not now implement these methods, subclasses cannot call them.

https://github.com/osgi/osgi-test/blob/0426c0b48202ed9c9de49e90ea04741ee995c431/org.osgi.test.junit5.cm/src/main/java/org/osgi/test/junit5/cm/ConfigurationExtension.java#L98

So we need to keep this to allow for the future were we want to implement these methods in the base class.

I still disagree, for a couple of reasons:

1. There are a number of other extension points that haven't been added. By the above argument, we should have added them all.
2. Re: the future proofing argument - if the methods don't exist in the superclass, then there's obviously no need to call them. If they did exist, then the decision about whether/when to call them might depend on the subclass' implementation. If we left them out and added them in the future, it would cause a bump in the version which would then alert subclass implementers to the change and force them to make a conscious decision about if/when/how to call the superclass implementations knowing what they do - rather than based on guessing what they might do in the future.

However, I am happy to let this ship sail now rather than introduce another version change to remove them.

[bjhargrave]

1. There are a number of other extension points that haven't been added. By the above argument, we should have added them all.

Good point. I am not aware of all of them. I only put the counterparts to the Before interfaces for symmetry. (Asymmetry offends my OCD :-)

2. it would cause a bump in the version which would then alert subclass implementers to the change and force them to make a conscious decision

They would only know if they see the compiler complaining about the lack of Override if they recompile to the new version.

2. rather than based on guessing what they might do in the future.

The sane subclasser would call the super methods now. But we should more clearly document this requirement in the javadoc. Properly designing classes for subclassing is hard :-) I don't always get it right, but I try to think about the future.

However, I am happy to let this ship sail now rather than introduce another version change to remove them.

Agree, at this point, it is already done. Removing then would be a major version bump.

[kriegfrj]

3. There is no reason for supportsType() to return a boolean. supportsParameter()and supportsField() both do a check to see if the field/parameter is annotated by the annotation type that is "owned" by the extension.

That is not enough. There are other reasons why the subclass may know that it cannot handle the type and needs to communicate this with the caller in the base class. So I don't agree this is wrong.

As I understand the pattern working:

• Each injecting extension is responsible for injecting fields/parameters annotated by a particular annotation type (parameterised as INJECTION).
• Any field/parameter not annotated by INJECTION must not be claimed by the extension. This is indicated by supportsParameter()/supportsField() returning false.
• Any field/parameter annotated by that type must be claimed by the extension. Once claimed, it must not return false - it must be handled in one of two ways:
  • In the event of a misconfiguration, by throwing an exception;
  • In the event of no misconfiguration, by returning true.

Returning true or false as appropriate can (and I argue, should) be handled by the caller in the superclass (supportsParameter()/supportsField()). All the subclass needs to do then is check for misconfiguration and throw an exception if appropriate. There is no use case for it to return false because by the time it is called it knows that it has been annotated by an annotation that it must claim.

[kriegfrj]

2. it would cause a bump in the version which would then alert subclass implementers to the change and force them to make a conscious decision

They would only know if they see the compiler complaining about the lack of Override if they recompile to the new version.

True. But that's why a good developer always looks at the compiler warnings. 😄

2. rather than based on guessing what they might do in the future.

The sane subclasser would call the super methods now. But we should more clearly document this requirement in the javadoc. Properly designing classes for subclassing is hard :-) I don't always get it right, but I try to think about the future.

Crystal balls are hard to come by and notoriously unreliable. 😄

[kriegfrj]

I am not sure what useful information the annotation has for supportsType? The TargetType parameter is the proper place (abstraction) to pass any necessary information to the method. If we need more information, we should improve TargetType and use it to pass information.

Because the question of whether or not it supports a type may depend on how it has been configured in the annotation. For those injectors that always inject into a particular type, this will not be an issue. But ServiceExtension is different - whether or not it supports the type depends on how it is configured in the annotation itself.

I managed to work around this by pushing the code into resolveValue().

This overlaps a bit with the issues I raised for supportType() not needing to return a boolean.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. If you feel this is something you could contribute, please have a look at our Contributor Guide. Thank you for your contribution.

[github-actions]

This issue has been automatically closed due to inactivity. If you can reproduce this or if you have a good use case for this feature, please feel free to reopen the issue with steps to reproduce, a quick explanation of your use case or a high-quality pull request.