Has-many through looks for intermediate association on wrong class (wrong side of the association) #280

Open
qris opened this Issue Apr 8, 2013 · 9 comments

Comments

Projects
None yet
3 participants

qris commented Apr 8, 2013

It's not clear from the documentation whether the association you want to go through is on the current class, or the far end class. In the example, the payments association is present on both, for no apparent reason.

However, it intuitively makes sense to me that it should be on the current class. If you are going from A->B->C, it makes sense to look for the A->B association on A, not on C.

I have the following class definitions (abbreviated for clarity, I hope not too much):

class Curriculum extends ActiveRecord\Model
{
    static $belongs_to = array(
        array('subject', ...),
    );
}

class Lesson extends ActiveRecord\Model
{
    static $has_many = array(
        array('lesson_curricula', 'class_name' => 'LessonCurriculum',
            'foreign_key' => 'LessonId'),
        array('curricula', 'class_name' => 'Curriculum',
            'through' => "lesson_curricula"),
    );
}

class LessonCurriculum extends ActiveRecord\Model
{
    static $belongs_to = array(
        array('lesson'),
        array('curriculum'),
    );
}

And when I try to access Lesson->curricula I get the following error (with extra debugging added):

ActiveRecord\UndefinedAssociationException: Undefined association: Curriculum->lesson_curricula (should be one of: subject, topic, subtopic, outcome, lessons) at /home/installuser/Dropbox/projects/ischool/website/web/lib/php-activerecord/lib/Relationship.php:505

/home/installuser/Dropbox/projects/ischool/website/web/lib/php-activerecord/lib/Relationship.php:505
/home/installuser/Dropbox/projects/ischool/website/web/lib/php-activerecord/lib/Model.php:523
/home/installuser/Dropbox/projects/ischool/website/web/lib/php-activerecord/lib/Model.php:350
/home/installuser/Dropbox/projects/ischool/website/tests/HomeWebsiteTest.php:842

Note that it's looking for the lesson_curricula association on Curriculum (the far side) for no apparent reason. Of course it doesn't exist there; it's on the Lesson class instead.

Is there a reason for this? Am I doing it wrong?

Contributor

anther commented Apr 8, 2013

I think this may currently be an error on your part if you're using the latest version on github.
Right now I think Lesson->curricula has a dead end, because once you get to LessonCurriculum, it tries to find LessonCurriculum->curricula.

So in LessonCurriculum try changing array('curriculum'), to array('curricula','class_name'=>etc...), and perhaps set up the relationship the other way around too.

Curriculum::$belongs_to = array(
     'lessons','through'=>'lesson_curriculum''),
     'lesson_curriculum','class_name'=>etc)
)

I think I remember getting an error when the relation was not defined the other way around, and the circular relationship probably helps phpactiverecord to know what type of query to generate..? "belongs_to_one vs belongs_to_many"

So, I'm not entirely sure, but it looks like you have to define the circular relationship, so that you can get Lesson->curricula and Curriculum->lessons I kind of wish the documentation showed an example without its usage of 'magical plural understanding', as right now I think the biggest issue is the error message is inaccurate and your code possibly has no idea what a LessonCurriculum->curricula is

qris commented Apr 8, 2013

@anther I am using the latest version from github (a local fork, merged today).

I'm not sure that's correct.

I can see that php-activerecord resolves $lesson->curricula by moving first to LessonCurriculum, as it's following a join that I've created.

If I try to use through to describe the far side of the double join (LessonCurriculum-Curriculum) instead of the near side, like this:

class Lesson extends ActiveRecord\Model
{
    static $has_many = array(
        array('curricula', 'class_name' => 'LessonCurriculum',
            'through' => "curriculum", 'foreign_key' => 'CurriculumId'),
    );
}

class LessonCurriculum extends ActiveRecord\Model
{
    static $belongs_to = array(
        array('lesson',     'foreign_key' => 'LessonId'),
        array('curriculum', 'foreign_key' => 'CurriculumId'),
    );
}

class Curriculum extends ActiveRecord\Model
{
    static $has_many = array(
        array('lessons', 'class_name'=>'Lesson',
            'foreign_key'=>"CurriculumId"),
    );
}

Then I get a database error, probably because it's trying to use the wrong joins in the wrong way:

ActiveRecord\DatabaseException: SELECT `mmLessonCurriculum`.*
FROM `mmLessonCurriculum`
INNER JOIN `Curriculum` ON(`mmLessonCurriculum`.id = `Curriculum`.lesson_curriculum_id)
WHERE `CurriculumId`=?:
HY000, 1, no such column: Curriculum.lesson_curriculum_id (sqlite://unix(/run/shm/lessons.test.sqlite))

/home/installuser/Dropbox/projects/ischool/website/web/lib/php-activerecord/lib/Connection.php:307
/home/installuser/Dropbox/projects/ischool/website/web/lib/php-activerecord/lib/Table.php:237
/home/installuser/Dropbox/projects/ischool/website/web/lib/php-activerecord/lib/Table.php:228
/home/installuser/Dropbox/projects/ischool/website/web/lib/php-activerecord/lib/Model.php:1600
/home/installuser/Dropbox/projects/ischool/website/web/lib/php-activerecord/lib/Relationship.php:536
/home/installuser/Dropbox/projects/ischool/website/web/lib/php-activerecord/lib/Model.php:523
/home/installuser/Dropbox/projects/ischool/website/web/lib/php-activerecord/lib/Model.php:350
/home/installuser/Dropbox/projects/ischool/website/tests/HomeWebsiteTest.php:842

If I have to describe the reverse relation (Curriculum->LessonCurriculum) instead, then that's exactly what I'm saying: php-activerecord is looking from the wrong (unintuitive) side (the furthest away class) for the through relation.

I can't say how it's supposed to work in php-activerecord. But I think it's designed to follow Rails Activerecord, and the examples there are very clear:

class Physician < ActiveRecord::Base
    has_many :appointments
    has_many :patients, :through => :appointments
end

class Appointment < ActiveRecord::Base
    belongs_to :physician
    belongs_to :patient
end

This is intuitive, and what I was expecting to work, and I think it's what I wrote above, but it doesn't seem to work.

Contributor

anther commented Apr 8, 2013

Alright, I recreated a basic version of you db schema and the exception I get is a little different but I'm sure it's the one you got before you made the error message better.
ActiveRecord\HasManyThroughAssociationException: Could not find the association lesson_curricula in model Lesson

Here's the modification I made that made it work for me.. which I know is pretty unintuitive and an extra step from rails.. and quite possibly a bug.

class Curriculum extends ActiveRecord\Model
{
    //For some reason it needs this
    static $belongs_to = array(
         array('lesson_curricula', 'class_name'=>'LessonCurriculum',
            'foreign_key'=>'curriculum_id'),
    );
}

class Lesson extends ActiveRecord\Model
{
    static $has_many = array(
        array('lesson_curricula', 'class_name' => 'LessonCurriculum',
            'foreign_key' => 'lesson_id'),
        array('curricula', 'class_name' => 'Curriculum', //It could not figure out curricula
            'through' => "lesson_curricula"), 
    );
}

class LessonCurriculum extends ActiveRecord\Model
{
    static $belongs_to = array(
        array('lesson','foreign_key'=>'lesson_id','class_name'=>'Lesson'),
        array('curricula', 'class_name' => 'Curriculum', 'foreign_key'=>'curriculum_id'), 
    );
}

And I'm sure you're going to look at this and say "Well wtf is 'curriculum_id'", and to that I respond I think I encountered ANOTHER very unfortunate bug where it's ignoring the foreign_key specified in the relations. (At the very least in the case of camel cased ids), and attempting to guess the foreign_keys as opposed to using the provided ones. It only worked when I renamed the keys in the table to curriculum_id and lesson_id

qris added a commit to aptivate/php-activerecord that referenced this issue Apr 8, 2013

Workaround the use of incorrect foreign keys in has_many through.
See jpfuentes2#280 (#280). Overriding
the keys provided by the user is dangerous (if an exception is thrown),
should not be necessary anyway, and breaks the `foreign_key` option.

qris commented Apr 8, 2013

@anther I agree, this looks like the same bug, thanks for reproducing it!

I found the keys bug as well. I made some hacky workarounds in my own code; well actually, I think it may be an improvement. Relationship changes its own foreign keys temporarily, overwriting the user provided ones, by passing $override == TRUE to set_keys(). I don't think this is a good idea, so I disabled it. (If the query throws an exception, the foreign keys will not be reset to the correct values, and the relationship will remain broken).

I think it happens because construct_join_sql() uses the wrong relationships to get the keys necessary for the join. But it might also be due to the choice of the wrong Model to search for the relationship on, and/or my workaround may not be correct when that bug is fixed, so I'd like to fix it first.

Anyway you can review the changes that I made here.

qris commented Apr 9, 2013

@anther I think I have fixed this properly on my branch: https://github.com/aptivate/php-activerecord/commits/master.

Note that this breaks backwards compatibility with through relationships, because it now looks for them on the same Model, not the remote side.

I know it's huge and scary. It passes all my tests including the following:

$this->assertEquals("INNER JOIN `mmLessonCurriculum`  ON ".
    "(`Lesson`.id = `mmLessonCurriculum`.LessonId)",
    Lesson::table()->create_joins(array('lesson_curricula')));
$this->assertEquals("INNER JOIN `mmLessonCurriculum`  ON ".
    "(`Lesson`.id = `mmLessonCurriculum`.LessonId) ".
    "INNER JOIN `Curriculum`  ON ".
    "(`mmLessonCurriculum`.CurriculumId = `Curriculum`.id)",
    Lesson::table()->create_joins(array('curricula')));
$this->assertEquals("INNER JOIN `mmLessonCurriculum`  ON ".
    "(`Curriculum`.id = `mmLessonCurriculum`.CurriculumId) ".
    "INNER JOIN `Lesson`  ON ".
    "(`mmLessonCurriculum`.LessonId = `Lesson`.id)",
    Curriculum::table()->create_joins(array('lessons')));
Contributor

anther commented Apr 9, 2013

I can't initialize the library with a straight pull of your branch. :(
Fatal error: Call to protected ActiveRecord\Connection::__construct() from context 'ActiveRecord\Config'

Maybe you could try the fix in the bare bones version of the library and get it to pass the tests, I have a branch where I've written a failing test for this particular bug that I can push later on if you would like to have a go at it. =p.

qris commented Apr 10, 2013

@anther OK, where can I get your branch and how do I run all the tests for php-activerecord?

Contributor

anther commented Apr 11, 2013

I think this link may work...
https://github.com/anther/php-activerecord/tree/relationship_custom_foreign_keys

You'll need to install phpunit: http://www.phpunit.de/manual/3.8/en/installation.html and get it running. If you use eclipse's PDT or aptana you can install a gui for it, but I believe most people run the tests through command line. (I really wish there was a widely accepted GUI for phpunit... unit testing's more fun with colors.

And for phpativerecord there's the tests folder with all of the test cases. You should be able to experiment a bit and figure out how to get them running in no time.

Contributor

louismrose commented Dec 8, 2016

Edit: On closer inspection, my suggestion below does not work, because PHP AR uses the "source" attribute to instantiate objects accessed through the relationship. (So you would end up with a bunch of Lesson objects when trying to call $lesson->curricula). It seems the only solution right now is to add the symmetric relationship: i.e., a has-many-through A->B->C will only work when there is a relationship from A->B and from C->B.


I think I ran into this too, and a workaround (for me at least!) is to use the source attribute on the has-many-through association to force PHP ActiveRecord to look for the intermediate association on the defining class.

So, for the OP's example, the code would become:

class Curriculum extends ActiveRecord\Model
{
    static $belongs_to = array(
        array('subject', ...),
    );
}

class Lesson extends ActiveRecord\Model
{
    static $has_many = array(
        array('lesson_curricula', 'class_name' => 'LessonCurriculum',
            'foreign_key' => 'LessonId'),
        array('curricula', 'class_name' => 'Curriculum',
            'through' => "lesson_curricula",
            'source' => 'Lesson' // <= this is the fix
        )
    );
}

class LessonCurriculum extends ActiveRecord\Model
{
    static $belongs_to = array(
        array('lesson'),
        array('curriculum'),
    );
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment