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

readPropertiesFromCursor combined with qualifiedFields #227

Closed MFlisar closed 8 years ago

MFlisar commented 8 years ago

Is this possible?

I select something from different tables, all with qualified fields, and now I want to create the objects with the corresponding values defined by those qualified fields. Can I do that without using the index?

sbosley commented 8 years ago

I'm not 100% sure what you're asking but I think the answer is yes. Is this what you're describing?

Table table2 = MyModel.TABLE.as("table2");
StringProperty qualifiedProp = table2.qualifyField(MyModel.SOME_STRING);
Query query = Query.select(qualifiedProp).from(table2);
SquidCursor<Model> cursor = db.query(MyModel.class, query);
MyModel model = new MyModel();
model.readPropertiesFromCursor(cursor, qualifiedProp);

I'm pretty sure this should work, have you tried it?

The caveat is that if the property also has an alias in addition to being qualified by a different table, it will work but not quite as expected. Properties/values are populated into models using the property name, so if the property used is aliased it will use a different name than the original column. Trying to persist such a model would be an error because the column name would be wrong, and you'd have to read the value using the aliased property rather than the default getter.

Also, in the future please direct questions like these to our Gitter chat at https://gitter.im/yahoo/squidb. We've started trying to streamline how we handle issues, so we'd prefer to use Gitter for support and answering/clarifying questions, and use Github issues for things that are either bugs or feature tracking. Thanks!

MFlisar commented 8 years ago

OK, I'll ask questions there in the future.

Here's a simple example:

public static ArrayList<DBMedia> test()
{
    ArrayList<DBMedia> medias = new ArrayList<>();

    List<Field<?>> fields = new ArrayList<>();
    List<Field<?>[]> fieldsFolders = new ArrayList<>();

    Table tableFolder1 = DBFolder.TABLE.as("F1");
    fieldsFolders.add(tableFolder1.qualifyFields(DBFolder.PROPERTIES));

    Table tableFolder2 = DBFolder.TABLE.as("F2");
    fieldsFolders.add(tableFolder2.qualifyFields(DBFolder.PROPERTIES));

    fields.addAll(Arrays.asList(DBMedia.PROPERTIES));
    for (int i = 0; i < fieldsFolders.size(); i++)
        fields.addAll(Arrays.asList(fieldsFolders.get(i)));

    Query query = Query.select(fields).from(DBMedia.TABLE)
            .leftJoin(tableFolder1, tableFolder1.qualifyField(DBFolder.ROWID), DBMedia.FK_FOLDER1)
            .leftJoin(tableFolder2, tableFolder2.qualifyField(DBFolder.ROWID), DBMedia.FK_FOLDER2);

    SquidCursor<MediaTag> cursor = MainApp.getDao().query(null, query);
    cursor.moveToFirst();
    if (!cursor.isAfterLast())
    {
        DBMedia media = new DBMedia();
        media.readPropertiesFromCursor(cursor);

        DBFolder folder1 = new DBFolder();
        DBFolder folder2 = new DBFolder();
        // read folders with qualified fields - HOW?
        // TODO...

        media.initData(folder1, folder2)

        medias.add(media);
        cursor.moveToNext();
    }
    cursor.close();

    return medias;
}

The resulting query looks like following:

SELECT media.rowid AS rowid,
       media.fkFolder1 AS fkFolder1,
       media.fkFolder2 AS fkFolder2,
       // rest of media
       F1.rowid AS rowid,
       // rest of F1...
       F2.rowid AS rowid,
       // rest of F2...
FROM media
LEFT JOIN folder AS F1 USING (rowid, fkFolder1)
LEFT JOIN folder AS F2 USING (rowid, fkFolder2)

This seems wrong. qualifiedField should NOT generate the column alias but use the unique column names in the format of <TABLE>.<COLUMN>

sbosley commented 8 years ago

No, that's the correct/expected SQL. qualifyFields doesn't set a column alias, but as we discussed in issue #112 (which I think you recently closed), all fields in a select statement need to have an AS something clause to force SQLite to use that column name in the result set. If one isn't given, we use the column name, which can lead to duplicates. So in your case you can either explicitly alias the columns with duplicate names (which will require some additional maneuvering to read them from the result) or you can use a ViewModel backed by a subquery, which will automatically alias things at compile-time for you.

MFlisar commented 8 years ago

I don't like the fact that I have to define every field of every table in a ViewModel... That's why I'm trying to get around it.

I now have following:

public static ArrayList<DBMedia> test()
{
    ArrayList<DBMedia> medias = new ArrayList<>();

    List<Field<?>> fields = new ArrayList<>();
    List<Property<?>[]> fieldsFolders = new ArrayList<>();

    Table tableFolder1 = DBFolder.TABLE.as("F1");
    fieldsFolders.add(DBUtilsV2.getTablePropertiesWithAlias(tableFolder1));

    Table tableFolder2 = DBFolder.TABLE.as("F2");
    fieldsFolders.add(DBUtilsV2.getTablePropertiesWithAlias(tableFolder2));

    fields.addAll(Arrays.asList(DBMedia.PROPERTIES));
    for (int i = 0; i < fieldsFolders.size(); i++)
        fields.addAll(Arrays.asList(fieldsFolders.get(i)));

    Query query = Query.select(fields).from(DBMedia.TABLE)
        .leftJoin(tableFolder1, DBUtilsV2.getPropertyWithAlias(DBFolder.ROWID, tableFolder1).eq(DBMedia.FK_FOLDER1))
        .leftJoin(tableFolder2, DBUtilsV2.getPropertyWithAlias(DBFolder.ROWID, tableFolder2).eq(DBMedia.FK_FOLDER2));

    SquidCursor<MediaTag> cursor = MainApp.getDB().query(null, query);
    cursor.moveToFirst();
    if (!cursor.isAfterLast())
    {
        DBMedia media = new DBMedia();
        media.readPropertiesFromCursor(cursor);

        DBFolder folder1 = new DBFolder();
        folder1.readPropertiesFromCursor(cursor, fieldsFolders.get(0));

        DBFolder folder2 = new DBFolder();
        folder2.readPropertiesFromCursor(cursor, fieldsFolders.get(1));

        media.initData(folder1, folder2);

        medias.add(media);
        cursor.moveToNext();
    }
    cursor.close();

    return medias;
}

Question

Why are my aliases removed from the join part of the query? The resulting query looks like following:

SELECT
   // ...
FROM
   media 
LEFT JOIN
   folder AS F1 
      ON (
         folder.rowid=media.fkFolder1
      ) 
LEFT JOIN
   folder AS F2 
      ON (
         folder.rowid=media.fkFolder2
      )
)

Here are my helper functions:

public static Property getPropertyWithAlias(Property property, Table table)
{
    return property.as(table.getName() + "_" + property.getName());
}

public static Property[] getTablePropertiesWithAlias(Table table)
{
    Property[] props = new Property[table.getProperties().length];
    for (int i = 0; i < table.getProperties().length; i++)
        props[i] = getPropertyWithAlias(table.getProperties()[i], table);
    return props;
}
sbosley commented 8 years ago

Your getPropertyWithAlias() function gives the column an alias, which is used for the select clause. The fully qualified name of the property is still used everywhere else in the query. So when you construct the join clause, it should be on the qualified field, not the aliased field:

Query query = Query.select(fields).from(DBMedia.TABLE)
            .leftJoin(tableFolder1, tableFolder1.qualifyField(DBFolder.ROWID).eq(DBMedia.FK_FOLDER1))
            .leftJoin(tableFolder2, tableFolder2.qualifyField(DBFolder.ROWID).eq(DBMedia.FK_FOLDER2));

Also, since you're adding the aliased fields to the select clause, I think you may want that getPropertyWithAlias() function to be qualifying the fields for the given table in addition to aliasing them. Because isn't this the SQL you want?

SELECT
   media.col_1 as media_col_1, ...
   F1.col_1 as F1_col_1, F2.col_2 as F2_col_2, ...
   F2.col_1 as F2_col_1, F2.col_2 as F2_col_2, ...
FROM
   media 
LEFT JOIN
   folder AS F1 
      ON (
         F1.rowid=media.fkFolder1
      ) 
LEFT JOIN
   folder AS F2 
      ON (
         F2.rowid=media.fkFolder2
      )
)

In order to get those fields in that form in the select clause they need to be both qualified (to get the table qualifier) and aliased (to get the unique AS clause).

Still, I urge you to consider using the ViewModel approach instead. Yeah, it's boilerplate, but in the long run I'll bet it's less boilerplate. Those readPropertiesFromCursor calls aren't going to work the way I think you're expecting them to -- they'll populate the model with the alias names rather than the real names, meaning the normal getters for reading data from the model won't work since they're looking at the wrong names. This is why ViewModel is good -- the mapToModel() methods do all of this boilerplate of converting from the safely aliased property names back to the real names for you, so the model objects can be used as expected.

sbosley commented 8 years ago

I should also mention that way back when issue #112 was open, we did create an experimental branch to do better with this kind of thing -- generating more unique aliases in the SELECT clauses and reading them back into the models more safely. We never merged it because a) it was a pretty complex change and b) it wreaked havok with backwards compatibility, but we may still introduce that enhancement in a future version.

MFlisar commented 8 years ago

Thanks for the patience. I see, the reading functions are not working, your right... Then I'll go back to the ViewModel solution and try to get this one working.

sbosley commented 8 years ago

Ok sounds good. I do feel your pain with the boilerplatey nature of ViewModel specs, so I'll keep thinking about it. I've been trying to figure out a way to include a full list of properties in a ViewModel for a while but never came up with a good solution. Haven't thought about it in a while but may revisit it again at some point.