Returned thread may exclude some users

TBH, I don't fully understand that either. I suppose it is to get to a model similar to Whatsapp and such apps where you have a unique conversation per other user or group of users. I am more familiar with PM models where it is more like email, where you create a message with a subject and might have multiple such threads, with the same or different users. Do note that I think a subject will be mandatory, in that case.

[I did understand the logic, but what I think is happening is that the logic fails to check if *all* users are part of the found conversation.]

WRT to the actual issue, I'll probably be doing a deeper dive this week to try and understand it better. Will try to get some better steps to reproduce too.

I did a deeper dive. Although the code indeed claims to find all threads where all given users are present, in fact it returns all threads any of the users are involved in*. It then proceeds to filter out threads that have other users too, but does so under the assumption that the threads the earlier query turned up at least has all of the users we are looking for. This is a wrong assumption. To make it easier to follow, this is the relevant code:

/**
   * {@inheritdoc}
   */
  public function getThreadIdForMembers(array $members) {
    $uids = [];
    foreach ($members as $member) {
      $uids[$member->id()] = $member->id();
    }

    // Select threads common for the given members.
    $threads_ids = $this->database->select('private_message_thread__members', 'pmt')
      ->fields('pmt', ['entity_id'])
      ->condition('members_target_id', $uids, 'IN')
      ->groupBy('entity_id')
      ->execute()
      ->fetchCol();
    // Exclude threads with other participants.
    foreach ($threads_ids as $thread_id) {
      $query = $this->database->select('private_message_thread__members', 'pmt')
        ->condition('members_target_id', $uids, 'NOT IN')
        ->condition('entity_id', $thread_id);
      if ($query->countQuery()->execute()->fetchField() == 0) {
        return $thread_id;
      }
    }
    return FALSE;
  }

I think the mistake is basically in the line

->condition('members_target_id', $uids, 'IN')

This effectively serves as an OR, not an AND, for all the user IDs in the $uids array. In many cases, this problem is then fixed by actually filtering out the threads with other users, but it may still turn up threads that do not have all users we are looking for.

I will push a fix shortly, that solves my specific test case. I'll also try and come up with a test that actually demonstrates the problem.

*: In my case, this is nicely demonstrated in the debugger, where the query comes up with dozens of threads; one of the users is a community manager who has threads with a lot of other users; just one with the second user involved in the lookup, though. Because there happens to be also a "corrupt" thread where she is the only member, and that thread happens to be hit on first in the filtering loop, that is what the method produces instead of the actual common thread between the two users involved.

Adding steps to reproduce. Note that this bug only exists in the current dev version, not in beta16.

Branch tests-only contains a test that basically replicates the steps to reproduce. Unfortunately, the existing thread members test surfaced that my first stab at the solution doesn't cut it either.

Basically, I now reversed the problem (the query is now basically doing what was inteded before, so it still needs the filtering to not find threads that have extra members). I'll restore the "filter" code.

See my branch, I think with the filtering code restored it will be a somewhat efficient solution. I'll create an MR to make it more visible.

Your patch looks OK too. I guess the test I wrote in https://git.drupalcode.org/project/private_message/-/merge_requests/9 should also work for it (you could create a branch for your patch and merge in the tests-only branch, that should let the test verify whether your solution is valid).

I think the groupBy might still be relevant, as depending on the number of users the query would now return duplicates for common threads. Question is what has more overhead. My solution (in MR 8) is more SQL-centric, but the basis question remains the same; more basic SQL vs. more processing in PHP.

Setting to Needs review. Quick summary:

• MR 8 holds my solution, which is based on subqueries. It includes the test from...
• MR 9, contains just an extra test, demonstrating the issue
• Patch #9, would need combining with the test to make sure it is valid. Makes the query less complicated, at the cost of more PHP processing. Could possibly be improved by re-introducing the groupBy on the thread ID, although it is hard to say whether that would slow the query down further than it improves the PHP processing speed because it would need to check less results