Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ServiceExtension] Type compatibility check for @InjectService.service() #495

Closed
kriegfrj opened this issue Mar 28, 2022 · 5 comments · Fixed by #497
Closed

[ServiceExtension] Type compatibility check for @InjectService.service() #495

kriegfrj opened this issue Mar 28, 2022 · 5 comments · Fixed by #497

Comments

@kriegfrj
Copy link
Contributor

The service() attribute of the @InjectService annotation allows you to specify a base service type to use when looking for services. At present, it allows you to specify a service type that is not assignment compatible with the field it is being assigned to. Eg:

@InjectService(service=AtomicBoolean.class) Date dateService;

The ServiceExtension will permit this but will cause a ClassCastException.

Admittedly the example is somewhat contrived but it illustrates the point. It would be better if this were caught and reported in a more friendly way rather than allowing the ClassCastException to happen.

kriegfrj added a commit to kriegfrj/osgi-test that referenced this issue Mar 28, 2022
Fixes eclipse-osgi-technology#495

Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
@bjhargrave
Copy link
Contributor

Is this really that important? The situation is clearly a test coding error. If we want, we can catch the CCE at the injection site and convert to a ParameterResolutionException pointing out the coding error. I don't see that we need any more sophistication.

@kriegfrj
Copy link
Contributor Author

Is this really that important? The situation is clearly a test coding error.

Clearly it is a test coding error. But I think it makes for a better UX if user errors are detected early and reported in a user-friendly way. Helps them more quickly identify and fix the precise cause of the error. I feel this is important.

If we want, we can catch the CCE at the injection site and convert to a ParameterResolutionException pointing out the coding error. I don't see that we need any more sophistication.

When injecting a List or ServiceAware there might not be a CCE at the point of injection, but instead it leaves a CCE bomb waiting to donate later. Without the fix in my PR ServiceExtension will happily inject an AtomicBoolean into a List, for example, and the CCE won't happen until you start trying to access the list.

@bjhargrave
Copy link
Contributor

When injecting a List or ServiceAware there might not be a CCE at the point of injection, but instead it leaves a CCE bomb waiting to donate later.

Yes, but that is the other issue you opened (#494). This one appears to be about not List or ServiceAware. Or is this and #494 the same issue duplicated?

@kriegfrj
Copy link
Contributor Author

When injecting a List or ServiceAware there might not be a CCE at the point of injection, but instead it leaves a CCE bomb waiting to donate later.

Yes, but that is the other issue you opened (#494). This one appears to be about not List or ServiceAware. Or is this and #494 the same issue duplicated?

They are closely connected. But #494 was more about types that have wildcard parameters, and the fact that the default type used for the service match will violate type safety on the upper bound (if there is one). This issue (#495) still exists if you have concrete (non-wildcard) parameters.

@bjhargrave
Copy link
Contributor

We either inject the object or a container of the object.

For injecting an object, the injection will fail if the target cannot contain the object. So we can catch the CCE and report as a ParameterResolutionException.

For injecting a container, the injection will always succeed, but there can be casts at the call sites to the injected container. We can only make a best effort to catch those programming errors but can never be 100% successful. But as I stated in one of the other related issues/PRs on this basic topic, we need to not prevent people from doing potentially dangerous things.

kriegfrj added a commit to kriegfrj/osgi-test that referenced this issue Mar 29, 2022
Fixes eclipse-osgi-technology#495

Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants