MavenTargetLocation equals comparison never returns true

[haubi]

In the TargetPlatformPreferencePage, upon the Apply button, some target definition containing some MavenTargetLocation always is rewritten to it's .target file, when MavenTargetDependency lacks an impl of equals(), causing MavenTargetLocation.roots to never be equal.

[laeubi]

@haubi you need to increment the version for this change.

One thing I wonder is if we should move the equals check if the location to MavenTargetLocation do you see any issue with that? The reason is that a MavenTargetDependency can be "bound" to a target location and therefore it makes it quite hard to have a true equals method in the sense of its definition in the java language.

[haubi]

Agreed, having the equals check in MavenTargetLocation does make sense.

Meanwhile, it turns out that MavenTargetLocation.equals ignores lots of other attributes right now - which becomes relevant when MavenTargetDependency.equals eventually returns true.
But then, adding explicit comparison for each and every relevant nested attribute is a pita, so I prefer to compare along serialize() instead.

[github-actions]

Test Results

  214 files  ±0    214 suites  ±0   16m 53s ⏱️ +16s
  664 tests ±0    654 ✅ ±0  10 💤 ±0  0 ❌ ±0 
1 328 runs  ±0  1 306 ✅ ±0  22 💤 ±0  0 ❌ ±0 

Results for commit 26bbd40. ± Comparison against base commit c3fa2ed.

♻️ This comment has been updated with latest results.

[HannesWell]

In general one could consider to cache the result of serialize() in order to speed-up equals(), but I assume that method is not called that often for a MavenTargetLocation so it is probably not relevant, is it?

[HannesWell]

After thinking about this again, we should cache the result of serialize(), either in the serialize() method itself, computing the value lazy or in the constructor where we could consider to even use the original string from which the target was parsed (if so).

[HannesWell]

If equals is changed, hashCode() should be adjusted in the same way.

[laeubi]

@haubi what I'm wondering and as you have already analyzed that, can you probably point out where exactly the equals check fails in the PDE code?

I wondering if instead of changing m2e, it might generally be better to change the code there to always check the serialize() for every location in an equals check (not only m2e ones).

[haubi]

Using serialize() for hashCode() is fine with me.

While I understand the motivation behind caching the serialize() value, I believe we are unable to catch each and every situation where (deeply nested) attributes can be modified, which would require the cached value to be flushed.
Alternatively, we could think of refactoring to some real immutable object, but would that make sense?

[laeubi]

Maven Target locations should be immutable already, do you see any mutated state here?

[haubi]

@haubi what I'm wondering and as you have already analyzed that, can you probably point out where exactly the equals check fails in the PDE code?

@laeubi Well, here's my journey:

• List<MavenTargetDependency> roots: fails because of missing MavenTargetDependency.equals - this was my first try
• Then I figured that there are quite some other relevant members that are ignored by equals, so I started to use Objects.equals on them, but then figured that:
• List<MavenTargetRepository> extraRepositories: fails because of missing MavenTargetRepository.equals - ok, would be easy
• IFeature featureTemplate: not sure how to guarantee proper IFeature.equals impls
• Map<String, BNDInstructions> instructionsMap: fails because of missing BNDInstructions.equals, but playing around with the editor I managed to end up with comparing the empty map against a map with empty values, and this was when I headed over to use serialize.

I wondering if instead of changing m2e, it might generally be better to change the code there to always check the serialize() for every location in an equals check (not only m2e ones).

Not sure where you mean by "there", but indeed I have thought on something similar too:
Compare the serialized values right in TargetPlatformPreferencePage.performOk (see also the backtrace in eclipse-pde/eclipse.pde#1059) instead of using ((TargetDefinition) original).isContentEqual(def), which seems strange anyway .

[laeubi]

Not sure where you mean by "there"

You mentions that you faced "In the TargetPlatformPreferencePage, upon the Apply button, some target definition containing some MavenTargetLocation always is rewritten to it's .target file"

So I would think about that lets say there is even other location types, maybe this check should be enhanced "there" (in the TargetPlatformPreferencePage or related code) to always compare a normalized serialized version of a target location, for example I would assume the following does not matter:

1. Order of locations
2. Order of attributes
3. Whitespaces
4. Line endings

so just roughly speaking, parse the XML of previous state and compare it with the parsed XML on new state to decide if something has to change and if not keep the file as is.

So given a location always produces the same XML equivalent (not necessary text representation) it should never touch the file.

[haubi]

Maven Target locations should be immutable already, do you see any mutated state here?

Well, maybe I'm misinterpreting, but this is what makes me believe it should be mutable:

• setExcluded modifying a relevant member
• getDependencyScopes returning a potentially mutable Collection
• getRoots returning a mutable List
• getFeatureTemplate not sure if IFeature is immutable
• String label is not final (ok, could be without warnings/errors)

[haubi]

So I would think about that lets say there is even other location types, maybe this check should be enhanced "there" (in the TargetPlatformPreferencePage or related code) to always compare a normalized serialized version of a target location,

Well, TargetPlatformPreferencePage uses at least two variants of comparisons using equals:

1. ((TargetDefinition) fPrevious).isContentEquivalent, not comparing the name
   https://github.com/eclipse-pde/eclipse.pde/blob/89bf2f0a1a25f6dd4278accd6f38415ec7c328ad/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/preferences/TargetPlatformPreferencePage.java#L886
2. ((TargetDefinition) original).isContentEqual(def), comparing the name as well
   https://github.com/eclipse-pde/eclipse.pde/blob/89bf2f0a1a25f6dd4278accd6f38415ec7c328ad/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/preferences/TargetPlatformPreferencePage.java#L939

Feels like not using Arrays.equals would make TargetDefinition.isContentEquivalent and TargetDefinition.isContentEqual less readable, and "implementing proper equals is difficult" feels like some poor argument, no?

[laeubi]

Yes I think org.eclipse.pde.internal.core.target.TargetDefinition.isContentEqual(ITargetDefinition) and isContentEquivalent is where we actually need a fix!

As mentioned the order of locations is irrelevant (what is currently checked!) and the locations itself are better compared by their serialized representation instead of their object representation!

[haubi]

@laeubi Did I understand it correctly in that you want something like:
eclipse-pde/eclipse.pde#1127

[laeubi]

@haubi yes, sorry for the confusion here but I think that is much more felxible and usefull than what we can do here at m2e (even though equals/hashcode should be improved of course!

[haubi]

@laeubi Well, it turns out that not each implementation of ITargetLocation really implements serialize - and indeed, TargetDefinition.serializeBundleContainers does not use serialize for some implementations.

[laeubi]

@laeubi Well, it turns out that not each implementation of ITargetLocation really implements serialize - and indeed, TargetDefinition.serializeBundleContainers does not use serialize for some implementations.

Yes this is some left overs from "old implementation, best actually would be to implement serialize always in the container ... thats really a mess.

[HannesWell]

@haubi is this now obsolete since eclipse-pde/eclipse.pde#1127 is submitted?

[haubi]

Sure!

[laeubi]

@haubi thanks for working on this one!