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

Add back _on filters or provide a way to sort unions #4825

Closed
michaeldaudiolo opened this issue Mar 5, 2024 · 11 comments
Closed

Add back _on filters or provide a way to sort unions #4825

michaeldaudiolo opened this issue Mar 5, 2024 · 11 comments
Labels
enhancement New feature or request

Comments

@michaeldaudiolo
Copy link

We've been using _on filters with interfaces to filter on specific fields of implementing types, while also being able to sort based on common fields across the implementing types. With version 5.0.0 that's no longer possible, since unions can't be sorted.

I don't understand why _on filters were removed but I'm sure there's a good reason. Could support for sorting with unions be added instead? If so, could _on be deprecated instead of outright removed until that's done?

@michaeldaudiolo michaeldaudiolo added the enhancement New feature or request label Mar 5, 2024
@darrellwarde
Copy link
Contributor

Hi @michaeldaudiolo! Thanks for this issue. We may have been overzealous in our removal of _on for unions as well as interfaces, so we will do some design work over the next couple of weeks to see if we can address for you! It was primarily removed because with interfaces, it was far too easy to craft queries which made no sense or would produce unpredictable results. We can't add _on back in as deprecated because it would be another breaking change to remove it, so I would recommend staying on version 4.x until we've finished looking into this. Sorry for any inconvenience!

@michaeldaudiolo
Copy link
Author

Hi @darrellwarde, thanks for the reply! Since you don't plan to just add _on back, implementing some kind of post union sorting on any field on union types would actually be even better for our use case, if possible. Right now since we have to rely on interfaces for the ability to sort multiple types together, we're limited to fields that are implemented in every type.

@darrellwarde
Copy link
Contributor

Hi @darrellwarde, thanks for the reply! Since you don't plan to just add _on back, implementing some kind of post union sorting on any field on union types would actually be even better for our use case, if possible. Right now since we have to rely on interfaces for the ability to sort multiple types together, we're limited to fields that are implemented in every type.

I would love to hear more about this use case. Would it be possible for you to give an example of a union which you might sort (especially if they have no common fields), and give an idea of what you would expect the sorting result to be? Struggling to think of a situation where the result would be a mixture of all of the different types in the union!

@michaeldaudiolo
Copy link
Author

michaeldaudiolo commented Mar 6, 2024

Admittedly It's been hard for me to think of a simple example too. But you could have something like this:

union Shipment = Box | Envelope | Pouch

type DistroCenter {
  location: String!
  shipments: [Shipment!]! @relationship(type: "HAS_SHIPMENT", direction: OUT)
}

type Box {
  tracking: String!
  weight: Float!
  // some other stuff unique to Boxes
}

type Envelope {
  weight: Float!
}

type Pouch {
  tracking: String!
  weight: Float!
  // some other stuff unique to Pouches
}

Every type has a weight, but only Box and Pouch have tracking.

It could be nice to be able to get shipments for a center sorted by weight and then by tracking for example. This assumes Pouch and Box are different enough that combining them into one type wouldn't make sense and envelopes are never tracked though.

We have a similar situation in our schema with three different implementations of the same interface where the separation between them makes sense, and there's varying degrees of overlap between them.

Maybe I only want to see boxes and pouches with specific tracking numbers along with all envelopes, that's where we would use _on currently and sort by weight on the interface if desired.

@darrellwarde
Copy link
Contributor

Hi @michaeldaudiolo, thanks for providing that example.

Honestly, it really sounds like Shipment should be an interface and not a union, and then you would be able to sort by the shared field weight across all of the different shipment types.

tracking is more tricky, because you won't be able to sort by that. However, you could do something like make it nullable and use the @settable directive to make it read-only, and then you could sort by it!

Just some ideas.

We've discussed the idea of adding sorting back in for union types, but have come to the conclusion that it doesn't really make sense. You can't accurately paginate over unions, and adding sorting would make this even less predictable.

We won't be adding _on back for interfaces because this removal unlocks a lot of opportunities for more efficient and performant Cypher being generated by the library.

We appreciate the feedback, and hopefully a workaround with interfaces works for you!

@darrellwarde darrellwarde closed this as not planned Won't fix, can't repro, duplicate, stale Mar 11, 2024
@michaeldaudiolo
Copy link
Author

michaeldaudiolo commented Mar 11, 2024

We may have been overzealous in our removal of _on for unions as well as interfaces, so we will do some design work over the next couple of weeks to see if we can address for you!

Just to clarify, are you saying the design work in this previous comment isn't planned anymore?

If that's the case, essentially if I want sorting I have to use an interface and if I want accurate filtering across types I have to use a union going forward?

You can't accurately paginate over unions, and adding sorting would make this even less predictable.

I'm sorry this doesn't really make sense to me. Isn't sorting usually the solution to unpredictable pagination? Especially if the sorting is done after the CALL subquery executing the union. Am I missing something?

@michaeldaudiolo
Copy link
Author

If that's the case, essentially if I want sorting I have to use an interface and if I want accurate filtering across types I have to use a union going forward?

To clarify this, if I want to do what I mentioned here:

Maybe I only want to see boxes and pouches with specific tracking numbers along with all envelopes, that's where we would use _on currently

With your suggestion here:

However, you could do something like make it nullable and use the @settable directive to make it read-only, and then you could sort by it!

It would be something like:

query DistroCenters($where: ShipmentWhere) {
  distroCenters {
    shipments(where: $where) {
      tracking
      weight
      __typename
    }
  }
}
{
  "where": {
    "OR": [
      {
        "tracking_IN": ["1", "2"]
      },
      {
        "tracking": null
      }
    ]
  }
}

Which is not the same as

{
  "where": {
    "_on": {
      "Box": {
        "tracking_IN": ["1", "2"]
      },
      "Pouch": {
        "tracking_IN": ["1", "2"]
      },
      "Envelope": {
        "tracking": null
      }
    }
  }
}

Because going forward it'll return Boxes and Pouches that are null too, instead of only Boxes and Pouches that match tracking numbers 1 or 2.

So if I want that kind of filtering I have to use unions, but if I want any kind of sorting I have to use interfaces?

@MacondoExpress
Copy link
Contributor

Sorry for the late reply, when we removed the _on filter we also added the typename_IN filter, this means that you can achieve the above by doing this:

query DistroCenters {
  distroCenters {
    shipments(
      where: { AND: [{ tracking_IN: ["1", "2"] }, { typename_IN: [Box, Pouch] }] }
    ) {
      tracking
      weight
      __typename
    }
  }
}

@michaeldaudiolo
Copy link
Author

Hi @MacondoExpress, thanks for your reply!

Unfortunately that still wouldn't satisfy the "along with all envelopes" part of the condition.

Maybe I only want to see boxes and pouches with specific tracking numbers along with all envelopes

Basically, before v5 we have been using interfaces and relied on _on very heavily since _on let's us do complex filtering and interfaces allow sorting.

It seems like after v5 you have to either choose complex filtering or sorting but you can never do both, since interfaces don't support _on anymore for complex filtering but can still be sorted and unions can't be sorted but they do still allow complex filtering. Honestly, that feels pretty bad.

@MacondoExpress
Copy link
Contributor

Hi @michaeldaudiolo!
Sorry, I didn't catch that requirement, does this query solve your case?

query DistroCenters {
  distroCenters {
    shipments(
      where: {
        OR: [
          { typename_IN: [Envelope] }
          { AND: [{ tracking_IN: ["1", "2"] }, { typename_IN: [Box, Pouch] }] }
        ]
      }
    ) {
      tracking
      weight
      __typename
    }
  }
}

@michaeldaudiolo
Copy link
Author

Yes that one would work for that case, thank you!

It does still feel a little bad to have to add any fields we might want to filter or sort on to the interface and just make them null for types that really shouldn't have those fields. It feels like a work around that muddies the schema, not a clean solution. I agree with the issues raised about that in #5213.

I think this library is amazing and I genuinely really appreciate the work you all put into it regardless though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants