-
-
Notifications
You must be signed in to change notification settings - Fork 731
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
Change tracking for value types (was: Make DataMapper class public?) #60
Comments
Hello @swiftlyfalling DataMapper has not been designed for public release. Making things public requires auditing of this class's features and APIs, exploring use cases, writing documentation, and long-term maintenance. That's a lot of work :-) So please help me rephrase your use case. Your use case is not "I want to use DataMapper", your use case is something that DataMapper happens to help with, and I need to know what it is. Reading your message, It looks that you'd like structs to get the hasPersistentChangedValues flag. This flag is indeed only provided by the Record class today. Is that it? Something else? |
Hi @groue, Apologies - I phrased my initial query inarticulately. Here's some more detail: For unrelated (non-GRDB) reasons, we're sticking with protocols, value types, and composition. As such, we can't inherit from the Record class, but we're looking to replicate some of its functionality (including hasPersistentChangedValues and persistentChangedValues). In addition, we need to be able to split the Persistance/GRDB-protocol-conforming code for a struct from the property/data side. (i.e. Assume that we have a consumer layer that requests structs from a separate provider layer. The provider layer implements persisting/retrieving these values using GRDB, but the consumer layer doesn't know anything about this and just needs to be able to request the structs from the provider layer, access/modify their values, and send modified structs back to the provider layer for persistance. The consumer layer shouldn't include the GRDB.Framework.) We have a solution for this, (with the consumer including basic struct info, and the provider including extensions for those structs that conform to the RowConvertible, TableMapping, etc) but perhaps you have some ideas about how to better achieve it. |
Here is an answer:
All right. A little background information: GRDB has no protocol for change tracking, because change tracking requires storage (the reference values), and protocols can't provide both storage and data-hiding. A protocol property is as public as the protocol it belongs to, and a public protocol for change tracking would also expose its inner guts, the implementation details that should remain hidden. In our particular case, the implementation detail that I want to remain hidden is the private We have thus a pretty strong limitation here that comes from the Swift language itself. But let's continue:
OK. You implement the classical separation of concerns between the model and its persistence. Nothing against that, even if GRDB, as you have seen, tends to merge the two layers because this covers 80% of application needs.
The way I see it is the following: providing a full-blown solution to your use case is not on my feature list. But I don't want to prevent you from implementing it. So instead of exposing GRDB's DataMapper, let's see why you can't just copy/paste it into your own module, and build your own protocol for change-tracking-enabled around it. Did you explore this solution already? |
Makes sense. I acknowledge that this is a 20% use-case.
There are several parts of DataMapper that require copying more code out from the framework (or slight re-working) because they aren't publicly available:
It's absolutely doable, and we have a working solution, but just wanted to make sure you were aware. As always, many thanks for a fantastic library that is a pleasure to use, and for your thorough responses. (We've been stress-testing the SQLite Concurrency support and DatabasePools and have nothing but the highest praise. 👍 GRDB is considerably better than how some other frameworks try to handle concurrency.) |
Most of them are helpers for DataMapper and can be copied as well. Yet I'm concerned about primary keys and prepared statements: the building blocks for robust caching (which means cache invalidation when the database schema changes) are not exposed. And caching helps performance a lot here. I think I could help you by making
I can imagine that it is somewhat frustrating to reimplement something that is mostly ready-made. But let's remember the rule of three: your use case is specific enough for you to prefer maintaining your own fork of the persistence layer. We'll maybe merge our common concerns later, but only after your have a satisfying working solution. I don't know if you have already explored a protocol that grants change tracking. You won't be able to make it inherit from the existing MutablePersistable protocol, because MutablePersistable has non-mutating Now since I'm not sure your protocol would have much advantage inheriting from MutablePersistable, I'll wait for your feedback.
Ha ha, thanks :-) Tell the world! |
@swiftlyfalling: you can use the Issue60 branch as support for the ideas explored above: it has public primary keys and cached statements. |
@groue: The Issue60 branch makes a huge improvement in how easy this is to implement. 👍It's working great thus far. |
@swiftlyfalling. OK, so I'll merge Issue60 into master, write a couple of documentation lines, and ship a new version. In this story, consider me satisfied if you don't have to modify GRDB in order to build your custom persistence engine on top of it. |
@swiftlyfalling: GRDB v0.68.0 is out. Relevant documentation: Should we keep this issue open? |
@swiftlyfalling: The cachedUpdateStatement() method had a bug that would make it return an invalid statement after a failure (for example a relational constraint violation). This bug is fixed in v0.70.1: be sure to update or you'll get unwanted crashes. |
@swiftlyfalling: your recent PRs aimed at providing a solution to your custom persistency engine, didn't they? This issue is no longer relevant, and I'm very happy of what we have achieved together. |
Yup - things seem to be working well. 👍 |
We have a use-case where it would be really handy to use the DataMapper class. Is there a reason it isn't public?
(We have some heavily-customized persistence and change-tracking behavior, and also want to use value-types/structs for our records instead of classes - so no inheriting from Record. It's all working, but we had to tweak the source to make DataMapper public.)
The text was updated successfully, but these errors were encountered: