Left outer join for nullable foreign keys

[vitomir.s...@gmail.com]

Looks like QDjango sometimes do not use LEFT OUTER JOIN in SELECT statement even if it should use it for joining by all nullable foreign key fields. Then, fetching objects together with foreign key objects in one query (with selectRelated() call) is not possible because INNER JOIN will return zero rows.

As I can see in QDjango sources, baseModel.localField() call in line (2) has wrong parameter, because "name" is not a local field name. But I am not sure yet what should be used instead. My specific test case where this error is manifesting is with three table one-to-many hierarchy: Company->Partner->Country where country id in Partner table is nullable and partner id in Company table is not nullable. With one level hierarchy error is not happening, though.

And, BTW line (1) does not do anything and could be removed.

QString QDjangoCompiler::fromSql()
{
    QString from = driver->escapeIdentifier(baseModel.table(), QSqlDriver::TableName);
    foreach (const QString &name, modelRefs.keys()) {
        baseModel.localField(name.toLatin1() + QByteArray("_id"));                                                          (1)
        from += QString::fromLatin1(" %1 %2 %3 ON %4.%5 = %6")
            .arg(baseModel.localField(name.toLatin1() + QByteArray("_id")).isNullable() ? "LEFT OUTER JOIN" : "INNER JOIN") (2)
            .arg(driver->escapeIdentifier(modelRefs[name].second.table(), QSqlDriver::TableName))
            .arg(modelRefs[name].first)
            .arg(modelRefs[name].first)
            .arg(driver->escapeIdentifier(modelRefs[name].second.localField("pk").column(), QSqlDriver::FieldName))
            .arg(reverseModelRefs.contains(name) ? databaseColumn(reverseModelRefs[name]) :
                                                   databaseColumn(name + QLatin1String("_id")));
    }
    return from;
}


Regards,
Vitomir

[Jeremy Lainé]

On 07/04/2014 09:05 PM, vitomir.s...@gmail.com wrote:
> Looks like QDjango sometimes do not use LEFT OUTER JOIN in SELECT
> statement even if it should use it for joining by all nullable foreign
> key fields. Then, fetching objects together with foreign key objects
> in one query (with selectRelated() call) is not possible because INNER
> JOIN will return zero rows.
>
> As I can see in QDjango sources, baseModel.localField() call in line
> (2) has wrong parameter, because "name" is not a local field name. But
> I am not sure yet what should be used instead. My specific test case
> where this error is manifesting is with three table one-to-many
> hierarchy: Company->Partner->Country where country id in Partner table
> is nullable and partner id in Company table is not nullable. With one
> level hierarchy error is not happening, though.
>

From reading the code I am pretty sure the bug you describe does exist,
but I am wondering whether joins with 2 levels work at all!

Could you put together a testcase for integration into
tests/db/qdjangocompiler/tst_qdjangocompiler.cpp which triggers the problem?

> And, BTW line (1) does not do anything and could be removed.
>

Right, it's gone now.

Cheers,
Jeremy

[vitomir.s...@gmail.com]

Thanks for the answer, Jerremy!

Problem is that looking for the joining field is always made in baseModel and only first level joins are there. Luckily, QDjangoCompiler::databaseColumn() method already has almost all what is needed. So, I took this method, simplified it, and made a new method to find correct model field from the relationship name:

const QDjangoMetaField QDjangoCompiler::foreignField(const QString &name)

{

  QDjangoMetaModel model = baseModel;

  QStringList bits = name.split(QLatin1String("__"));

  while (bits.size() > 1) {

    const QByteArray fk = bits.first().toLatin1();

    QDjangoMetaModel foreignModel;

    if (!model.foreignFields().contains(fk)) {

      // this might be a reverse relation, so look for the model

      // and if it exists continue

      foreignModel = QDjango::metaModel(fk);

      if (!foreignModel.isValid())

        break;

    } else {

      foreignModel = QDjango::metaModel(model.foreignFields()[fk]);

    }

    model = foreignModel;

    bits.takeFirst();

  }

  return model.localField(bits.join(QLatin1String("__")).toLatin1());

}

Then I changed line (2) in fromSql() to call this new method:

QString QDjangoCompiler::fromSql()

{

  QString from = driver->escapeIdentifier(baseModel.table(), QSqlDriver::TableName);

  for (const QString &name : modelRefs.keys()) {

    from += QString::fromLatin1(" %1 %2 %3 ON %4.%5 = %6")

        .arg(foreignField(name + QLatin1String("_id")).isNullable() ? "LEFT OUTER JOIN" : "INNER JOIN")

        .arg(driver->escapeIdentifier(modelRefs[name].second.table(), QSqlDriver::TableName))

        .arg(modelRefs[name].first)

        .arg(modelRefs[name].first)

        .arg(driver->escapeIdentifier(modelRefs[name].second.localField("pk").column(), QSqlDriver::FieldName))

        .arg(reverseModelRefs.contains(name) ? databaseColumn(reverseModelRefs[name]) :

                                               databaseColumn(name + QLatin1String("_id")));

  }

  return from;

}

Maybe this solution can be further refined, but with that change it seems to me that it is working correctly.
When I get same time, I will also add a test case, as you asked.

Regards,
Vitomir

[Jeremy Lainé]

I have added a (commented) testcase in tests/db/qdjangocompiler/tst_qdjangocompiler.cpp
which triggers the behaviour you describe:

https://github.com/jlaine/qdjango/commit/aa24695883a8c5d77076b9211b6a9dc91124ca05

I will take a look at your suggested change, but instead of re-resolving the foreign keys,
couldn't we store the relevant information (is the foreign field nullable) during
::databaseColumn ?

Jeremy

[vitomir.s...@gmail.com]

Of course, it would be less code and work faster if databaseColumn() would do everything in one pass. I am, simply, not that much familiar with QDjango sources and didn't want to make any side effect bugs. Thus, I came up with a safe solution, so I can evaluate QDjango further. Except the plain functionality, I was also interested if persistence API can be used with as few (more-or-less) database accesses as if I would use plain SQL queries, and there a proper use of inner and outer joins is certainly important.

I have took a look of this new test case and I can confirm that it is correct.

Vitomir

[Jeremy Lainé]

I have commited a fix which should handle your usecase correctly. I have enabled the
corresponding test.

Can you please let me know whether this works for you?

Jeremy

[vitomir.s...@gmail.com]

Yep, it works now!
Thank you for your help, Jeremy.

Vitomir