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

Cannot use with entities not SafeToInsert #70

Closed
pbrisbin opened this issue Jul 25, 2023 · 2 comments
Closed

Cannot use with entities not SafeToInsert #70

pbrisbin opened this issue Jul 25, 2023 · 2 comments

Comments

@pbrisbin
Copy link
Member

pbrisbin commented Jul 25, 2023

As of lts-21, the version of persistent present needs SafeToInsert to use insert. This serves as witness that the entity in fact uses an auto-increment key and so is "safe to insert [without explicit key]". Entities where this is not true must use insertKey and supply the key. insertKey, of course, does not enforce SafeToInsert. So safe.

In Graphula, we have only the single insert member in the MonadGraphulaFrontend typeclass. And it always requires (our compatibility type for) SafeToInsert. This is because it takes a Maybe Key argument and uses Persist.insert on Nothing (or Persist.insertKey on Just).

Up until lts-21, this constraint was a no-op and we in fact created cases where we use Graphula on entities that aren't safe to insert. Since we always supplied a Just key to our insert for these cases (based on KeySource), it all worked. But once that constraint becomes actually-enforced, they no longer type check.

At this point, in order to depend on newer persistent and keep up with the latest LTS, we think we need to expand the type-class to include insertKey, which will be used by nodeKeyed (not node), and then we can migrate these non-compiling use-cases.

This calls into question the overall design of insert, with its Maybe argument, node and KeySource, etc -- as we seem to be diverging with how persistent is tackling a similar design space. If it turns out we can't avoid a major version bump, we might consider taking larger liberties here.

However, if we can do this as a minor version bump that would be preferred. It could be possible if we give insertKey some default. But we have to do that in a way that doesn't keep the SafeToInsert constraint on nodeKeyed when used. (For example, insertKey = insert . Just won't work.) 🤔

/cc @tomcarste

@github-project-automation github-project-automation bot moved this to 👜 To do in Open Source Jul 25, 2023
@pbrisbin
Copy link
Member Author

And note that this is not a problem compiling graphula itself, it only breaks with a particular use-case. We might want to add a test case for the now-broken way we use KeySource on none-safe-to-insert entities.

@chris-martin
Copy link
Contributor

Done in #73

@github-project-automation github-project-automation bot moved this from 👜 To do to ✅ Done in Open Source Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

No branches or pull requests

2 participants