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

Remove surplus _IN and _NOT_IN filters for relationships #400

Merged
merged 2 commits into from
Aug 10, 2021

Conversation

darrellwarde
Copy link
Contributor

@darrellwarde darrellwarde commented Aug 9, 2021

Description

Let's take the opportunity of this breaking release to remove some surplus requirements, _IN and _NOT_IN for one-to-many and one-to-one relationships. We don't currently treat these relationship cardinalities differently anywhere else, the filters are completely untested, and the same filters can be achieved through other means (see migration guide for example).

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
Contributor

@danstarns danstarns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to remove them, Let's remove them for good. You have forgotten about the code behind these here and here. I'm not sure I completely understand the reasoning for removing them and if they work just fine why are we getting rid?

@darrellwarde
Copy link
Contributor Author

darrellwarde commented Aug 9, 2021

If we are going to remove them, Let's remove them for good. You have forgotten about the code behind these here and here. I'm not sure I completely understand the reasoning for removing them and if they work just fine why are we getting rid?

Honestly, I don't think the Cypher for them is even correct (wrong predicate), and we really shouldn't have any untested paths in our code. I was able to remove these two filters and only 1 (!) TCK test broke - I don't think that's really the level of quality we should be aiming for.

It's also super inconsistent - if we have these, then we should also have _INCLUDES and _NOT_INCLUDES for the "many" side of a relationship.

Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@darrellwarde darrellwarde merged commit 0c0f6a0 into neo4j:2.0.0 Aug 10, 2021
@darrellwarde darrellwarde deleted the bugfix/remove-extra-in-filters branch May 9, 2022 16:29
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 this pull request may close these issues.

3 participants