Feature: NodeSet.has filtering against a NodeSet

[eyalgr]

NodeSet.has can accept a NodeSet value and filter the relationship against it, like so:

Person.nodes.has(pet=Pet.nodes.filter(name='kitty'))

NodeSet.has can accept a NodeSet that itself uses a NodeSet relationship filtering:

Person.nodes.has(
    child=Person.nodes.has(
        child=Person.nodes.filter(name='john')
    )
)

Node naming convention in QueryBuilder was modified - underscores in node names are duplicated as an escaping mechanism.

[aanastasiou]

@eyalgr Hello and thank you for the commit. Can we please clarify how exactly does has, works here? If it replicates the functionality of in, we should keep only one of them. But if it is closer to this, that would be even better.

All the best

[eyalgr]

@aanastasiou Hi, sure, please tell me if I'm wrong but both of these filter nodes based on the properties of the node, whereas in this commit has filters the nodes based on the properties of the targets of the node's relations, which as far as I know wasn't possible without two distinct queries in neomodel before.

Just to clarify, the implicit models in my examples look something like:

class Person(StructuredNode):
    pet =  RelationshipTo('Pet', 'PET')
    child =  RelationshipTo('Person', 'CHILD')
    name = StringProperty()

class Pet(StructuredNode):
    name = StringProperty()

[aanastasiou]

Hello @eyalgr and thank you for the clarification.

Alright, this sounds like an additional useful feature. Can we please clarify the following?

1. This has, seems to make it possible to query for a structure starting from a Node of some label. Can we please clarify how does this translate to cypher queries? Because at the moment, it looks like every subsequent has triggering a query itself (on every top level node)...and this can quickly get you into more trouble than writing down an equivalent parametrisable query in cypher and executing it with cypher_query (?)

2. Node naming convention in QueryBuilder was modified - underscores in node names are duplicated as an escaping mechanism.

   This is kind of "scary" :) Can you please clarify if you foresee breaking any backwards compatibility?

All the best

[eyalgr]

I’ll do my best to clarify, let me know if I misunderstood something -

1. In this implementation, only one cypher query will be issued to the db regardless of the amount of NodeSets involved - note you need to call .all or .get to trigger an actual query. In this implementation, the .has call instantiate a QueryBuilder for the provided NodeSet argument, pulls its ast and uses it to extend its own ast (https://github.com/neo4j-contrib/neomodel/pull/460/files#diff-eeaf1d19783ad16393cac96402adeeadR348)

2. It’s a good question. I think it can only break code that do nasty things with private members, such as the _ast, or tries to manually intervene with the resulting nodeset query. I should note that this commit does exactly that, but I feel like it’s ok because it’s limited to classes inside match.py. It may be a good idea to define a class-name-to-node-name function.

[aanastasiou]

@eyalgr

From a functionality point of view I think it's a useful operator to have.

There are some "caveats" that would have to be mentioned in the documentation I think. For example, to not actually call .all() or .get() on the intermediate NodeSets.

Can you please write a bit about that in a brief documentation section about .has()?

After that, it would be best to make sure that .has() is at least backwards compatible as a plain simple relationship check and that the "juggling" that has() is doing is...not going to break anything :)

All the best

[eyalgr]

Note that passing the result of .all or .get to .has raises a ValueError.

I believe it is backwards compatible, the code flow for boolean arguments is left untouched and the tests are passing. Would you prefer to have tests for the new code too?

Where should I put the documentation?

[aanastasiou]

@eyalgr Regarding the documentation: Any section you think it is suitable. I try to review changes in the documentation anyway.

[eyalgr]

@aanastasiou

While writing the documentation I noticed a bug in this feature, where a single node returns more than once in the query. I fixed that in a new change in this pull request: https://github.com/neo4j-contrib/neomodel/pull/460/commits/bee75bd69d1addd6c5adfddc3eed72133e4585e3 This is very small but critical change, I would like to have your opinion on that.

Either way, the documentation is ready too.

The tests are failing in the initiation phase, not because of the code.

[eyalgr]

Hi @aanastasiou, any news regarding this?

[eyalg-cye]

Hi @aanastasiou, any updates?

[mariusconjeaud]

Closing because this feature will be covered with a follow-up to this, with the same syntax idea : https://github.com/neo4j-contrib/neomodel/pull/723/commits/2ccd44a0d61773c85e204b7f94b145eb9d57064d

This will allow to filter along a path like filter('rel1__rel2__rel3.property="value"

Waiting for this PR to come in is safer because it comes from an extension which has been in production as an extension for a year now, and follows up on this earlier commit with the same syntax so it's more consistent.