BROOKLYN-379: Fix rebind when bundle prefixes are used to load classes #442
Conversation
| Bundle b = bundle.get(); | ||
| Class<T> clazz; | ||
| Optional<String> strippedType = osgiClassPrefixer.stripMatchingPrefix(b, type); | ||
| String typeToLoad = strippedType.isPresent() ? strippedType.get() : type; |
There was a problem hiding this comment.
@aledsage wrote 9 days ago:
Not sure if this is the right place for the fix! Arguably if it has the prefix, we should only look in one bundle (or multiple versions of the bundle), rather than iterating over all bundles.
@neykov responded 7 days ago:
If a class has bundle prefix obviously it's not relying on the libraries functionality to load it. Still could be useful to have this to avoid version mismatch (half of the blueprint is coming from libraries section the other half is using prefixes because of copy&paste).
I'd structure it a bit differently - if there's a prefix short-circuit and don't try to load it from non-matching bundles, but can't get away from iterating over them. And it's not all, just those in the libraries section so it's fine.
The prefix could include version as well - would be nice to support it here as well (by skipping non-matching versions) to avoid introducing subtle inconsistencies in the prefix behaviour.
@neykov responded 7 days ago:
This makes more sense now that I saw the commit comment. This is called by persistence, not only by yaml parsing code.
Another approach would be to skip writing the prefix if it the class comes from bundles in the memento's catalog item.
There was a problem hiding this comment.
Good points @neykov. I'll strip the version from the prefix as well.
I'll let it match by just comparing the symbolic name (even if wrong version number) because that will better support upgrades/rollback of bundles, where the symbolicName:version:className has been persisted (I'd hope we miss out the version number in persisted state for that reason though).
I'd prefer not to try skipping the prefix "if the class comes from the bundles in the memento's catalog item". It makes sense at a high level, but I worry about situations where there are nested catalog items or sub-types (where different levels declare their libraries). We should handle such cases, but it feels to me like belt-and-braces if we include the bundle id here as well. If we do want to skip writing the prefix in such cases, let's look at that in a separate PR.
| public Optional<String> stripMatchingPrefix(Bundle bundle, String type) { | ||
| String symbolicName = bundle.getSymbolicName(); | ||
| if (symbolicName != null && type.startsWith(symbolicName + ":")) { | ||
| return Optional.of(type.substring((symbolicName + ":").length())); |
There was a problem hiding this comment.
@neykov wrote: The prefix could include version identifier as well.
|
|
||
| public Optional<String> getPrefix(Class<?> type) { | ||
| Optional<Bundle> bundle = Osgis.getBundleOf(type); | ||
| if (bundle.isPresent() && !whiteListRetriever.isBundleWhiteListed(bundle.get())) { |
There was a problem hiding this comment.
@neykov wrote 7 days:
Don't like how the white list is proliferating throughout the code base. It's supposed to go the other way :).
What do we gain by stripping the prefix for white listed bundles? I think not much because if classes move bundles then their package will change anyway. This can be handled by the renaming list.
Removing this will simplify the code and we don't lose much if anything.
There was a problem hiding this comment.
On the plus side, this is just moving the white list code (rather than proliferating). It is deleted from XmlMementoSerializer.OsgiClassnameMapper, which now delegates to this code. It does mean the white-list code is called slightly more often, but thus in a more consistent manner for handling prefixes.
Let's revisit the white list in a separate PR (potentially on the dev@brooklyn mailing list first).
aledsage
force-pushed
the
neykov-aled/test/osgi-prefix-rebind
branch
from
49b6535 to
f4d3c18
Compare
|
@neykov can you review this again please? |
aledsage
force-pushed
the
neykov-aled/test/osgi-prefix-rebind
branch
from
f4d3c18 to
ea9084e
Compare
Without this, RebindOsgiTest.testUsesCatalogBundleVersion fails
aledsage
force-pushed
the
neykov-aled/test/osgi-prefix-rebind
branch
from
ea9084e to
bad3280
Compare
| if (osgiClassPrefixer.hasPrefix(typeToLoad)) { | ||
| bundleProblems.put(osgiBundle, new UserFacingException("Bundle does not match prefix in type name '"+typeToLoad+"'")); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
getResource(s) needs the same update. Turns out we can't load prefixed resources when combined with brooklyn.libraries.
Could be addressed as another PR.
|
LGTM, merging. |
3 participants
This replaces #396
See also neykov#4 (but I'll replay all the comments from that PR into this PR).