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

Support generic operator for relationship filters #5873

Open
wants to merge 9 commits into
base: generic-scalar-filters
Choose a base branch
from

Conversation

MacondoExpress
Copy link
Contributor

@MacondoExpress MacondoExpress commented Dec 6, 2024

This PR adds the implementation for the generic filtering for relationships such as actors: { some: { name: { eq: "Keanu Revees" } } } }, it also removes from the 7.x branch the usage of isNot.

Copy link

changeset-bot bot commented Dec 6, 2024

🦋 Changeset detected

Latest commit: f91dd3e

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

This PR includes changesets to release 1 package
Name Type
@neo4j/graphql Minor

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 angrykoala changed the title Suppor generic operator for relationship filters Support generic operator for relationship filters Dec 9, 2024
}

return fields;
}
// Relationship filters
Copy link
Member

Choose a reason for hiding this comment

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

Maybe explicitly say non-connection relationship or simple relationship


// NOTE: This used to be a specialized function used specifically to generate relationship fields,
// but now after this refactor, it could be used as schema composer utility if needed.
function fieldConfigsToFieldConfigMap({
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, maybe move the utility function under the exported function?

function getRelationshipConnectionFiltersLegacy(
relationshipAdapter: RelationshipAdapter | RelationshipDeclarationAdapter
): FieldConfig[] {
return [
Copy link
Member

Choose a reason for hiding this comment

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

These should be deprecated right? Is this a good place to add that deprecation maybe?

operator,
relationship,
});
}

// TODO: we keep call this Generic filters but maybe we should rename it to something more meaningful
// Proposal: TypeSpecificFilters
Copy link
Member

Choose a reason for hiding this comment

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

🤔

parseGenericTypeFilters?

I'm not sure what TypeSpecific means

if (objectEntries.length !== 1) {
throw new Error("Expected one quantifier in a relationship predicate");
}
const [genericOperator, genericValue] = objectEntries[0] as [string, any];
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure genericOperator is a very good naming convention

We've fallen to that convention for the schema generation types, but here is a bit weird tbh. We are parsing an operator after all. Same with value

query($movieIds: [ID!]!) {
${Movie.plural}(where: { AND: [{ id: { in: $movieIds }}, { actors_${predicate}: { NOT: { flag: {equals: false }} } }] }) {
${Movie.plural}(where: { AND: [{ id: { in: $movieIds }}, { actors: { ${predicate}: { NOT: { flag: {equals: false }}} } }] }) {
Copy link
Member

Choose a reason for hiding this comment

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

This is going to cause conflicts all over the place

Suggested change
${Movie.plural}(where: { AND: [{ id: { in: $movieIds }}, { actors: { ${predicate}: { NOT: { flag: {equals: false }}} } }] }) {
${Movie.plural}(where: { AND: [{ id: { in: $movieIds }}, { actors: { ${predicate}: { NOT: { flag: {eq: false }}} } }] }) {

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.

2 participants