spring-projects / spring-data-neo4j

Provide support to increase developer productivity in Java when using Neo4j. Uses familiar Spring concepts such as a template classes for core API usage and lightweight repository style data access.
http://spring.io/projects/spring-data-neo4j
Apache License 2.0
828 stars 620 forks source link

@DynamicLabel with non-abstract class inheritance #2886

Closed LGDOI closed 6 months ago

LGDOI commented 6 months ago

SDN 6.3.18 returns incorrect instance type when a dynamic label is set on a node.

I have an example repo but here is a summary of the problem.

Entities

Given the following entity types

@Getter
@Setter
@NoArgsConstructor
@Node(primaryLabel = "Fruit")
public abstract class Fruit {

  @Id
  protected String id;

  @JsonIgnore
  @DynamicLabels
  protected Set<String> labels = Set.of();
}

Subclass of Fruit


@Getter
@Setter
@NoArgsConstructor
@Node(primaryLabel = "MagicalFruit")
public class MagicalFruit extends Fruit {

  @Serial
  private static final long serialVersionUID = -8776591636750117301L;

  @Property(name = "volume")
  private double volume;

  @Property(name = "color")
  private String color;

  @Override
  public int hashCode() {
    return new HashCodeBuilder().append(id).hashCode();
  }

  @Override
  public boolean equals(Object obj) {
    return obj == this || (obj instanceof MagicalFruit other && Objects.equals(id, other.id));
  }
}

Sub classes of MagicalFruit

@Getter
@Setter
@NoArgsConstructor
@Node(primaryLabel = "Apple")
public class Apple extends MagicalFruit {

  @Override
  public boolean equals(Object obj) {
    return obj == this || (obj instanceof Apple other && Objects.equals(id, other.id));
  }
}

and

@Getter
@Setter
@NoArgsConstructor
@Node(primaryLabel = "Orange")
public class Orange extends MagicalFruit {

  @Override
  public boolean equals(Object obj) {
    return obj == this || (obj instanceof Orange other && Objects.equals(id, other.id));
  }
}

Repository

@Repository
public interface FruitRepository extends Neo4jRepository<Fruit, String> {
  @Query("MATCH (f:Fruit) RETURN f")
  List<Fruit> findAllFruits();
}

Test

@DataNeo4jTest
class FruitRepositoryTest(
  val fruitRepository: FruitRepository,
) {

  @Test
  fun `debug dynamic label and deserialization`() {
    val applesWithDynamicLabel = List(2) {
      Apple().apply {
        volume = it.toDouble()
        color = "Red"
        labels = setOf("Apple_$it")
      }
    }
    val applesWithoutDynamicLabel = List(2) {
      Apple().apply {
        volume = it.toDouble()
        color = "Blue"
      }
    }
    val orangesWithDynamicLabel = List(2) {
      Orange().apply {
        volume = it.toDouble()
        color = "Red"
        labels = setOf("Orange_$it")
      }
    }
    val orangesWithoutDynamicLabel = List(2) {
      Orange().apply {
        volume = it.toDouble()
        color = "Yellow"
      }
    }
    fruitRepository.saveAll(
      applesWithDynamicLabel + applesWithoutDynamicLabel + orangesWithDynamicLabel + orangesWithoutDynamicLabel)

    val fruits = fruitRepository.findAllFruits()
    assertThat(fruits.filterIsInstance<Apple>()).hasSize(4)
    assertThat(fruits.filterIsInstance<Orange>()).hasSize(4)
  }
}

The above test fails because fruits contains instances of MagicalFruit when dynamic labels are set.

Other Findings

When I change MagicalFruit class to an abstract class, the same test pass.

https://github.com/spring-projects/spring-data-neo4j/issues/2529 is very similar bug but that example has abstract on Feline class.

Is this required to have only one concrete class as a leaf node of the inheritance hierarchy for dynamic label to work?

michael-simons commented 6 months ago

Thanks @LGDOI for the detailed report. I can reproduce it and have a fix ready by the end of the day.

LGDOI commented 6 months ago

Thank you @michael-simons for fixing this bug so quickly.

LGDOI commented 6 months ago

Hi @michael-simons Do you know when this change will be released as SDN 6.3.19? I saw 7.x release today but no 6.3.x. Thank you.

michael-simons commented 6 months ago

@mp911de Would you consider crafting a single 6.3.x release for SDN?

mp911de commented 6 months ago

We would require a full release train as we do not manage individual artifact versions. Spring Data 2021.2 is out of support since November last year. Let me find out more details.

michael-simons commented 6 months ago

Thanks for looking into this, Mark!

michael-simons commented 6 months ago

Hello @LGDOI

We talked about this topic with our friends at Broadcom, who do mange the releases for all Spring Data projects.

They cannot release Spring Data Neo4j individually, not due to technical, but legal restrictions imposed by support contracts for older versions.

The options for you that we see here are:

LGDOI commented 6 months ago

Thank you @michael-simons for recommending the possible options for us. I will forward your recommendations to our team and decide which path to take.

Cheers! Joji