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

Identities with optional results #45

Closed
ghost opened this issue May 26, 2016 · 5 comments
Closed

Identities with optional results #45

ghost opened this issue May 26, 2016 · 5 comments
Assignees
Labels
help wanted Extra attention is needed
Projects

Comments

@ghost
Copy link

ghost commented May 26, 2016

The current semantics of Fetch are that identities must have a result. If a result is missing, the fetch execution short-circuits and fails. Maybe we should support the notion of optional fetches, much like Clump does with the .optional method.

@ghost ghost added the help wanted Extra attention is needed label May 26, 2016
@anamariamv anamariamv added this to Backlog in Fetch Mar 21, 2017
@gregbeech
Copy link

The current behaviour is actually pretty surprising, that a method with this signature:

def fetchOne(id: I): Query[Option[A]]

Causes the whole execution to fail. It's not clear why the result is an Option if None isn't a valid result.

@peterneyens
Copy link
Contributor

@gregbeech I agree.

I was planning to rework Query a bit anyway after the next fetch release supporting Cats 1.0.0-MF is out in one of the next few days. I'll see if we can support optional results as well.

@prurph
Copy link

prurph commented Nov 21, 2017

👍

In particular this would make fetch an even better fit for GraphQL APIs, where the default is to make everything optional, so parts of a query can fail, while others succeed. For example, in the GitHub API if you search for foo and bar and only foo succeeds, you'll see the foo data and and an error at the bar node. This is very challenging to implement with fetch because fetchMany will immediately yield a MissingIdentifiers so you can only see which fetches failed/didn't return results.

@paulpdaniels
Copy link
Contributor

@peterneyens @raulraja Any movement on this? Cats has since stabilized. I'm curious what design changes you had in mind to support this feature.

@purrgrammer
Copy link
Contributor

purrgrammer commented Aug 28, 2018

Causes the whole execution to fail. It's not clear why the result is an Option if None isn't a valid result.

The None signals that the identity hasn't been found, the current assumption of the library is that every identity exists, that's why Fetch(identity) returns a Fetch[A]. As the interest in this issue suggests we should support optional identities.

For individual identities you could create an optional fetch doing Fetch.optional(identity) which returns a Fetch[Option[A]], in case of optional identities when fetching a collection I'm not sure how to approach it, one option would be to add a version of Fetch.multiple that marks fetched identities as optional.

@peterneyens @raulraja Any movement on this? Cats has since stabilized. I'm curious what design changes you had in mind to support this feature.

Hey @paulpdaniels, I'm working on updating Fetch to work with the latest cats version and to rely on cats-effect for most of its internals, meaning that we will move away from the implementation using a free monad and state-based interpreter to interpreting Fetch to a user-selected Effect, which will likely be cats.effect.IO most of the time, but since Monix or Scalaz Task can implement Effect it should be easy to use it with your preferred effect type.

This means that Query will no longer be there, but fetchOne and fetchMany will look something like:

trait DataSource[I, A] {
  def fetchOne[F[_]: Effect](id: I): F[Option[A]]
  def fetchMany[F[_]: Effect](ids: NonEmptyList[I]): F[Map[I, A]]
}

I've outlined a couple of design changes on the response above but I need to get into implementing optional identities to have a better idea of what would be required to support them.

What I have in mind is to enrich FetchQuery to have knowledge about the optional identities, this way a FetchOne could be marked as optional with Fetch.optional and if batched into a FetchMany the optional identities would be remembered for not resulting in a MissingIdentities error if an optional identity wasn't found.

Hope it helps, I'll open a PR shortly for the redesign using cats-effect and after that this will be my priority. Thanks for your comments and time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
Fetch
Backlog
Development

No branches or pull requests

5 participants