Disable generation of Id property

[aminelaadhari]

I am trying to migrate to squidb but my old database uses String ids. And as squidb supports only INTEGER as primary key, the generated class has now two methods returning 'ids': the rowid and the server id. This situation is error prone because all my code is base on the server id.

Any idea how to disable the generation of the Id property ?

[sbosley]

This depends a little bit on exactly what you're asking.

If you've already declared your String server id column explicitly with a PRIMARY KEY constraint, this is probably not currently possible. SQLite only supports a single primary key per table, and squidb assumes it to be the rowid as a means of bookkeeping about whether or not a model object exists in the table yet. We can take a look at enhancing things in such a way that you could specify the rowid to NOT be a primary key, but I can't say for sure what changes that might require -- it'll take a bit of experimenting, as we'd still need to use the rowid for bookkeeping.

It sounds though like you might be asking more about naming. The base TableModel class that supports all table models does declare getId() and setId() methods for the long rowid, so those can't be removed or renamed -- if you're already using those method names for accessing the server id there's not much that can be done without updating the code that calls those methods.

If on the other hand those accessor methods DON'T exist yet, it will be possible to make sure that there's no naming confusion. The getter/setter/Property constant names are derived from the name of the field in your model spec, but the column name underlying them can be declared as something else using the @ColumnSpec annotation. So, you could declare something like this:

class MyModelSpec {

    // This will make the squidb integer primary key column be named myRowId instead of _id,
    // and the generated Property for the column will be the same. However, the getId and setId
    // methods will still exist as they're in the base class rather than generated code
    @PrimaryKey
    long myRowId;

    // This will make the accessors be getServerId/setServerId, but the underlying column will
    // be named "_id" -- useful if it already exists that way.
    @ColumnSpec(name = "_id")
    String serverId;
}

I've been meaning to look into supporting things other than integer primary key for a while, but it's never been a high priority. Again, whether or not what you're asking will be possible right now depends a bit on your specific use case, but I'll leave the issue open to motivate us to improve our support for different kinds of primary keys.

[aminelaadhari]

Thanks a lot, it's very helpful !

if you're already using those method names for accessing the server id there's not much that can be done without updating the code that calls those methods

This is exactly my case, so I renamed the server id before starting the migration.

Is there a way to make the generated getter/setter of the row id private ? this way we ensure that we use use always the server id instead on the local id (rowid).

[sbosley]

Unfortunately not at the moment. Those methods are actually defined in a base class rather than being code generated, as seen here: TableModel#getId().

I started working on a change that will eliminate the restrictions on our primary key support and use the internal SQLite rowid column for our bookkeeping, rather than requiring that all table models generate an _id integer primary key. As a part of this change I'll be deprecating those existing getId() and setId() methods, but we can't remove them entirely until the next major version release, as it would be a backwards-incompatible change to the public API and we'd need to give users some time to update. While deprecating them wouldn't prevent them from being called, it would at least be a warning every time you did -- so in the meantime, maybe that would be enough of a deterrent in your use case to ensure that you always used the server id?

I know it's still not 100% ideal for your use case at the moment but I appreciate that you're forging ahead and giving us this feedback! It's made me take a very close look at how we treat the autogenerated ID column and primary keys in general, and I think squidb will be better for it once some of these changes land.

[aminelaadhari]

This is awesome, I think it will help a lot to avoid the confusing of having two different ids. Maybe an option is to make the generation of the setId and getId configurable, it will be convenient for the new users like me without any backwards-incompatible changes.

I really appreciate your efforts on creating this library and your responsiveness. It makes my migration journey a lot easier. If interested I can give you more feedback on the migration difficulties.

[sbosley]

Happy to help! If you wanted to provide a list of the pain points you ran into when migrating it could be helpful for informing other feature requests. I can guess at what some of them are, but I'd be interested to get your perspective to see if I'm right :)

Maybe an option is to make the generation of the setId and getId configurable, it will be convenient for the new users like me without any backwards-incompatible changes.

This might work, not a bad idea at all. I'll have to experiment, but maybe as part of the changes I'm working on we can start generating those methods so that we can remove them from the base class. Then we can add some kind of temporary option to disable generating those methods for users like yourself.

[aminelaadhari]

Happy to see that there is already a PR for this :) Is there any snapshot that I can start using ?

[sbosley]

Not yet, but we may publish a beta 3.1.0 version after the PR has been reviewed and merged.

I should also mention that unfortunately it wasn't possible in this PR to remove the getId and setId methods from TableModel after all, so the restriction of "no user properties named ID" will have to remain for now. The main reason for this is that users (including ourselves) may have defined custom methods operating generically on TableModel instances that call getId and setId, so removing them from the class entirely would be a breaking change even if they were generated in the model classes. We'll be able to remove this restriction whenever the next major version of SquiDB lands, because we'll have given users enough time to upgrade from the deprecated getId/setId to the new getRowId/setRowId that replace them.

I think other than this restriction the PR addresses the use cases discussed here, but feel free to review the summary in the PR and add comments if you have other thoughts!

[aminelaadhari]

Ok thanks. I looked at the PR and it looks fine except the fact that I can't name any property as Id.

To better understand things and to best implement my migration from ormlite to Squidb: Now I am using integer and string Id properties and they are named "id".

For The classes with an integer id proeperty: nothing to do. I can keep using them.

For the classes with string id property: I have to rename the id field to something else (for example uid), and with the new beta I can annotate them with @PrimaryKey

is that correct ?

[sbosley]

That sounds mostly correct, yes. A few things to take note of, just to be sure:

• The property and underlying column don't have to be named the same thing -- the property name comes from the field name, but you can name the underlying column something else using @ColumnSpec. This will probably be useful to you to make sure that the column names match in your migration.
• The default squidb integer primary key ID property makes a column named _id, so if this doesn't match your existing schema you should use @ColumnSpec to make sure the column names match, even in those tables that use integer primary key ids.
• The restriction of "non-integer primary key properties can't be named ID" only applies to the property name -- any underlying column can still be named id.

[aminelaadhari]

Ok great, everything is clear now. Can't wait to try the new beta.

[sbosley]

If you want to try it now so you don't have to wait for an official beta, you can always build from source as described here.

[aminelaadhari]

Great, thanks !

[aminelaadhari]

It's working great ! Thanks for all the help, I just finished migrating the code and works like a charm. I didn't have any issue until now.

[sbosley]

PR merged!