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

Change tracking for value types (was: Make DataMapper class public?) #60

Closed
swiftlyfalling opened this issue May 24, 2016 · 12 comments
Closed

Comments

@swiftlyfalling
Copy link
Collaborator

swiftlyfalling commented May 24, 2016

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.)

@groue
Copy link
Owner

groue commented May 24, 2016

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?

@swiftlyfalling
Copy link
Collaborator Author

swiftlyfalling commented May 24, 2016

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.

@groue
Copy link
Owner

groue commented May 24, 2016

OK @swiftlyfalling

Here is an answer:

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).

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 referenceRow of the Record class.

We have thus a pretty strong limitation here that comes from the Swift language itself.

But let's continue:

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.)

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.

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.

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?

@swiftlyfalling
Copy link
Collaborator Author

swiftlyfalling commented May 25, 2016

@groue,

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.

Makes sense. I acknowledge that this is a 20% use-case.

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?

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:

enum PrimaryKey
extension Database { func primaryKey(tableName: String) throws -> PrimaryKey }
which then uses Database's schemaCache, which DatabasePool (which we're using) creates but doesn't expose (nor does Database)
databaseValues(forColumns: _, inDictionary _)
class InsertQuery
removingElementsOf(...)
class UpdateQuery
class DeleteQuery
class ExistsQuery

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.)

@groue
Copy link
Owner

groue commented May 25, 2016

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:

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 database.primaryKey(tableName) and database.cachedUpdateStatement(sql) public. Those are APIs I can commit to maintain in the future.

It's absolutely doable, and we have a working solution, but just wanted to make sure you were aware.

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 update and save methods. But they would need to mutate the reference values used for change tracking so that hasPersistentChangedValues is false after a save.

Now since I'm not sure your protocol would have much advantage inheriting from MutablePersistable, I'll wait for your feedback.

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.)

Ha ha, thanks :-) Tell the world!

@groue groue changed the title Make DataMapper class public? Change tracking for value types (was: Make DataMapper class public?) May 25, 2016
groue added a commit that referenced this issue May 25, 2016
@groue
Copy link
Owner

groue commented May 25, 2016

@swiftlyfalling: you can use the Issue60 branch as support for the ideas explored above: it has public primary keys and cached statements.

@swiftlyfalling
Copy link
Collaborator Author

@groue: The Issue60 branch makes a huge improvement in how easy this is to implement. 👍It's working great thus far.

@groue
Copy link
Owner

groue commented May 28, 2016

@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.

groue added a commit that referenced this issue May 28, 2016
groue added a commit that referenced this issue May 28, 2016
@groue
Copy link
Owner

groue commented May 28, 2016

@swiftlyfalling: GRDB v0.68.0 is out. Relevant documentation:

Should we keep this issue open?

@groue
Copy link
Owner

groue commented May 30, 2016

@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.

@groue
Copy link
Owner

groue commented Jun 5, 2016

@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.

@groue groue closed this as completed Jun 5, 2016
@swiftlyfalling
Copy link
Collaborator Author

Yup - things seem to be working well. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants