Let's assume we have simple directional unique relation type 'user -> user' and 2 users uid 1 and uid 2.

1. create a relation 1 from uid 1 to uid 2
2. try to create relation 2 from uid 1 to uid 1

The relation insert will fail, direction is not enforced in field validation and because of that relation_relation_exists() seems to match both endpoints to the same endpoint of relation 1.

This might cause data loss if relation_relation_exists() is used to retrieve relations to be deleted!

CommentFileSizeAuthor
#18 relation_self_to_self_validation-1760026-18.patch900 bytesmikran
#15 relation-self-to-self-validation-1760026-15-D7.patch767 bytesadamwhite
#14 join_table_limit_exceeded-1970156-15.patch765 bytesmikran
#4 ensure_distinct_endpoints-1760026-4.patch2.75 KBmikran
#3 ensure_distinct_endpoints-1760026-3.patch2.8 KBmikran
#1 tests_only-1760026-1.patch1015 bytesmikran

Comments

mikran’s picture

mikran commented
Status: Active » Needs review
StatusFileSize
new tests_only-1760026-1.patch1015 bytes

Tests only

Status: Needs review » Needs work

The last submitted patch, tests_only-1760026-1.patch, failed testing.

mikran’s picture

mikran commented
Status: Needs work » Needs review
StatusFileSize
new ensure_distinct_endpoints-1760026-3.patch2.8 KB

Here is one attempt to try to solve this. Maybe there is smarter ways? Review please

mikran’s picture

mikran commented
StatusFileSize
new ensure_distinct_endpoints-1760026-4.patch2.75 KB

-dpm

mikran’s picture

mikran commented

#4: ensure_distinct_endpoints-1760026-4.patch queued for re-testing.

mikran’s picture

mikran commented
Status: Needs review » Fixed

committed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

dpi’s picture

dpi
Location Perth, Australia
commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Closed (fixed) » Active

This issue has regressed for Drupal 8.

I'm not sure I have the patience to interpret and fix what is going on here. :@

dpi’s picture

dpi
Location Perth, Australia
commented

Related: #1859084: Conditions on multi-column fields in EntityFieldQuery

mikran’s picture

mikran commented
Assigned: Unassigned » mikran

I was not happy with the implementation of d7 fix so maybe I'll take a look at this

dpi’s picture

dpi
Location Perth, Australia
commented
Issue summary: View changes

I have fixed the current implementation of relation_query_enforce_distinct_endpoints_alter(). Tests pass again.

I would like to get your (mikran), chx, and others input on whether there is a better way.

If chx could clarify what this means for us in regards to comment #29 on #1859084: Conditions on multi-column fields in EntityFieldQuery, since tables are still getting multiple joins per field condition group.

Jorrit’s picture

Jorrit commented

With regards to issue #1859084 I notice that applying that patch and using the current Relation module causes an SQL error related to field_data_endpoints0.endpoints_r_index being an invalid column. The hook relation_query_enforce_distinct_endpoints_alter makes very specific assumptions about the table alias names which are different when using the performance patch.

It seems

      $column_left = 'field_data_endpoints_delta_' . $i . '.endpoints_r_index';
      $column_right = 'field_data_endpoints_delta_' . $k . '.endpoints_r_index';
      $query->where("$column_left != $column_right");

fixes this problem. I hope someone can confirm this, so a patch can be made that can be used in drush make.

mikran’s picture

mikran commented

I hope someone can confirm this, so a patch can be made that can be used in drush make.

Yes that is correct. I haven't personally tested this one apart from quick tests in other issue where max join limit was reached.

mikran’s picture

mikran commented
StatusFileSize
new join_table_limit_exceeded-1970156-15.patch765 bytes

#1970156: MySQL can only use 61 tables in a join: field_data_endpoints on node_save closed as the fix there is exactly same as here.

so here is a patch by burningdog

file name is missleading but other issue also links back here so I guess its ok

adamwhite’s picture

adamwhite commented
StatusFileSize
new relation-self-to-self-validation-1760026-15-D7.patch767 bytes

The patch in #14 didn't work for me. The table names were missing the underscore after the delta.

Changing it to work like the attached patch (rolled against 7.x-1.x dev) solved the problem for me.

mikran’s picture

mikran commented
Version: 8.x-1.x-dev » 7.x-1.x-dev

#1859084: Conditions on multi-column fields in EntityFieldQuery is about to get committed to D7 so reacting on that is more urgent now. Relation patch will be trivial but it also requires new release as soon as next core is out.

mikran’s picture

mikran commented
Status: Active » Needs work 
+++ b/relation.module
@@ -566,8 +566,8 @@ function relation_query_enforce_distinct_endpoints_alter(QueryAlterableInterface
       // @see http://drupal.org/node/1859084
       // For now each endpoint condition adds 2 joins to the query. Add
       // conditions between even numbers.

This comment is not needed anymore

mikran’s picture

mikran commented
Assigned: mikran » Unassigned
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new relation_self_to_self_validation-1760026-18.patch900 bytes

Next core release window is less than two weeks away now, so I'm preparing this issue to be ready to be committed then.

mikran queued 18: relation_self_to_self_validation-1760026-18.patch for re-testing.

mikran’s picture

mikran commented
Status: Reviewed & tested by the community » Fixed

I'm going to commit this now, tests are failing because of this. So whoever is running -dev of Relation also needs -dev of Drupal core until next core is released.

  • mikran committed 2ca9fe2 on 7.x-1.x 
    Issue #1760026 by Jorrit, burningdog, adamwhite | mikran: Fixed...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Jorrit’s picture

Jorrit commented

Drupal core 7.33 is released with issue #1859084. I think a new release of Relation is in order, such that people who update to the latest Drupal can also update to the latest Relation module.

mikran’s picture

mikran commented

Yep, rc6 release node is created. That'll take a while to get packaged.