Skip to content

'findByProperty' not finishing. It tries to load whole graph #2137

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
mrksph opened this issue Feb 8, 2021 · 4 comments
Closed

'findByProperty' not finishing. It tries to load whole graph #2137

mrksph opened this issue Feb 8, 2021 · 4 comments
Assignees

Comments

@mrksph
Copy link

mrksph commented Feb 8, 2021

Spring Boot: 2.4.2

I have the following Model:

@Node
public class Country {
  private String code;

  @Relationship(type="HAS_CURRENCY", direction = Relationship.Direction.OUTGOING)
  private Set<Currency> currencies = new HashSet<>();

}

Note: A HAS_CURRENCY example: (Country) - [HAS CURRENCY] -> (Currency)

@Node
public class Currency {
  private String name;
}
@Repository
public interface CountryRepository extends Neo4jRepository<Country, Long> {
  Optional<Country> findByCode(String code);
}

If I do countryRepository.findByCode("CODE"), Spring Data executes the following query

MATCH (n:`Country`) 
WHERE n.code = "ES" 
RETURN n{ .code,
__nodeLabels__: labels(n),
__internalNeo4jId__: id(n),
__paths__: [p = (n)-[:`HAS_CURRENCY`]->()-[:`HAS_CURRENCY`*0..]-() | p]}

Expected Behavior:
I expect countryRepository.findByCode("CODE") to return the Country with the given "CODE" and its Currencies.

Actual Behavior:
The actual behavior is that Spring Data Neo4j tries to load the whole graph. I think that is because when Spring Data generates the query, it won't add the direction in the second depth level.

How it is:

__paths__: [p = (n)-[:`HAS_CURRENCY`]->()-[:`HAS_CURRENCY`*0..]-() | p]}

How it should be:

__paths__: [p = (n)-[:`HAS_CURRENCY`]->()-[:`HAS_CURRENCY`*0..]->() | p]}
@mrksph mrksph changed the title 'findByProperty' not finishing 'findByProperty' not finishing. It tries to load whole graph Feb 8, 2021
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 8, 2021
@meistermeier meistermeier self-assigned this Feb 10, 2021
@meistermeier
Copy link
Collaborator

Hi.
There seems to be something missing in the model you provide.
I just followed along your explanation and end up with a non-cycle model (https://github.com/meistermeier/neo4j-issues-examples/tree/master/sdn-gh2137).
As a result the query is not path-based like you describe but the standard nested relationships style:

MATCH (n:`Country`) WHERE n.code = $code RETURN n{.code, __nodeLabels__: labels(n), __internalNeo4jId__: id(n), Country_HAS_CURRENCY_Currency: [(n)-[:`HAS_CURRENCY`]->(n_currencies:`Currency`) | n_currencies{.name, __nodeLabels__: labels(n_currencies), __internalNeo4jId__: id(n_currencies)}]}

I assume that the Currency has a backward relationship to the Country that you did not mention, right?
Besides this, if your model defines for one Country to fetch the whole graph, SDN will do this. We are right now mitigating the problem that a query can make the application (and database) unusable by moving towards cascading queries. This will still fetch the whole graph if the model is defined this way but improve the situation a lot because the path(s) / path segments do not stack up in the database's prepared return collection.

@meistermeier meistermeier added blocked: awaiting feedback and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 10, 2021
@mrksph
Copy link
Author

mrksph commented Feb 10, 2021

Hi @meistermeier,

Yes, you're right. My assumptions were wrong. To reproduce the behavior I must add another relationship to the example Country entity.

The missing relationship is between two Countries, DEPENDENCY_OF is like "A Country that depends on another Country".

So when querying by "findByCode" the generated query uses both Relationships types to hydrate the fields. The HAS_CURRENCY is not a problem because the Target node is of another type. The problem is with the DEPENDENCY_OF relationship, as the target node is the same type of the source node.

The generated query uses an OR operator without a direction arrow so the resultant query tries to load the whole graph.

MATCH (n:`Country`) 
WHERE n.code = $code 
RETURN n{.code, .id, 
__nodeLabels__: labels(n), 
__internalNeo4jId__: id(n), 
__paths__: [p = (n)-[:`DEPENDENCY_OF`|`HAS_CURRENCY`]->()-[:`DEPENDENCY_OF`|`HAS_CURRENCY`*0..]-() | p]}

(dependentCountry:Country)-[DEPENDENCY_OF]->(parentCountry:Country)

Also:

(country:Country)-[HAS_CURRENCY]->(currency:Currency)

meistermeier added a commit that referenced this issue Feb 12, 2021
This mitigates the data that is generated in the database's memory
upfront returning.
Otherwise this could lead to un-responsive applications.
meistermeier added a commit that referenced this issue Feb 12, 2021
This mitigates the data that is generated in the database's memory
upfront returning.
Otherwise this could lead to un-responsive applications.
meistermeier added a commit that referenced this issue Feb 12, 2021
This mitigates the data that is generated in the database's memory
upfront returning.
Otherwise this could lead to un-responsive applications.
@meistermeier
Copy link
Collaborator

We changed the path-based queries now to cascading matches. It will still look for everything that is reachable with the given domain model but returns a result (#2140)

@mrksph
Copy link
Author

mrksph commented Apr 8, 2021

Hi Gerrit, I think this Issue raised another Bug. The cascading matches (I think those are the OPTIONALs MATCHes I've been encountering with) is making ours findByProperties queries very slow.

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

Successfully merging a pull request may close this issue.

3 participants