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

Update OSGi Bundling #58

Merged
merged 6 commits into from
Mar 21, 2022
Merged

Conversation

bstopp
Copy link
Collaborator

@bstopp bstopp commented Mar 19, 2022

First - this PR relies/assumes on #49 being merged - so that update is already here in this PR. Due to the number of updates in the pom, if i went back to the baseline main branch, there would be a number of conflicts. I assume there will be discussion on this PR, so I'll rebase after the other is merged. Now, details:

This PR is based on a few assumptions and expectations of following OSGi best practices, as follows:

  1. Jackson should not be exported from the shared library.

    • This needs to be provided by the container
    • AEM Provides v2.9 w/ 6.5.0; the Manifest was configured to bind to this version up to, but not including, 3.0.
  2. The use of Feign is an implementation detail of the embedded Jars. It should not be exported from the libraries and exposed to the OSGi context

  • All references to feign were moved to non-exported packages
  • This changed the API builders - the impls must have a public constructor
  • OSGi Classloaders wont be able to use these as they are not exported from the bundle.
  • This detail can be "hidden" from consumers in non-OSGi context by excluding from JavaDoc.
  • It's moot in non-OSGi contexts as consumers of libraries have other means to access the previously protected constructors and they do so at their own risk.
  1. Because the use Feign is now abstracted from library users - the JWT Authentication Interceptor is removed from all OSGi references as well.
  • The services are responsible for creating and managing their interceptors.
  • The interceptor is built from the Workspace, and the services require the workspace already to perform operations

There is still one OSGI warning that I cannot remove and will likely cause issues:

aio-lib-java/aem/lib_osgi/src/main/bnd/aio-lib-osgi.bnd [0:0]: Export com.adobe.xdm.common, has 1, private references [javax.validation.constraints]

The Shared Library is exporting the Adobe XDM Event common library. This has one class XDMEvent that relies on the NotNull constraint. This a Runtime retained annotation, and therefore the classloader/classpath needs it at runtime to perform correct processing. As far as I can tell, this package does not exist in OSGi/Sling/AEM OOTB. And given it's context, I don't think it's appropriate to export it.

I honestly have no idea if this will or won't cause issues when the XDMEvent is used in the AEM classpath.

@francoisledroff
Copy link
Collaborator

Thanks a lot for this osgi clean up.
It does look really good.
I had to push a few fixes on top see #60
for the osgi warning pointing to xdm, here is the xdm repo that we fully own too, so we could remove the offending javax.validation.constraints from there too
see https://github.com/adobe/xdm-event-model

@francoisledroff francoisledroff merged commit 4124262 into adobe:main Mar 21, 2022
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 this pull request may close these issues.

2 participants