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

@DynamicLabel with non-abstract class inheritance #2886

Closed
LGDOI opened this issue Apr 1, 2024 · 8 comments · Fixed by #2887
Closed

@DynamicLabel with non-abstract class inheritance #2886

LGDOI opened this issue Apr 1, 2024 · 8 comments · Fixed by #2887
Assignees
Labels
type: bug A general bug

Comments

@LGDOI
Copy link

LGDOI commented Apr 1, 2024

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.

#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?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 1, 2024
@michael-simons michael-simons self-assigned this Apr 2, 2024
@michael-simons
Copy link
Collaborator

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

@LGDOI
Copy link
Author

LGDOI commented Apr 2, 2024

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

@LGDOI
Copy link
Author

LGDOI commented Apr 12, 2024

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
Copy link
Collaborator

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

@mp911de
Copy link
Member

mp911de commented Apr 15, 2024

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
Copy link
Collaborator

Thanks for looking into this, Mark!

@michael-simons
Copy link
Collaborator

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:

  • Upgrade to a 7.x release on your earliest convenience. While we did not have any significant changes in SDN that would affect straight usage of SDN, some behaviour has changed, mostly around id() vs elementId() (the Neo4j functions with the same name and how we use them as internal object identifiers), and of course will require a Spring Boot upgrade if you use it from Boot (whose version you are on is out of support as well). Anyway, it will be the most future proof and recommended solution.
  • You take the bits here and publish them into a repository of your liking
  • You are a Neo4j enterprise customer in which case we would consider releasing 6.3.x under separate coordinates for you
  • You go on a support contract with Broadcom

@LGDOI
Copy link
Author

LGDOI commented Apr 23, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants