Fix ModuleCapability/ModuleRequirement hashcode/equals according to API

[laeubi]

The API of Capability/Requirement currently defines the contract to be: This Capability/Requirement is equal to another Capability/Requirement if they have the same namespace, directives and attributes and are declared by the same resource.

But Equinox currently implements hashCode/equals in terms of object identity.

A similar problem might be in ModuleRevision where it says two resources are equal if:

This Resource is equal to another Resource if both have the same content and come from the same location. Location may be defined as the bundle location if the resource is an installed bundle or the repository location if the resource is in a repository.

I find this a bit unclear what "content" means, in the context of a repository it might be the hashsum but for a Bundle(Revision) I think this is hard to answer.

[github-actions]

Test Results

  660 files  ±0    660 suites  ±0   1h 11m 19s ⏱️ - 1m 3s
2 195 tests ±0  2 108 ✅  -  40   47 💤 ±0  24 ❌ +24  16 🔥 +16 
6 729 runs  ±0  6 468 ✅  - 118  143 💤 ±0  72 ❌ +72  46 🔥 +46 

For more details on these failures and errors, see this check.

Results for commit 46bd105. ± Comparison against base commit c8ce736.

♻️ This comment has been updated with latest results.

[laeubi]

@tjwatson how can we proceed here? I wonder what would be the advantage of Object.equals over the specified behavior, e.g. if Equinox require it on some places then still Object Identity might better be used explicitly (and documented) on these places?

[tjwatson]

@tjwatson how can we proceed here?

I don't think we should proceed here. In the framework each BundleRevision is represented by a distinct installation generation of a Bundle. There are cases where the same Bundle may be installed multiple times if using https://github.com/osgi/osgi/blob/11d440e336ac2c22bade54105ed4ef2c9aab79d8/org.osgi.framework/src/org/osgi/framework/hooks/resolver/ResolverHook.java#L178 to filter out singleton collisions. It is also possible when doing bundle updates which leave behind "pending" removals for a particular BundleRevision, but then the same bundle is installed in a different location.

Equinox regions does this to create isolated regions. So for the case where the "same" bundle JAR (Resource) is installed multiple times (into different "regions"), the framework HAS to treat each BundleRevision instance as identity unique. Since each instance of BundleRevision is identity unique then that implies each Capability and Requirement from a revision is also identity unique. This is by design and should not be changed.

[laeubi]

But "same bundle" then means same bsn and same version is this really something to concern? Otherwise Req/Cap can never be the same (as bundle version would be different). Also as mentioned the Framework internally can still treat them differently than the default equals/hashcode...

[tjwatson]

But "same bundle" then means same bsn and same version is this really something to concern?

Not sure what you mean by concern. Yes same exact JAR is installed multiple times at a different bundle location. For this the framework has to treat each BundleRevision as different even if they are backed by the same exact bundle JAR.

Otherwise Req/Cap can never be the same (as bundle version would be different).

Yes, this is what I am asserting. Each BundleRevision is identity unique so each cap/req is identity unique by design.

Also as mentioned the Framework internally can still treat them differently than the default equals/hashcode...

I see no advantage to that complication when each BundleRevision must be identity unique, just use the default equals/hashcode to get you that behavior.

[laeubi]

But if the underlying BundleRevision == Resource is different then the Req/Cap wont be equal as well so maybe we are talking about different things.

So I think we are safe here, I just wanted to reiterate on what the spec says:

This Resource is equal to another Resource if both have the same content and come from the same location.

So if bundle location is different two resources are not equal.

Now for what is done here (Req/Cap) it says

Capability/Requirement is equal to another Capability/Requirement if they have the same namespace, directives and attributes and are declared by the same resource.

So that should be safe as well from what you described, as each Rev is a different Resource.

[tjwatson]

So that should be safe as well from what you described, as each Rev is a different Resource.

Okay I think we are on the same page then? We agree that the BundleRevision instances managed by the framework must be use identity equals/hashcode which implies that req/cap also can use identity equals/hash. To do extra calculations based on attributes/directives would be a waste because the backing resource (BundleRevision) is always identity unique.

[tjwatson]

I should have mentioned this already. But the main reason each BundleRevision must be treated as identity unique is because each BundleRevision is used as a "recipe" for creating a bundle class loader. That creates a unique class space for the BundleRevision instance. The resolver has to then treat each BundleRevision as unique so that the uses conflicts are properly accounted for the different class loaders.

[laeubi]

which implies that req/cap also can use identity equals/hash.

The spec says something different and I don't see why Req/Cap must use identity equals/hash here.

To do extra calculations based on attributes/directives would be a waste because the backing resource (BundleRevision) is always identity unique.

A common short-cut for this is to do identity comparison first, so I don't see we loose much here but just assume I do wrap a Requirement (e.g. bnd does that because Req/Cap and Resource are all immutable objects) and then compare it to the original Requirement. For clarity we now name them BndRequirement and EquinoxRequirement, what now happens is

• BndRequirement.equals(EquinoxRequirement) = true (because bnd follows OSGi Spec)
• EquinoxRequirement.equals(BndRequirement) = false (because Equinox NOT follow OSGi Spec and use identity)

even though (from the perspective of the spec) must return equal in both cases.

[tjwatson]

A common short-cut for this is to do identity comparison first

I don't see how that works. Identity comparison is false and you fall back to the other non-identity comparison just makes it always use non-identity comparison which will have it return true when it should be false.

[laeubi]

it always use non-identity comparison which will have it return true when it should be false.

Why should it return true when false is expected?

[tjwatson]

Why should it return true when false is expected?

I've lost track on what you are proposing or what problem you are trying to fix. Perhaps an update to the PR would help?