fix problem when deleting a reference when FK is part of foreign PK #521

Closed
wants to merge 4 commits into
from

Projects

None yet

3 participants

@woodspire

Here is a example to better explain the problem and the solution.

2 tables: request & requestStockCode

request:
id -> PK
name

requestStockCode
request_id -> PK
stockCode -> PK
comment

when deleting a stockcode from the request object, the request_id colulmn in the requestStockCode object was set to null, even in the "deletion" list. When the doSave() function was called, no entry in the requestStockCode table was deleted because the request_id was null on each of these object.

The "clone" keyword was missing so that the "deletion" list of objects would be unaffected by setting the FK to null.

Felix Labrecque fix problem when deleting a reference table
and the FK in part of the PK of the foreign table.
ff1b1a1
@jaugustin
Propel member

could you fix your PR, you change the "end of line" char

could you add tests to prove the issue

Your PR broke 3 tests, could you also fix them.

@woodspire

I was able to configure and run the test suite. I have the same errors. I am checking why my pull request is causing the test to fail.

@woodspire

ah! that's why I was not able to see the diff, because I changed the "end of line" char. I use an hybrid dev environment (windows-linux), that that happen that I commit with windows instead of linux and vice-versa and that the end of line get messed up.

Also, I am fixing my errors (because I was able to install and run the testsuite). And I am adding 2 new tests to show why I asked for the changes.

@willdurand
Propel member

You can also "fix" some stuff automatically using the PHP CS fixer: http://cs.sensiolabs.org/ (don't use it on classes containg code templates, like builders).

@willdurand
Propel member

well, obviously you patched a builder, so ignore my previous comment.

woodspire added some commits Nov 30, 2012
@woodspire woodspire add 2 test cases to show the problem.
testRemove_CompositePK is what symfony form bind() method does when a relation is removed from the main object.

testMove_CompositePK is another example when we try to override the relation, but I cannot make my code works for it.

Also, I changed the "clone" for an unserialize(serialize()) in the set() method because it was causing a problem, but using the same trick in the remove() method cause a problem.

Really strange.
64bdfe4
@woodspire woodspire remove stupid CRLF setting in turtoisegit. 7cee1bf
@woodspire woodspire using dos2unix to make sure the end of line are correct 04fe9e7
@woodspire

My latest commit explain the problem in the test case.

The auto-build is currently failing, but looking at the log, it's because there is an internal problem with the build environment.

My second test case (testMove_CompositePK) is a little more exotic, and my code does not currently pass it.

I currently have no idea why both books think they have the same BookOpinion. Really strange because when you fetch the BookOpinion, the book_id will change depending of the BookQuery used.

But if you look at the table structure in the DB, the book_opinion table PK is a composite of book_id and reader_id. So if there is only 1 BookOpinion (like the assert tell us), then only 1 book can be have a BookOpinion, not both !

@woodspire

The travis build is failing because I added a new test that is failing without fixing it.

I am writing this comment because I am not sure that my new test is correct.

example:

2 books and 1 book review, attach to book #1

and book review PK is compose of an id and of book_id (composite PK)

if we do this:

$reviews = $book1->getBookReviews();

$book2->setBookReviews($reviews);

I taught it would:

  • flag to be removed the book reviews on book2 (none in this case)
  • flag to be removed the book reviews on book1 (because the PK of book review will change)
  • add the book reviews from book1 to book2

I am now thinking that when doing the setBookReviews on book2, the method does not know that these books reviews are currently attached on book1 AND that setting them on book2 will impact the primary key of the book review and that the same book review cannot be attach to book1 and book2 at the same time.

maybe the valid test code should be:

$reviews = $book1->getBookReviews();

$book1->removeBookReviews($reviews);

$book2->setBookReviews($reviews);

this way, the book1 object know that the book reviews will be reassigned.

Which method is the right one so we can build a correct test case to test the current version of Propel ?

P.S. I am using the setBookReviews method, not 'add method'.

the add method would add a book review to the current list.
the set method will delete the current book review and replace them by the provided list.

@woodspire woodspire closed this Dec 7, 2012
@woodspire

will re-open a new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment