yahoo / squidb

SquiDB is a SQLite database library for Android and iOS
https://github.com/yahoo/squidb/wiki
Apache License 2.0
1.31k stars 132 forks source link

QueryBuilder with join - reading multiple models with same fields (ID) #112

Closed MFlisar closed 8 years ago

MFlisar commented 9 years ago

Here's how I tried to use it, but this had one problem: the default ID is wrong. The MediaTag reads the ID from the Tag model...

Query query = Query.select().from(MediaTag.TABLE).leftJoin(Tag.TABLE, MediaTag.FK_TAG.eq(Tag.ID));
SquidCursor<MediaTag> cursor = MainApp.getDao().query(null, query);
cursor.moveToFirst();
while (!cursor.isAfterLast())
{
    MediaTag mediaTag = new MediaTag();
    Tag tag = new Tag();
    mediaTag.readPropertiesFromCursor(cursor);
    tag.readPropertiesFromCursor(cursor);
    tags.add(new WrappedTag(tag, mediaTag));
    cursor.moveToNext();
}
cursor.close();

This happens because the query gives all fields simple aliases without table prefixes. Do I really have to manually create aliases and use them and read them? Following works:

List<Field<?>> fields = new ArrayList<>();
fields.add(MediaTag.ID.as("ID1"));
... other fields ...
fields.add(Tag.ID.as("ID2"));
... other fields ...

Query query = Query.select(fields).from(MediaTag.TABLE).leftJoin(Tag.TABLE, Tag.ID.eq(MediaTag.FK_TAG)).orderBy(MediaTag.PATH.asc());
SquidCursor<MediaTag> cursor = MainApp.getDao().query(null, query);
cursor.moveToFirst();
while (!cursor.isAfterLast())
{
    MediaTag mediaTag = new MediaTag();
    Tag tag = new Tag();
    mediaTag.readPropertiesFromCursor(cursor);
    tag.readPropertiesFromCursor(cursor);
    tag.setId(cursor.getLong(3));
    mediaTag.setId(cursor.getLong(0));
    tags.add(new WrappedTag(tag, mediaTag));
    cursor.moveToNext();
}
cursor.close();

Is there an easier way? That does not require to manually define fields? Or can I somehow enable to use no aliases in the query and use prefixed fields instead?

sbosley commented 9 years ago

At the moment, you will need to explicitly give columns with the same name aliases for the query.

The reason we give all columns aliases when building the query is due to an obscure behavior in SQLite where if no AS clause is specified for a column in a SELECT statement, no guarantees are made about what the names of the result columns will be -- see the last paragraph of this page. Usually it's given a reasonable name, but we actually have run into cases where the name in the result set is something totally random, and so we give all columns aliases.

We can evaluate if there's a better way to generate these aliases to make them unique. I can't guarantee it will work because Property names are closely tied with insert/update operations as well as reads, but we can see if there's a better solution.

MFlisar commented 9 years ago

I see, didn't know that. Why not use it like following: select table.column1 as table_column1? The default id will make problems in every join query if someone uses the default id column...

Another solution I've seen in an orm already is, to allow to define exact offsets for readPropertiesFromCursor(int offset, Cursor cursor). In my case I would use it like following:

mediaTag.readPropertiesFromCursor(0, cursor);
tag.readPropertiesFromCursor(3, cursor);

The offset defines that I don't want to read as many fields as the table defines, one by one, starting at the offset. This means, above would be resolved as following:

mediaTag.setProperty1(cursor.getCursor().getInt(offset + 0));
mediaTag.setProperty2(cursor.getCursor().getString(offset + 1));
...

This would at least not touch any deeper integration...

sbosley commented 9 years ago

I actually starting working on something like your first suggestion as soon as I read the issue to see if we can make that work :)

readPropertiesFromCursor already allows passing a varags list of Property objects to be read, although this will still only work if the names in the cursor are unique:

mediaTag.readPropertiesFromCursor(cursor, MediaTag.PROPERTIES);
tag.readPropertiesFromCursor(cursor, Tag.PROPERTIES);

We want to try hard to avoid forcing the user to ever think about column indexes, types, etc. When working with properties, even when it's with a cursor, you should almost always use SquidCursor.get:

mediaTag.setId(cursor.get(MediaTag.ID));
tag.setId(cursor.get(Tag.ID));

Again though, as you correctly pointed out, this only works if the selected names in the properties are unique. I'm pretty optimistic that just generating more unique aliases under the hood and using those will work though.

sbosley commented 9 years ago

Another thing you could consider is working with a view model when doing a common join query like this. ViewModels do some automatic aliasing for you to prevent exactly this problem, and you can split a single view model back into its constituent models using mapToModel.

sbosley commented 9 years ago

Quick update -- it turns out that a simple change to how query objects are compiled will break compatibility with views, since the SQL for creating views is itself generated by compiling a query. There may still be some solution to this so I'll leave the issue open and I intend to try more experiments, but it won't be as quick a fix as I was hoping.

In the meantime, I'd recommend that you try a ViewModel backed by a subquery. It will encapsulate the data grouping represented by your join nicely and take care of all the aliasing issues for you.

MFlisar commented 9 years ago

I think this restriction should be somewhere in the tutorials, as it's really not expected... Took me some time to find out, why my delete functions were not working.

View models seem to work, I'll check them and try them out. Thanks