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

Fix relationship filter on interfaces #5890

Merged
merged 8 commits into from
Dec 19, 2024

Conversation

Masadow
Copy link

@Masadow Masadow commented Dec 14, 2024

Description

Note

Please provide a description of the work completed in this PR below

This line duplicates variable names in count when trying to filter on interfaces because every concrete entities would share the same countVariable name and therefore override themselves which result in the where to fail for all except one concrete entity

Complexity

Note

Please provide an estimated complexity of this PR of either Low, Medium or High

Complexity: Low

Issue

Note

Please link to the GitHub issue(s) in which the proposal for this work was discussed

To link to multiple issues, use full syntax for each, for example Closes #1, closes #2, closes #3

Closes #5887

Checklist

The following requirements should have been met (depending on the changes in the branch):

  • Documentation has been updated
  • TCK tests have been updated
  • Integration tests have been updated
  • Example applications have been updated
  • New files have copyright header
  • CLA (https://neo4j.com/developer/cla/) has been signed

Copy link

changeset-bot bot commented Dec 14, 2024

🦋 Changeset detected

Latest commit: f7358d0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@neo4j/graphql Patch
@neo4j/graphql-ogm Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@angrykoala
Copy link
Member

angrykoala commented Dec 16, 2024

Hi @Masadow Thanks for the contribution!

I'm quite happy with this fix, as the comment says, this is a leftover to keep the old naming conventions in cypher, but definitely not something we need to keep around if it is causing name collisions

However, a few things needed (mainly regarding testing in this PR):

  1. The tck tests (checking the cypher output) have not been updated in this PR. This can be done with yarn test:tck -u and commit the automatic changes into this PR. I've tested that this PR updates the tck tests correctly so it shouldn't cause a problem
  2. Ideally, we should have an integration test validating this fix. (something similar to packages/graphql/tests/integration/issues/5698.int.test.ts) but replicating the issue you describe
  3. Running the integration tests yarn test:int shows that this PR breaks the test issues/5023.int.test.ts. This is actually caused by a separate problem, that was hidden by this variable rename. This new error can be patched as follows:
  4. Update the file create-update-and-params.ts. The method createAuthorizationBeforeAndParams should receive an extra parameter indexPrefix: "update"
  5. Update the file create-delete-and-params.ts. The method createAuthorizationBeforeAndParams should receive an extra parameter indexPrefix: "delete"

@Masadow Let me know if you are happy doing these changes or if you prefer for me or other maintainer to make those changes to your PR

Thanks

@angrykoala
Copy link
Member

Note to maintainers: After merging this PR, the fix should be ported over to dev (v.6)

@Masadow
Copy link
Author

Masadow commented Dec 18, 2024

@angrykoala Thanks for the detailed explanation, should be all good by now, let me know if anything needs to be updated. Hope the integration test is well written as well 😅

@angrykoala angrykoala requested a review from mjfwebb December 18, 2024 10:38
@neo4j-team-graphql
Copy link
Collaborator

neo4j-team-graphql commented Dec 19, 2024

Performance Report

No Performance Changes

Show Full Table
name dbHits old dbHits time (ms) old time (ms) maxRows
aggregations.TopLevelAggregate 3404 3404 44 54 1134
aggregations.TopLevelAggregateWithMultipleFields 6802 6802 52 82 1134
aggregations.NestedAggregation 15407 15407 56 75 2174
aggregations.AggregationWithWhere 10833 10833 49 59 2174
aggregations.AggregationWhereWithinNestedRelationships 20097917 20097917 2035 2254 2008534
aggregations.AggregationWhereWithinNestedConnections 20097917 20097917 2089 2120 2008534
aggregations.NestedCountFromMovieToActors 8603 8603 32 39 2174
aggregations.NestedCountFromActorsToMovie 8791 8791 37 44 2174
aggregations.DeeplyNestedCount 10052335 10052335 2829 2636 2008534
aggregations.InterfacesAggregations 6242 6242 44 57 2080
aggregations.InterfacesAggregationsWithTwoFields 11444 11444 73 89 2080
batch-create.BatchCreate 4200 4200 127 151 600
batch-create.BatchCreateSmall 77 77 63 65 11
connect.createAndConnect 6433 6433 142 203 3003
connections.Connection 12951 12951 69 84 2174
connections.NestedConnection 37705 37705 135 148 4516
create.SimpleMutation 7 7 55 63 1
cypher-directive.TopLevelMutationDirective 1135 1135 30 28 1134
delete.SimpleDelete 19401 19401 655 779 1040
delete.NestedDeleteInUpdate 16844 16844 136 174 1179
2871.NestedRelationshipFilter 19632 19632 100 65 4395
2925.SingleRelationshipFilter 5245 5245 39 43 1040
2925.NestedSingleRelationshipFilter 17641 17641 81 101 2174
2925.SingleRelationshipRequiredFilter 5201 5201 49 41 1040
2925.NestedSingleRelationshipRequiredFilter 9361 9361 55 60 1040
query.SimpleQuery 3121 3121 22 23 1040
query.SimpleQueryWithRelationship 15031 15031 46 41 2174
query.QueryWhere 8564 8564 32 36 2154
query.SimpleQueryWithNestedWhere 8713 8713 46 52 2154
query.Nested 10084891 10084891 6290 6320 2008534
query.NestedWithFilter 10064992 10064992 6145 6379 2004000
query.OrFilterOnRelationships 36664 36418 149 166 1933
query.OrFilterOnRelationshipsAndNested 26653 26453 188 259 1933
query.QueryWithNestedIn 14126 14114 174 60 1842
query.NestedConnectionWhere 8703 8703 51 61 2174
query.DeeplyNestedConnectionWhere 8702 8702 69 97 2174
query.DeeplyNestedWithRelationshipFilters 17357 17357 121 166 1552
query.NestedWithRelationshipSingleFilters 3808 3808 140 179 1134
query.Fulltext 64 64 32 38 16
query.FulltextWithNestedQuery 516 516 43 48 84
sorting-and-cypher.TopLevelSortWithCypher 12961 12961 41 47 2174
sorting-and-cypher.TopLevelConnectionSortWithCypher 12961 12961 64 78 2174
sorting-and-cypher.TopLevelSortWithCypherWithNested 13096 13096 57 61 2174
sorting-and-cypher.TopLevelConnectionSortWithCypherWithNested 13096 13096 93 102 2174
sorting-and-cypher.TopLevelSortWithExpensiveCypher 13725 13725 97 121 2174
sorting-and-cypher.TopLevelConnectionSortWithExpensiveCypher 13266 13266 101 144 2174
sorting.SortMultipleTypes 3436 3436 86 99 1040
sorting.SortMultipleTypesWithCypherWithCypher 13321 13321 124 113 2174
sorting.SortOnNestedFields 12951 12951 50 53 2174
sorting.SortDeeplyNestedFields 39785 39785 78 97 4516
sorting.ConnectionWithSort 3271 3271 71 90 1040
unions.SimpleUnionQuery 321 321 58 74 35
unions.SimpleUnionQueryWithMissingFields 293 293 54 58 35
unions.NestedUnion 309975 309975 262 289 33033
unions.NestedUnionWithMissingFields 283949 283949 255 283 33033
update.NestedUpdate 14137 14137 96 112 2002

Old Schema Generation: 27.398s
Schema Generation: 27.573s
Old Subgraph Schema Generation: 28.956s
Subgraph Schema Generation: 27.982s

@mjfwebb mjfwebb merged commit 8b6d256 into neo4j:lts Dec 19, 2024
69 checks passed
mjfwebb added a commit to mjfwebb/graphql that referenced this pull request Dec 20, 2024
mjfwebb added a commit that referenced this pull request Dec 20, 2024
…rfaces

Cherry pick: Fix relationship filter on interfaces #5890
@darrellwarde darrellwarde linked an issue Dec 20, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter on interfaces is not working
4 participants