Skip to content

Add association support for DTO projections #2305

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

Closed
pelletier197 opened this issue Jun 22, 2021 · 7 comments
Closed

Add association support for DTO projections #2305

pelletier197 opened this issue Jun 22, 2021 · 7 comments
Assignees
Labels
status: feedback-provided Feedback has been provided type: enhancement A general enhancement

Comments

@pelletier197
Copy link

pelletier197 commented Jun 22, 2021

Given I have an entity that has a single relationship to another entity. If I want to have a projection of that entity with a nullable related entity, the request fails.

interface TestPersonRepository : Neo4jRepository<PersonEntity, String> {
    @Query("""
        MATCH (person:Person) 
        OPTIONAL MATCH (person)-[:WORKS_IN]->(department:Department)
        return person, department
    """)
    fun findPeopleWithDepartments() : List<PersonProjection>
}

data class PersonProjection(
    val person: PersonEntity,
    val department: DepartmentEntity?
)

@Service
class TestService(
    private val repository: TestPersonRepository
)
{
    init {
            val results = repository.findPeopleWithDepartments() // Throws exception
            println(results) 
    }
}

I actually saw a related issue that is marked as resolved, but I think the null use-case was forgotten in 6.1.2: #2269

The stacktrace I get is

Caused by: org.neo4j.driver.exceptions.value.NotMultiValued: NULL is not iterable
	at org.neo4j.driver.internal.value.ValueAdapter.values(ValueAdapter.java:401)
	at org.neo4j.driver.internal.value.ValueAdapter.values(ValueAdapter.java:395)
	at org.springframework.data.neo4j.core.mapping.DefaultNeo4jEntityConverter.determineQueryRoot(DefaultNeo4jEntityConverter.java:127)
	at org.springframework.data.neo4j.core.mapping.DefaultNeo4jEntityConverter.read(DefaultNeo4jEntityConverter.java:102)
	at org.springframework.data.neo4j.core.mapping.DefaultNeo4jEntityConverter.read(DefaultNeo4jEntityConverter.java:67)
	at org.springframework.data.neo4j.core.mapping.DtoInstantiatingConverter.lambda$getPropertyValueFor$4(DtoInstantiatingConverter.java:160)
	at org.springframework.data.neo4j.core.mapping.DtoInstantiatingConverter.getPropertyValueFor(DtoInstantiatingConverter.java:173)
	at org.springframework.data.neo4j.core.mapping.DtoInstantiatingConverter$1.getParameterValue(DtoInstantiatingConverter.java:102)
	at org.springframework.data.mapping.model.ClassGeneratingEntityInstantiator.extractInvocationArguments(ClassGeneratingEntityInstantiator.java:269)

The source of this is in DefaultNeo4jEntityConverter.

Iterable<Value> recordValues = mapAccessor instanceof Value && ((Value) mapAccessor).hasType(nodeType) ?
		Collections.singletonList((Value) mapAccessor) : mapAccessor.values(); // <-- this last part throws when the single entity result is null, since the mapAccessor is a `NullValue`.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 22, 2021
@pelletier197 pelletier197 changed the title Projection with a single result Projection with a nullable single result Jun 22, 2021
@meistermeier meistermeier self-assigned this Jun 22, 2021
@meistermeier
Copy link
Collaborator

Thanks for reporting this. It was not really forgotten but more something that we just had not on our list at all (now it is).

@meistermeier meistermeier added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 22, 2021
meistermeier added a commit that referenced this issue Jun 23, 2021
meistermeier added a commit that referenced this issue Jun 23, 2021
@meistermeier
Copy link
Collaborator

meistermeier commented Jun 23, 2021

TBH I cannot reproduce the particular issue, you are facing...but
Your DTO projection is based on the wrong assumptions: The PersonProjection should already define the PersonEntity fields instead of encapsulating it. An example can be seen here https://github.com/spring-projects/spring-data-neo4j/compare/6.1.x...issue/gh-2305#diff-e7c31326d3475b6bb745de782319b4c3c3261bff1651348184a52eead5aa9cd8R43
Also I added a test case for your scenario, or at least how I understand it: https://github.com/spring-projects/spring-data-neo4j/compare/6.1.x...issue/gh-2305#diff-870bdc2d6110e1d562badf679618d645bcdd33d31c21bf16cc505f528f99b3c6R1292
With this you would also have to return the relationship from the query to not just join two arbitrary nodes in the projection object but only ones with the right relationship type.

If you could clarify how my example differs from yours, this would be really helpful to give you more input on your problem.

Your question also helped us to uncover another missing behaviour around the DTO projections, thanks.

@meistermeier meistermeier added the status: waiting-for-feedback We need additional information before we can continue label Jun 23, 2021
@pelletier197
Copy link
Author

The links you provided don't seem to work for me. However, I looked at the code on your branch and I think that the difference with my example is that the PersonProjection is not annotated with @Node, in comparison to the OneToOneSource.

Also, I really don't see why passing the relation would be necessary here? My projection object does not specify any relation in it. I want to project a person with a possibly null department. What if there was more than one relation to get from person to department?

========

The PersonProjection should already define the PersonEntity fields instead of encapsulating it.

I don't know if it's an official feature, but encapsulation does work (except with my use-case of a null one to one projection). I find it much more convenient to use encapsulation than copying 15 fields of Person, don't you think?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 23, 2021
@meistermeier
Copy link
Collaborator

(Updated the links above, somehow the GitHub magic creates something "interesting" out of this link format)
A projection is based on a modeled entity. You cannot project some independent values in the context of a domain / entity specific operation. And as a consequent only existing fields from the original entity are valid to get hydrated.
(Note: there are some special cases where you can use Spring Expression Language but this is out of this scope here)
If you want to have also the related node in there, Spring Data Neo4j needs to know "how" (relationship type) they are connected. This is the reason why the relationship needs to get returned by the query.
The projection itself is also not annotated with @Node in the example I am referring to but shows a valid DTO projection.

If you really, really want to have this objects, you would have to use the Neo4jClient and map those fields independently.
This would be similar to #2288 (comment)

@meistermeier meistermeier added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 23, 2021
@pelletier197
Copy link
Author

I see. In your example, I think the difference is that i'm projecting an entity. with another entity, while you seem to project an entity with a property. Also, it's important to know that the entity returned is null and that I do not have an association between Person and Department in the entities themselves. My code example above uses those two entities:

@Node("Person")
data class PersonEntity(
    @Id
    val id: String,
    val email: String,
    val fullName: String,
)

@Node("Department")
data class DepartmentEntity(
    @Id
    val id: String,
    val name: String
)

In my graph, I have a relation between those two entities that is not defined in my domain entity.

  • FYI, it was implemented this way for performance purpose. We would have around 10 levels of nested entities in the person entity, and some of those levels might have tens of thousands of connected relations. It's not realistic to represent our domain with @Relationship.

Anyway, it seems i'll be using the Neo4jClient instead with an object mapper to map the result. I understand why you implemented projections this way. Maybe i'm lacking some understanding of the projections, but this way of doing things seems to bring complexity to mapping, while also forcing me (and i'm likely not the only one) to do a lot of custom mapping for my queries with the Neo4jClient even if I just want to do a direct field for field mapping.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 23, 2021
@meistermeier
Copy link
Collaborator

Maybe i'm lacking some understanding of the projections, but this way of doing things seems to bring complexity to mapping.
I would say that your interpretation of projections is different. For Spring Data (and also in the domain driven design sense) a projection is just another "view" on the entity itself. It should not been seen as a data collector or similar.

I will close this issue now but give you a small preview of the future: There will probably nested projections coming up. So you can describe your graph model "completely" in SDN but for load and persist operations use projections as a kind of blueprint for the data that you really want to fetch or persist. This will improve not only the performance of fetching data (by just not fetching unneeded data) but also the way data gets persisted without touching / updating unchanged parts.

@pelletier197
Copy link
Author

pelletier197 commented Jun 23, 2021

I do get how projection work in spring data, tho I find that occasionally useful, my usage of Neo4j really does not fit the projection's use-case at all. That's easy to see with all the problems I get while using them.

On the other hand, If I don't want projection, but I want a data collector, because my queries and my model is too complex for spring data, I have to use the Neo4jClient directly and I cannot rely on spring data repositories at all.

@meistermeier meistermeier changed the title Projection with a nullable single result Add association support for DTO projections Jul 15, 2021
@meistermeier meistermeier added this to the 6.1.3 (2021.0.3) milestone Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants