Allow updating of primary keys

taltstidl commented 8 years ago

Goal

There should be a method for updating the primary key of an object after it has been created. Previously one could update it using e.g. object.setId(newId), but starting with Realm 2.0.0 this throws an exception to keep it in line with the Realm API in other languages. See the dicussion in Pull Request #3418. The workflow of my app is similiar to the following:

User creates a new object in the database while offline
This object gets a temporary (negative) id, as the app does not yet know what final id the server will assign to the object
User/app syncs the database when online again
The server receives the new object and assigns a final id to it
The app receives the final id from the server and updates the objects id to the final one (this is currently not directly supported by Realm)
Expected Results

I personally see 2 options going forward with this:

Reallow changing primary key fields: this would basically put it in the same state as previously with the following code to update an objects primary key:
```
TestObject object = realm.where(TestObject.class).equalTo("mId", oldId).findFirst();
object.setId(newId);
```
Add dedicated method for changing primary keys: this probably makes more sense if you want to align it more with the Realm API in other languages:
```
realm.updatePrimaryKey(TestObject.class, oldId, newId);
```
Actual Results

Currently only a rather ugly and not very performant workaround does the job, basically bypassing the check by making an in-memory copy of the object:

private static <E extends RealmObject> void updatePrimaryKey(Realm realm, Class<E> type, long oldId, long newId) {
    E object = realm.where(type).equalTo("mId", oldId).findFirst();
    E copy = realm.copyFromRealm(object);
    copy.setId(newId);
    realm.insertOrUpdate(copy);
    object.deleteFromRealm();
}

I'm really looking forward to seeing this fixed in a future update (though I absolutely acknowledge that you have lots of other issues to attend to 😉)

Version of Realm and tooling

Realm version(s): >2.0.0 Android Studio version: all Which Android version and device: all

References

realm-cocoa: https://github.com/realm/realm-cocoa/issues/4194

Zhuinden commented 8 years ago

Well, I do think this restriction is somewhat odd when you're not actually using sync and the Realm Mobile Platform.

taltstidl commented 8 years ago

@Zhuinden Are you referring to the Realm Mobile Platform sync? I'm not using that, and for a couple of reasons won't be able to do so in the near future. So I'll always need to maintain my own sync code and there's no reasonable way around the temporary ids, unfortunately (don't know how the Realm Mobile Platform actually solves this).

Zhuinden commented 8 years ago

@TR4Android I mean that this restriction is done to support the Realm Object Server's sync.

So if you don't have sync, it's weird that we are limited by it.

taltstidl commented 8 years ago

@Zhuinden Yes, I agree. It's a strange limitation to have, especially when writing your own sync code. Though I understand their current focus on the Realm Mobile Platform, it seems kind of odd to me to introduce regressions to the Realm Mobile Database in order to support it.

kneth commented 8 years ago

We decided to add this restriction in 2.0.0 of two reasons. First, the Objective-C/Swift product already had the restriction, and it is important to agree on the behaviour of primary keys. Second, we have to change the behaviour for Realm Mobile Platform. Changing the value of a primary key on one device may lead to inconsistencies on another device. And we don't wish to avoid the behaviour in one set up, and disallow it in another. If you have an standalone app, moving to Realm Mobile Platform must be as easy as possible.

Besides that, it is not recommended to change the value of primary keys in relational databases (see for example http://stackoverflow.com/questions/2499246/how-to-update-primary-key) since values foreign keys also have to be updated. One could argue that allowing changing the value of a primary key prior to 2.0.0 was a mistake at our side.

It is much better to add a new object and then remove the old one.

taltstidl commented 8 years ago

@kneth I'm well aware of the reasons for adding the restrictions, but still I think there should be an easier way for updating the primary keys that does not need copying and deleting. Let me respond to a few of your words inline:

First, the Objective-C/Swift product already had the restriction, and it is important to agree on the behaviour of primary keys

I agree, that's why I've opened a similiar issue in the realm-cocoa repo (issue https://github.com/realm/realm-cocoa/issues/4194).

Second, we have to change the behaviour for Realm Mobile Platform.

I understand the reasoning behind it, but this shouldn't affect users of the library not using the Realm Mobile Platform, in my opinion. Writing custom sync code is that much harder without having a way of updating primary keys.

If you have an standalone app, moving to Realm Mobile Platform must be as easy as possible.

Unfortunately the Realm Mobile Platform will never be a viable option for this app, for a couple of reasons:

We already have an existing MySQL database on our server (though with some effort it's probably possible to migrate all the data to a Realm database)
The server database is quite large and it's not possible to store the complete database on our clients (Android and iOS). Instead we only sync a partial dataset (i.e. a few months worth of data).
We don't have the concept of user data. All entries are visible to all users, regardless of who entered it.
Clients can also sync across devices using Bluetooth. Chances are that this will break the automatic sync system of the Realm Mobile Platform.
We also have a website and as far as I know Realm doesn't support usage with the PHP language.
We need to be able to react to events (for merging duplicates and sending push notifications to all registered clients), but can't afford the Enterprise edition as we're an NGO with a free app.
Probably a couple of other reasons that would pop up during development.

As such we can not invest the resources needed to switch to the Realm Mobile Platform, as much as I like the prospect of never having to write a single line for sync anymore. And it's probably very unlikely that the Realm Mobile Platform will support such complex requirements in the near future.

Besides that, it is not recommended to change the value of primary keys in relational databases.

That's correct, but it's unavoidable in our case as we need stable ids generated by our server. Besides, I though the whole point of Realm is that it's an object store, as such changing an id shouldn't affect the relations pointing to that object 😉!

It is much better to add a new object and then remove the old one.

Better in regards to what? In terms of API required it is definitely better that way. In terms of usability and performance not that much. As such I'd really welcome a updatePrimaryKey() method 👍. It forces users to reflect on changing their primary key and should probably throw an exception if used in conjunction with the Realm Mobile Platform, but it'd make a developers life that much easier if it's absolutely required (as in my case).

Again and again, thanks for all the hard work and the time you invest in listening to the users of your library!

stk1m1 commented 8 years ago

@TR4Android

Realm is that it's an object store, as such changing an id shouldn't affect the relations pointing to that object 😉!

Let's forget about PrimaryKey thingy, and say we have two objects with exactly the same values except different ids. Now, I make the two ids identical. Are they then different objects?

Believe it or not, the very idea of database is built on top of Set Theory. It could be defined that If a set is any collection of definite distinguishable things, we can conceive of the set as a whole¹. When a database is to be considered as an universe, we should be able to tell if an entity, an object - in the case of Realm, of the database is distinguishable.

In order to meet an expected performance for distinction in the presence of large number of entities, one should have an attribute with which no two are the same in the database. With the attribute, an entity or an object becomes one and one only in the database. If the attribute of two entities are the same, we tell they are identical. Whatever name the attribute bears, the nature of its role won't change and is commonly known as PrimaryKey.

Whether one recognizes the theoretical debate or not, it is clearly an indecent idea to update PrimaryKey, which increases chances of PK duplication that would lead to mangle the uniqueness of an entity. That's the primary motivation behind many suggestions against updating PK.

Other battle-hardened (or battle-shredded) databases also suggest not to; Oracle advises against updating PK, IBM DB2 considers update on PK as deletion of an entry, and MariaDB wants PKs to be as stable as possible.

They also recommend to design PK immutable from the get-go; should PKs need to change frequently, it is a good indication of schema design flaws.

(*You might argue that surrogate keys might do the job. Firstly, it most likely won't, and I don't believe that's feasible in a constraint environment like mobile devices. I don't know about you, but I cannot become gentle when a database hogs down my phone's resource just for searching keys.)

Below is my 2 cents and is my personal opinion only. I believe restricting update on PK is more of a progression than a regression as it fills up the previously existing gap and promotes a good practice rather than an easy-get-around.

Zhuinden commented 8 years ago

@TR4Android you could just replace your current @PrimaryKey with an @Index @Required field, no?

Although in that case, you would have to check if there's already a managed object by the same "indexed field", rather than just put insertOrUpdate() based on primary key value regardless of whether it is managed or not.

taltstidl commented 8 years ago

@stk1m1 I understand the purpose and value of primary keys, but still I have to update my primary key once (and only once). Call it bad design, but it's _required_ in my case. In the current library I have to do workarounds, which is ugly to say the least.

Let me reiterate this: I totally agree that primary keys normally shouldn't be updated, there's no doubt about that. But in my case there's no other way. Being offline, a client can't tell the next id that's usable in the server database. As such, it has to assign a temporary id to the object, of which it is sure that it won't clash with any server objects already synced with the client database (i.e. negative ids in my case). One could theoretically try to generate unique ids on the device, but there's currently no reliable way to do this. Generating temporary ids is such the best a client can do. In order to keep everything consistent though, the client has to update the primary key after the server has assigned the final id though. That is unavoidable and currently unsupported by Realm, which is why I created this issue. I hope I could shed some more light on the use case.

bmunkholm commented 8 years ago

@TR4Android Thanks a LOT for the detailed descriptions and also needs in terms of sync - thats really helpful for us to understand our current limitations! I just want to throw one argument into the mix here: In terms of API design we strive to avoid having methods that works in some situations and not in others - it's just leads to really bad API's.

As we all agree that this is a bad design to include it, I think it's worth exploring if an alternative solution could solve your needs. I obviously don't know how you make your keys, but if you use UUIDs or generate the id's with some part of it including a client unique ID, you could ensure it would be unique. Another alternative is maybe (not sure the team would accept this!) to make a PR with a constant that enabled this, and you could then compile it yourself.

I surely empathize with the annoyance since this already worked, and now we pulled the plug on it :-( But our motivation is just for the greater good of mankind :-)

taltstidl commented 8 years ago

@bmunkholm Yes, I agree that methods that work in some cases, but not in others are bad API design. Still, I see no way around this, at whatever angle I look at this.

Going down the path of unique device ids, a couple of issues pop up:

There's no way of generating unique ids across platforms (i.e. Web, Android and iOS) that I know of, which makes it entirely impossible to rely on unique ids.
Android unique ids are hard to get to in a reliable way (see the extensive discussion in this StackOverflow thread).
Queries are slower when using VARCHARs as primary keys, especially for more complex queries with lots of JOINs (on the server database).
I already have an existing framework that works with Integer primary keys (but this shouldn't really be an argument in this discussion, that's just additional work and time required on my side :wink:).

All in all I think temporary ids are still the best approch in my case, even though I generally try not to change my primary keys. So I'm still hoping that one day Realm will reallow this. Thanks for the consideration!

reinaldomoreira commented 7 years ago

I would be glad if this post have more attention :)

ntimesc commented 7 years ago

I am facing the same issue related to sync process that TR4Android is facing. Do we have any solution yet ?

nkanellopoulos commented 7 years ago

In the meantime, has setId been completely removed from the API? This makes even @TR4Android 's workaround a PITA to use... How can we now write a generic method to do this?

Note that our Java objects have to extend RealmObject DIRECTLY. So, our only choice is to use the RealmModel interface?

Zhuinden commented 7 years ago

The setId() was his own method.

Generic method to do what exactly? Interfaces work.

nkanellopoulos commented 7 years ago

@Zhuinden in Kotlin, I am not allowed to inherit from my own base class and, say, implement RealmModel. I get a compilation error (Even with cleaning and rebuilding the project).

:app:kaptGenerateStubsDebugKotlin
:app:kaptDebugKotlin
e: /Users/nk/Projects/android/FieldObservations/app/build/tmp/kapt3/stubs/debug/fieldobservations/model/Trap.java:5: error: Valid model classes must either extend RealmObject or implement RealmModel.
e: 

e: public class Trap extends fieldobservations.model.ModelObject implements io.realm.RealmModel {
e:        ^
e: java.lang.IllegalStateException: failed to analyze: org.jetbrains.kotlin.kapt3.diagnostic.KaptError: Error while annotation processing
    at org.jetbrains.kotlin.analyzer.AnalysisResult.throwIfError(AnalysisResult.kt:57)
    at org.jetbrains.kotlin.cli.jvm.compiler.KotlinToJVMBytecodeCompiler.compileModules(KotlinToJVMBytecodeCompiler.kt:138)

So, how can I write the following code only once ?

open class ModelObject : RealmModel {
    @PrimaryKey
    open var id : Long = -1

    open var createdAt = Date()
    open var updatedAt : Date? = null
    open var synced : Date? = null

    fun <E : RealmObject> updatePrimaryKey(realm: Realm, type: Class<E>, oldId: Long, newId: Long) {
        val realmObject = realm.where(type).equalTo("id", oldId).findFirst()
        val copy = realm.copyFromRealm(realmObject)
        id = newId
        realm.insertOrUpdate(copy)
        realmObject.deleteFromRealm()
    }
}

Zhuinden commented 7 years ago

@nkanellopoulos you cannot extend a class that isn't specifically RealmObject in Realm model classes see https://github.com/realm/realm-java/issues/761

But you can do extends RealmObject implements HasId<Long> or something like that.

nkanellopoulos commented 7 years ago

Granted @Zhuinden, I can do this (which is exactly what I was contemplating to do). But it feels like a hack to me, when I face too many restrictions that seem arbitrary.

HasId<Long> CreatedAt<Date> and the list goes on...

Zhuinden commented 7 years ago

I mean if there are multiple fields shared across pretty much all entities then you can put them into a single interface.

I do admit that inheriting accessors isn't ideal, but I don't have a better alternative at the moment

nkanellopoulos commented 7 years ago

The main problem is that I cannot have a base class for common stuff. It makes perfect sense to share the implementation, when it is exactly the same.

Why is there the restriction that my Realm objects should be direct subclasses of RealmObject ?

Zhuinden commented 7 years ago

@nkanellopoulos because then Core would need to support sharing fields across classes and queries that return the superclass of your concrete objects from all "tables". And it doesn't do that yet.

nkanellopoulos commented 7 years ago

@Zhuinden I am not requesting the sophisticated behaviour you describe. I probably do not even want it. I just want to be able to have a base class. It would be perfectly fine with me if "Core" would replicate the the fields of the base class in all "tables".

The base class does not even need to be a RealmObject. I think I have tried to extend a POJO and implement RealmModel, but this does not work either.

Zhuinden commented 7 years ago

@nkanellopoulos

I just want to be able to have a base class.

That means Realm needs to support the "sophisticated behavior" I described, and it doesn't yet, so it doesn't allow it until it does.

I admit, inheriting accessors is not that ideal, but that's one option, the other is to make your DAO classes have a shared getId()/setId method that is implemented for each class in each Dao, or you can also use reflection.

You'll still need to copy the fields across the RealmObjects to make them be part of the schema definition for each class, until polymorphism is supported anyways. Which will probably take a while, so I wouldn't sit idly till then.

taltstidl commented 6 years ago

Any update on this issue? This is still a huge pain point for me. I love Realm and it is making lots of progress, yet this limitation has continued to annoy me. Currently I just copy the fields and then delete the old object, but unfortunately this triggers the change listener with a deletion and not with the new updated fields. 😢

I would be glad if you could tell me what would I need to do to remove this limitation? What parts of the code would need to be changed. Would you be interested in a pull request (that e.g. ties this limitation to the syncEnabled variable)? Thanks in advance!

celesteshire commented 6 years ago

I am facing the same issue related to sync process that TR4Android is facing. Do we have any solution yet ?

taltstidl commented 6 years ago

@celesteshire Unfortunately no official solution, yet 😢. The current workarounds are:

Copy the object in-memory (copyFromRealm()), change the primary id there, insert the new object and then finally delete the old object. However, this makes actual updates appear as deletions in any attached change listeners. You can find a short, generic code snippet in the issue description.
Remove the @PrimaryKey annotation from your field and replace it with @Required @Index annotations. However, this requires you to do primary key validation yourself! Depending on your project size and the associated risk of accidentally overwriting an existing object, this might be also be a valid solution.

sipersso commented 6 years ago

I am facing this issue when moving to Realm Mobile Platform as well, so it is not just a problem when using custom sync. My app was local and preloaded with a default realm file. The user was then free to modify the data as they wish. Now I want to move to Realm Mobile Platform. This would mean that multiple users have objects with the same primary key, which means I can't just upload the user data to Realm Mobile Platform.

Zhuinden commented 6 years ago

well i mean if they shouldn't overwrite each other then the user could have their GUID and then the actual item primary key could be userguid_itempk string value 🙄

sipersso commented 6 years ago

@Zhuinden yes, but then the userguid_itempk would have to be in the default realm file too, which it wasn't. In my case though, I guess I could just change the field used as primary key and generate a new one using UUID.randomUUID or a combination of the current id and the userId

@TR4Android In your case, wouldn't just using UUID.randomUUID() suffice to generate a unique identifier on the client?

oantajames commented 4 years ago

Still a big pain to this day. No updates whatsoever? 😞

Zhuinden commented 4 years ago

I guess you could theoretically close all Realm instances on all threads, open the Realm with a DynamicRealm, remove the primary key constraint from your object field, change the value, then add the primary key constraint 🤔

oantajames commented 4 years ago

Thanks @Zhuinden for the response! Theoretically, should that work if I do it in the migration function? I also have a dynamic realm instance there...

oantajames commented 4 years ago

Update: What @Zhuinden suggested did the trick. And it does work also in a migration instance. Just make sure you don't use schema?.addField("id", String::class.java, FieldAttribute.PRIMARY_KEY).tranform... but instead, you do the addField + transform first and then at the end you do schema?.addPrimaryKey("id")

But still, this is something that is missing from the library...

rovkinmax commented 3 years ago

something like this

realmSchema.get(DocUrlMetadata::class.java.simpleName)
            ?.removePrimaryKey()
            ?.addField(DocUrlMetadata::id.name, String::class.java)
            ?.transform { data ->
                data.set(DocUrlMetadata::id.name, data.get(DocUrlMetadata::url.name))
            }
            ?.addPrimaryKey(DocUrlMetadata::id.name)

realm / realm-java