-
Notifications
You must be signed in to change notification settings - Fork 150
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
Move interface projection to cypher builder #2030
Move interface projection to cypher builder #2030
Conversation
|
…ion-to-cypher-builder
Thanks for the UI updates. The UI has now been torn down - reopening this PR will republish it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nothing really to add apart from maybe double-checking if the comments on your new code are (still) needed.
c820d7a
to
7e0baa2
Compare
7e0baa2
to
42f3075
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good, some nice improvements on top of the refactor! It might even address the bug I have fixed in my PR, time will tell when I merge!
MATCH (this)-[:ACTED_IN]->(this_Series:Series) | ||
CALL apoc.util.validate(NOT ((this_Series.episodes IS NOT NULL AND this_Series.episodes = $this_Seriesauth_param0)), \\"@neo4j/graphql/FORBIDDEN\\", [0]) | ||
MATCH (this)-[thisthis1:ACTED_IN]->(this_Series:\`Series\`) | ||
WHERE apoc.util.validatePredicate(NOT ((this_Series.episodes IS NOT NULL AND this_Series.episodes = $thisparam0)), \\"@neo4j/graphql/FORBIDDEN\\", [0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is super slick, I love it! 👏
|
||
function createInterfaceProjectionAndParams({ | ||
export default function createInterfaceProjectionAndParams({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for the default export? It feels to me like we're moving away from this direction in most cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it is just because that was what were being exported, and to reduce the scope of the refactor I kept it as such
MATCH (this)-[:HAS_CONTENT]->(this_Post:Post) | ||
WHERE (exists((this_Post)<-[:HAS_CONTENT]-(:\`User\`)) AND all(auth_this0 IN [(this_Post)<-[:HAS_CONTENT]-(auth_this0:\`User\`) | auth_this0] WHERE (auth_this0.id IS NOT NULL AND auth_this0.id = $this_Postauth_param0))) | ||
MATCH (this)-[thisthis1:HAS_CONTENT]->(this_Post:\`Post\`) | ||
WHERE (exists((this_Post:\`Post\`)<-[:HAS_CONTENT]-(:\`User\`)) AND all(thisthis2 IN [(this_Post:\`Post\`)<-[:HAS_CONTENT]-(thisthis2:\`User\`) | thisthis2] WHERE (thisthis2.id IS NOT NULL AND thisthis2.id = $thisparam0))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we're currently suffering from this bug with EXISTS
, but I feel like we should shortly (or maybe even already) be in a position where we can move to using EXISTS
clauses across the board! 🥳
Description
This PR refactors interface projections so they are generated with CypherBuilder. This allows to remove duplicate code regarding filtering