PostgreSQL: Fix tests in node test group [#2356987]

Comment	File	Size	Author
#9	node_group_tests-4.txt	7.2 KB	jaredsmith
#7	node_group_tests-2.txt	6.15 KB	jaredsmith
#4	2356987-2-fix_database_exception.patch	882 bytes	jaredsmith

#1	node_group_tests.txt	562.5 KB	bzrudi71

Comment #1

bzrudi71 CreditAttribution: bzrudi71 commented 16 October 2014 at 08:46

Issue summary:

File	Size
node_group_tests.txt	562.5 KB

And again some failing tests because of revsions

Log in or register to post comments

Comment #2

mradcliffe

he/him

English

CreditAttribution: mradcliffe commented 22 November 2014 at 20:42

node/1/revision/1/delete when the default revision is 2. isDefaultRevision() returns true despite vid = 1 on the node object being checked. This returns access denied.

Looking deeper into node revision handling and mapFromStorageRecords...

The records query result has a property "isdefaultrevision" which case sensitively is never mentioned in core. It's "isDefaultRevision" property that defaults to TRUE. An expression is added to the query that sets this property to false based on a Sql conditional (When statement). There's an issue that's related to case sensitivity for PostgreSQL: #1600670: Cannot query Postgres database that has column names with capital letters

core//lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:

      $query->addExpression('CASE base.' . $this->revisionKey . ' WHEN revision.' . $this->revisionKey . ' THEN 1 ELSE 0 END', 'isDefaultRevision');

As an aside, this line seems to be very unfriendly to non-sql drivers in NodeRevisionDeleteForm:

    if ($this->connection->query('SELECT COUNT(DISTINCT vid) FROM {node_field_revision} WHERE nid = :nid', array(':nid' => $this->revision->id()))->fetchField() > 1) {

Log in or register to post comments

Comment #3

bzrudi71 CreditAttribution: bzrudi71 commented 24 November 2014 at 17:05

Patch from #1600670: Cannot query Postgres database that has column names with capital letters fixed the revision tests, but opens some others because of double quoting. See testbot results in the mentioned issue.
(Hope to get over my terrible cold within the next days...)

Log in or register to post comments

Comment #4

jaredsmith CreditAttribution: jaredsmith commented 13 February 2015 at 17:52

Status:

Active

» Needs review

File	Size
2356987-2-fix_database_exception.patch	882 bytes

I realize that this doesn't fix all of the failing tests, but this patch does fix one exception with a failed SQL query causing an exception in NodeTranslationUITest. I'm adding this so that I don't lose it before I get a chance to tackle more exceptions and failed tests.

In PostgreSQL, you can't use an alias to refer to a column in a HAVING clause.

Log in or register to post comments

Comment #5

jaredsmith CreditAttribution: jaredsmith commented 13 February 2015 at 17:52

Issue tags:

+LatinAmerica2015

Log in or register to post comments

Comment #6

bzrudi71 CreditAttribution: bzrudi71 commented 13 February 2015 at 20:11

Great catch! Can you please try a testbot run with your patch and #1600670: Cannot query Postgres database that has column names with capital letters combined? Guess we should get near to 100% pass with both patches applied!?
E.g.
DCI_PATCH='https://www.drupal.org/files/issues/drupal-1600670-postgres-capital-letters-70.patch,.;https://www.drupal.org/files/issues/2356987-2-fix_database_exception.patch,.'

Log in or register to post comments

Comment #7

jaredsmith CreditAttribution: jaredsmith commented 16 February 2015 at 13:09

File	Size
node_group_tests-2.txt	6.15 KB

I re-ran my tests using the drupalci support for PostgreSQL using the following settings, including the two patches mentioned in comment #6.

DCI_PHPVERSION=5.5
DCI_DRUPALBRANCH=8.0.x
DCI_TESTGROUPS=node
DCI_UPDATEREPO=TRUE
DCI_DBTYPE=pgsql
DCI_DBVER=9.1
DCI_PATCH='https://www.drupal.org/files/issues/drupal-1600670-postgres-capital-lett...,.'

Unfortunately, we still have two failing tests (Drupal\node\Tests\NodeCreationTest and Drupal\node\Tests\NodeTypeRenameConfigImportTest), and a fatal error (Base lambda function for closure not found in /var/www/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php on line 387).

The complete listing of tests is in the attached text file.

Log in or register to post comments

Comment #8

mradcliffe

he/him

English

CreditAttribution: mradcliffe commented 16 February 2015 at 13:49

The NodeCreationTest and NodeTypeRenameConfigImportTest fails may just be exposed with the capital letters patch.

I haven't seen the SqlContentEntityStorage issue yet. I wonder if that fails for MySQL as well?

Log in or register to post comments

Comment #9

jaredsmith CreditAttribution: jaredsmith commented 19 February 2015 at 03:57

File	Size
node_group_tests-4.txt	7.2 KB

The NodeCreationTest and NodeTypeRenameConfigImportTest fails may just be exposed with the capital letters patch.

No, these tests fail with or without the capital letters patch. There must be something else going on here.

In the case of the NodeCreationTest problem, the failure notice from the test is:
Rollback explanatory error logged to watchdog.
From what I've found, a database exception isn't being properly logged to the watchdog table as it should.

In the case of the NodeTypeRenameConfigImportTest problem, the failure notice from the test is:

Value array (
  0 =&amp;gt; &amp;#039;node.type.ualzsvyr::node.type.wp8dwzyjqx6lhk6l&amp;#039;,
  1 =&amp;gt; &amp;#039;core.base_field_override.node.ualzsvyr.status::core.base_field_override.node.wp8dwzyjqx6lhk6l.status&amp;#039;,
  2 =&amp;gt; &amp;#039;core.entity_form_display.node.ualzsvyr.default::core.entity_form_display.node.wp8dwzyjqx6lhk6l.default&amp;#039;,
  3 =&amp;gt; &amp;#039;core.entity_view_display.node.ualzsvyr.default::core.entity_view_display.node.wp8dwzyjqx6lhk6l.default&amp;#039;,
  4 =&amp;gt; &amp;#039;core.entity_view_display.node.ualzsvyr.teaser::core.entity_view_display.node.wp8dwzyjqx6lhk6l.teaser&amp;#039;,
  5 =&amp;gt; &amp;#039;field.field.node.ualzsvyr.body::field.field.node.wp8dwzyjqx6lhk6l.body&amp;#039;,
) is identical to value array (
  0 =&amp;gt; &amp;#039;node.type.ualzsvyr::node.type.wp8dwzyjqx6lhk6l&amp;#039;,
  1 =&amp;gt; &amp;#039;core.entity_view_display.node.ualzsvyr.default::core.entity_view_display.node.wp8dwzyjqx6lhk6l.default&amp;#039;,
  2 =&amp;gt; &amp;#039;core.entity_view_display.node.ualzsvyr.teaser::core.entity_view_display.node.wp8dwzyjqx6lhk6l.teaser&amp;#039;,
  3 =&amp;gt; &amp;#039;core.entity_form_display.node.ualzsvyr.default::core.entity_form_display.node.wp8dwzyjqx6lhk6l.default&amp;#039;,
  4 =&amp;gt; &amp;#039;core.base_field_override.node.ualzsvyr.status::core.base_field_override.node.wp8dwzyjqx6lhk6l.status&amp;#039;,
  5 =&amp;gt; &amp;#039;field.field.node.ualzsvyr.body::field.field.node.wp8dwzyjqx6lhk6l.body&amp;#039;,

I haven't yet begun to track down the cause of this failure.

I haven't seen the SqlContentEntityStorage issue yet. I wonder if that fails for MySQL as well?

I can't reproduce this issue today, so it was probably something broken in head that has since been fixed, or some sort of intermittent issue. I'm not going to to worry about it unless it comes back to haunt me.

I'm attaching the latest results from the tests with the capital letters patch and the database exception patch (from comment 4).

Log in or register to post comments

Comment #10

mradcliffe

he/him

English

CreditAttribution: mradcliffe commented 19 February 2015 at 04:51

Hmm, that fail looks like a sorting issue for config entities. The schema is the same, but the keys are re-ordered in the comparison.

Log in or register to post comments

Comment #11

bzrudi71 CreditAttribution: bzrudi71 commented 19 February 2015 at 09:40

Yes, for the NodeTypeRenameConfigImportTest it looks like we hit a missing order by again. I'm more worried about NodeCreationTest. I verified that the actual value is logged in to watchdog, but the query does a select from bytea and this may require that we need to think about #2238253: Add bindValue to a PDO::PARAM_* type in database query arguments :-(
But this needs investigation, just speculation from my short look at the test...

Log in or register to post comments

Comment #12

bzrudi71 CreditAttribution: bzrudi71 commented 19 February 2015 at 14:04

Hmm, see Connection.php ;-)

  public function prepareQuery($query) {
    // mapConditionOperator converts LIKE operations to ILIKE for consistency
    // with MySQL. However, Postgres does not support ILIKE on bytea (blobs)
    // fields.
    // To make the ILIKE operator work, we type-cast bytea fields into text.
    // @todo This workaround only affects bytea fields, but the involved field
    //   types involved in the query are unknown, so there is no way to
    //   conditionally execute this for affected queries only.
    return parent::prepareQuery(preg_replace('/ ([^ ]+) +(I*LIKE|NOT +I*LIKE) /i', ' ${1}::text ${2} ', $query));
  }

Log in or register to post comments

Comment #13

jaredsmith CreditAttribution: jaredsmith commented 19 February 2015 at 15:00

@bzrudi71, can you please give more detail on the INSERT query being used to write the information to the watchdog table, as well as the SELECT statement used to retrieve it?

Log in or register to post comments

Comment #14

bzrudi71 CreditAttribution: bzrudi71 commented 20 February 2015 at 08:37

@jaredsmith: I just did a quick additional query for all variables in watchdog:

$data = db_query("SELECT variables FROM {watchdog}")->fetchAll();
debug($data, 'Variables', true);

This shows that the watchdog message (Test exception for rollback) is logged:

Variables:
Array
(
    [0] => stdClass Object
        (
            [variables] => a:1:{s:7:"%module";s:5:"dblog";}
        )

    [1] => stdClass Object
        (
            [variables] => a:1:{s:7:"%module";s:14:"test_page_test";}
        )

    [2] => stdClass Object
        (
            [variables] => a:1:{s:5:"%name";s:8:"uJ94DDvL";}
        )

    [3] => stdClass Object
        (
            [variables] => a:5:{s:5:"%type";s:9:"Exception";s:8:"!message";s:28:"Test exception for rollback.";s:9:"%function";s:33:"node_test_exception_node_insert()";s:5:"%file";s:112:"/opt/local/apache2/htdocs/drupal8/core/modules/node/tests/modules/node_test_exception/node_test_exception.module";s:5:"%line";i:15;}
        )

)

So it's not a problem with the INSERT...
BTW: I'm offline until next Wednesday!