Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

FIXED: Crashed caused by viewing versioned page #716

Merged
merged 5 commits into from Aug 20, 2012

Conversation

Projects
None yet
4 participants
Contributor

tractorcow commented Aug 10, 2012

When viewing the archived version of a page the augmentSQL function of the Versioned extension would generate invalid SQL. E.g.

SELECT 
...
FROM SiteTree_versions 
AND Page_versions.Version = SiteTree_versions.Version 
INNER JOIN _ArchiveSiteTree ON _ArchiveSiteTree.ID = SiteTree_versions.RecordID AND _ArchiveSiteTree.Version = SiteTree_versions.Version
...

This is due to an odd $query->addFrom which attempts to re-add an existing from table name, but with an additional join condition; However, it doesn't actually do that, but it adds a SQL fragment "AND .... " in the place of the table name. It's possible that I may have misunderstood the intent of this code. Possibly it assumes the addFrom will perform a recursive table/join/condition merge. Actually, addFrom instead completely replaces the existing from condition.

The actual addFrom is not necessary as the target table would already be listed in the from items.

I have replaced the addFrom with an addWhere condition to add in the new join condition. This however doesn't add the condition in the actual join itself, but in the 'where' clause. This is still valid SQL and achieves the same purpose. I tried a version of this code using addFilterToJoin, but not every "from" in a query (such as _ArchiveSiteTree) would be guaranteed to be in an array format (that addFilterToJoin requires). Could add additional code to check, ensure queries in required format, joined classes are subclasses of $baseTable, but, code is a burden, so simpler is better.

This has been tested against the VersionedTest and passes all 12 tests.

Might need to test this against other modules which augmentSQL (subsites, translatable, etc).

Additionally, there was as small but relevant bugfix where versioned tables were being joined against the wrong fields (ID,Version instead of RecordID, Version). This came about because in the $query->replaceText the tables were renamed first from table to table_versions, and then the string replacer replaced table.id with table.recordID (which by this time, no longer existed).

So much testing investigation and documentation for a small change. :)

tractorcow added some commits Aug 10, 2012

FIXED: Issue where viewing an archived version of a page caused inval…
…id SQL to be generated. This would only occur with subclasses of Page.

@sminnee sminnee closed this Aug 17, 2012

@sminnee sminnee reopened this Aug 17, 2012

This pull request passes (merged c55b018 into 023721a).

Owner

sminnee commented Aug 17, 2012

Thanks very much for this - I don't suppose you could supply a unit test that was previously failing and now works? Something in VersionedTest.php, perhaps?

Contributor

tractorcow commented Aug 17, 2012

I'll look into writing a test next week, if I'm unable to get it out this afternoon. Will let you know.

Thanks for looking at this for me.

Owner

sminnee commented Aug 17, 2012

Sweet as. Just commit to tractorcow:3.0-versioned-fixes and then we'll merge

tractorcow added some commits Aug 20, 2012

FIXED: Issue where temporary table would cause unpredictable behaviou…
…r. Temporary table functionality was substituted with subqueries in each use case.

ADDED: Test case for version archive functionality.
REMOVED: Unnecessary publish actions from test cases
ADDED: Test case for get_all_versions

This pull request passes (merged 89728ac into 023721a).

Owner

sminnee commented Aug 20, 2012

Great!

sminnee added a commit that referenced this pull request Aug 20, 2012

Merge pull request #716 from tractorcow/3.0-versioned-fixes
FIXED: Crashed caused by viewing versioned page

@sminnee sminnee merged commit 47b56d4 into silverstripe:3.0 Aug 20, 2012

1 check passed

default The Travis build passed
Details
Contributor

tractorcow commented Aug 20, 2012

I have added two additional Versioned test cases and checked that all Versioned test cases still pass.

Additionally, I noted an issue with the use of a temporary table when doing versioned queries. The issue would creep in when doing subsequent queries on the same table over different archived dates (as in my test cases). This was due to the rigidity of using a single temporary table over multiple queries.

It could have been fixed using the same method, but I opted instead of replace the temporary table model with a pair of similar (yet different) subquery conditions in each of the two places this function was called.

It could also have been implemented using a join on a derived table, but I wasn't certain how well the query library would handle an aliased query as a table name, so I erred in favour of a slightly more self contained correlated subquery.

Contributor

tractorcow commented Aug 20, 2012

Thanks mate. :) You guys are pretty quick on the update!

Owner

sminnee commented Aug 20, 2012

I'm enjoying Travis' pre-tested pull requests - now I can read over the patch online and click the big green button, knowing there's no build failures. :-)

It's also good to see another dev debugging in the core for the ORM! Looking forward to future patches...

Contributor

tractorcow commented Aug 20, 2012

As am I; It's a new feature isn't it?

Anyway, expect plenty more as I trudge through the Translatable migration. :)

Owner

sminnee commented Aug 20, 2012

Yeah - just set it up a few days ago.

Contributor

tazzydemon commented Oct 31, 2012

I don't think this passes real world tests as in my thread http://www.silverstripe.org/general-questions/show/21283 and ticket 7975. There is, I believe an ID mixup which rears its head when ID and RecordID diverge. The subsequent effect on Hierarchy and CMSSiteTreeFilter is another matter.

Contributor

tractorcow commented Oct 31, 2012

Thanks Tazzy for noting this; I'd like to jump into this but it's likely to be a while before I'm free.

Apologies for the bug as you noted above... it seems I often miss certain cases in my tests, my fault there.

I do highly disagree with one of the points you made in the thread above, however. One of the major changes in the above is moving away from the whole process around using temporary tables, so I'd still expect to find a solution that avoids this. Expressing a query that relies on the state of a particular table to be correct means it's not portable. augmentSQL only generates the query, and doesn't control the order in which queries are executed (or if it even is at all). Even if the process requires another layer of subquery then it would still be far more robust than using an unnecessary temp table.

Are you able to write a test case to demonstrate the error?

I think the code itself could also do with some additional documentation on the assumptions that versioned relies on (as you noted, for example, the relationship between RecordID and ID).

Sorry if any of the above is wrong or unresearched, as it's mostly off the top of my head, so it's possible I'm just spewing garbage. I'll take some time to properly research this and spot the bug in my code.

Thanks for taking the time to get back to me on this. :)

Owner

sminnee commented Oct 31, 2012

The temporary table thing was a bit of a hack, but a working hack does beat an elegant but broken solution. ;-)

At the time of the original SS2 implementation, trying to write it the code without a temporary table made my brain hurt. Of course, this was also a time when we didn't have unit tests, so we've come a long way since then.

Contributor

tazzydemon commented Oct 31, 2012

Well I have never written a test so I will have to learn. The real world example is better in this case when you create a raft of pages and delete them forcing the counts to separate. In the case of Display All Pages Including Deleted these is the remaining problem that the site tree (IMHO) cant find the pages to display them.

As far as I can tell, either the query will have to be replaced entirely to get rid of ID or a function added to each schema.inc to removeField() to get rid of the duplicate ID

I have been buried in print_r $query-sql() lately. My primitive and old-guy (which I am) way of debugging. This problem has been perverse and I got adrexia to report it for me. I forgot at the time about trac. I work on symfony as well (or at least I did until today when a company merger occurred) so switching is always wierd.

I was getting quite despondent that I can't get anybody to respond to this except adrexia, so thank you for that. The takeover company is an MS house so my development of the new sites is on a knife edge and any instability would not be cool.

Try the Display All Pages including Deleted function button. You will find that when you delete a page it will appear as deleted, then then after a refresh. Poof. Gone!

Contributor

tractorcow commented Oct 31, 2012

Well, if it makes you feel better, I'm directly responsible for this, so feel free to bug me all you need in order to get it fixed! I have a couple other rather serious bugs on my plate to fix, so this won't be too big an extra burden I suppose. :)

Contributor

tazzydemon commented Nov 1, 2012

Tractorcow,

Thank you for your response. I fear this might go deeper than you imagine, in the way the cms site tree is displayed and which tables are used to populate the names. It isn't just the Versioned query. I kludged that and the sitetreefilter to see what would happen. I don't know enough to go deeper.

I first noticed this and other wierdness on a three-site subsites install along with the Swedish multilanguage module. I had some decoupling to do to prove it. In the end building several new bare installs. It first came to light when reordering pages by drag and drop and accidentally dropping them into another subsite during a short drag and drop "freeze" so to speak. Not sure of the mechanism there and its almost impossibleto reproduce deliberately. There's no way back from that, although I have suggested it to subsites maintainers. This led me to look at deleted pages as I was creating a few deleted pages from the wrong subsites. The result was unexpected and I have spent days and days analysing why!

Contributor

tazzydemon commented Nov 1, 2012

Dear Tractorcow

Just an observation about your comment that I missed. I'm not sure I was advocating a return to a temporary table. But this, I think, was when the problem started, but not what was responsible.

However I have not established which SS3 version came just before this change or I would have done a checkout of that to compare. If you know then I will do just that. I appreciate that this is very tricky which is why I have spent a stupid amount of time proving it to myself.

Contributor

tractorcow commented Nov 1, 2012

There is no such thing as a stupid amount of time spent when it's on open source code. It's all a part of the course. :) Once you get more into this you'll find that the most trivial of fixes often involve,

  1. Fixing the real world case
  2. Creating a fictional situation test case
  3. Fixing errors in the test case
  4. Fixing the test case in some obscure DB connector library
  5. Fixing that obscure connector error, with its own test case, and wait for it to be absorbed into master
  6. Finally fixing the error in the original problem
  7. Finally write the code that was blocked by your minor error
  8. Once you feel that all is resolved, some sneaky guy down the track notes another (real world / hypothetical, often both) situation where your code reaks havoc.
  9. GOTO step 1.

It's really quite fun at times, and is fulfilling to be able to contribute to something along with others.

I suggest you start by looking at the current test suite for versioned, and see how it is built. Once you understand how databases are able to be mocked (and it really is quite ingenous) you should be able to translate your real-world test situations into standard unit tests. Look at atomic transactions, and think about solving small problems, rather than big processes involving lots of human steps. The worst that could come out of it is that you develop a lot of test cases that prove the existing code already works.

Perhaps look at the assumption you'd like to test above. Do the current test cases assume a direct relationship between ID and RecordID? Perhaps a test case where these are randomly assigned would be a good candidate? You could possibly edit the existing test case with this assumption (with comments describing the testing scenario), without having to create some from scratch, or perhaps you could extend those existing tests with additional data.

Check the VersionedTest::testGetIncludingDeleted function. Are there some cases here that are not currently being tested by the code, but that come up in the real world?

I must sincerely apologise for writing all this up, but not actually doing any work on this myself. If I were less mentally lazy I'd spend some of my own time helping with this.

Once I get some time I'd actually like to go back to the issues I'm struggling with at silverstripe#887. If you have a moment, would you be able to assist me with this? Postgres is causing me some serious issues (despite having rewritten the module personally >_>).

Contributor

tazzydemon commented Nov 7, 2012

The more I work on this the harder it gets (and more embarrassing). I don't really need to look at the test suite. I can see it doesn't work quite clearly without that.

This is what I have done so far:

Versioned.php after L235

$query->selectField(sprintf('"%s_versions"."%s"', $baseTable, 'ID'), "DummyID");
$query->selectField(sprintf('"%s_versions"."%s"', $baseTable, 'RecordID'),sprintf('%s_versions.%s', $baseTable, 'ID'));
$query->addOrderBy(sprintf('"%s_versions"."%s"', $baseTable, 'Version'));

This is to fool the system into not seeing the ID and seeing recordID as ID. The code isn't perfect, especially the quotes in the second line. But it works and I can see deleted entries in the site tree.

Unfortunately it also didnt work... this is the result of a print id $child in Heirarchy.php L102

Page Object .....
(

[record:protected] => Array
    (
        [ClassName] => Page
        [Created] => 2012-11-07 11:08:28
        [LastEdited] => 2012-11-07 11:08:36
        [URLSegment] => tbd
        [Title] => tbd
        [ShowInMenus] => 1
        [ShowInSearch] => 1
        [Sort] => 6
        [HasBrokenFile] => 0
        [HasBrokenLink] => 0
        [CanViewType] => Inherit
        [CanEditType] => Inherit
        [Version] => 2
        [ParentID] => 0
        [ID] => 585
        [RecordClassName] => Page
        [RecordID] => 29
        [WasPublished] => 1
        [AuthorID] => 1
        [PublisherID] => 1
        [DummyID] => 585
        [SiteTree_versions.ID] => 29
        [DropInContent_Lazy] => Page
        [MenuExcerpt_Lazy] => Page
        [DropInLinkID_Lazy] => Page
        [DropInImageID_Lazy] => Page
    )

So I failed to swop the ID for the new SiteTree_versions.ID and no other method works. I was being a bit optimistic ( and desperate) there. I need a way of recreating this query from scratch. This is the $child that's being rendered in forTemplate()

Some more experimental changes

LeftandMain.php L724

$titleFn = function(&$child) use(&$controller, &$recordController, $childrenMethod) {

    $useID = ($childrenMethod == 'AllHistoricalChildren')? $child->RecordID :$child->ID;
$link = Controller::join_links($recordController->Link("show"), $useID);
return LeftAndMain_TreeNode::create($child, $link, $controller->isCurrentPage($child), $childrenMethod)->forTemplate();

Notice I added $childrenMethod as an arg and switched the ID. I also added $childrenMethod as a call to forTemplate().

Also in LeftandMain.php but this time treenode

public function __construct($obj, $link = null, $isCurrent = false, $childrenMethod = null) {
$this->obj = $obj;
$this->link = $link;
$this->isCurrent = $isCurrent;
$this->childrenMethod = $childrenMethod;
}

public function forTemplate() {
$obj = $this->obj;
$useID = ($this->childrenMethod == 'AllHistoricalChildren')? $obj->RecordID :$obj->ID;
return "<li id="record-$useID" data-id="$useID" data-pagetype="$obj->ClassName" class=""

The snag is that although this is on the right track, none of it works because this notion of ID is everywhere, save() and heaven knows what besides. The real problem starts with the way the table xxx_versions is used with its ID instead of Record ID. Perhaps the temp table was a good idea after all!

Contributor

tazzydemon commented Nov 7, 2012

It dawns on me that perhaps the simplest solution might be the best. Why not just move deleted pages to a SiteTree_deleted table (with a correct ID of course!) With or without versioning. Is versioning required on a deleted page? Arguable. This way the working tables would be purged as well.

Contributor

tractorcow commented Dec 4, 2012

Hi Tazzy, I've moved the discussion for this issue to the ticket raised at http://open.silverstripe.org/ticket/7975

Contributor

tazzydemon commented Dec 19, 2012

Thanks Damian for all your work. I was a bit off deck as my ticker went bonkers (stress?) and I had to have a load of scans to see what was going on. Silverstripe was not quite so compelling during that.

Julian

From: Damian Mooyman [mailto:notifications@github.com]
Sent: Wednesday, 5 December 2012 11:55 a.m.
To: silverstripe/sapphire
Cc: tazzydemon
Subject: Re: [sapphire] FIXED: Crashed caused by viewing versioned page (#716)

Hi Tazzy, I've moved the discussion for this issue to the ticket raised at http://open.silverstripe.org/ticket/7975


Reply to this email directly or view it on GitHub silverstripe#716 (comment) .

https://github.com/notifications/beacon/3BkCoYavAxLGgVu7Na1HEa2MwudzZI1uCSRUMjbnLkLv-PtRvquJ8xgzw1obOfjj.gif

Contributor

tractorcow commented Dec 20, 2012

Hi Julian, I hope you're feeling better, and I trust that your health is well. Don't let the scary code ruin your Christmas! :)

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