getConstrainedRelatedId method not returning ids as string if key is a numeric string

[danielsouzaaf]

• Laravel-mongodb Version: 3.6.6
• PHP Version: 7.3.18
• Database Driver & Version: mongodb/mongodb 1.6.1

Description:

When using a whereHas filter, if the field under the whereHas clause is a string with a numeric value, getConstrainedRelatedIds on the QueriesRelationships trait is returning the keys as numeric instead of string. With that, if the column in the where clause is a string, it fails.

Steps to reproduce

1. Create a parent register with a _id PK as string and another field for testing purposes, e.g: active: true.
2. Create two child associated with the parent register
3. Created Example:

Parent: {
      _id: "12345",
      "active": true
},

Child: {
   _id:"id1",
  "parent_id": "12345"
},
Child: {
  _id: "id2,
  "parent_id": "12345"
}

4. Run the query:

Child::whereHas('parent', function ($query) {
       return $query->where('active', true);
       })

Expected behaviour

It should return the two childs

Actual behaviour

None child is returned.

Logs:

Insert log.txt here (if necessary)

[danielsouzaaf]

I could get it working by doing this fix:

$relationCount = array_count_values(array_map(function ($id) {
            return (string) '0' . $id; // Convert Back ObjectIds to Strings
        }, is_array($relations) ? $relations : $relations->flatten()->toArray()));

And later,

// All related ids.
        return array_map(function ($key) {
            return substr($key, 1);
        }, array_keys($relationCount));

On the getConstrainedRelatedIds method. However, it does not seem like the better one

[Smolevich]

What is the root cause to create document with string _id, but not ObjectId ?

[stephandesouza]

Our projects use internally nanoid to generate IDs because mongodb was duplicating ObjectIds, so we can prevent this kind of dup. One of them is running 2~3 years, and this week we got our first only numeric hash, as example "2791684". To prevent this we already fixed our nanoid generator with invalidation only numeric hash, but opened this warning on the package!

My PR to fix stopped on hybrid relations, the same happens with @danielsouzaaf solution, because of MySQL integer id on mongo document.

[divine]

I vote no for this custom fix which might just slow down for everyone. That's it.

Custom code for only fixing this in your projects?

Thanks!

[stephandesouza]

Is not only for our project, but is a bug that could happen on others with numeric keys when using ‘has’ methood.

My second commit fixes the bug with hybrid relations.

[divine]

Is not only for our project, but is a bug that could happen on others with numeric keys when using ‘has’ methood.

My second commit fixes the bug with hybrid relations.

There are bugs that are exactly created by this type of custom code. Fix over on a fix. What if I have geo type _id? Does your code fix it? No.

If its going to be fixed then a global fix should be created for casted attributes as outlined here #1974 (comment).

Thanks!

[stephandesouza]

Aye, I understand your point but working with CastsAttributes will make it a BC with tag 3.6, as it works with Laravel < 7, no? And Laravel 6 is currently on LTS. :(

And my point here is with relation keys, I do not see a usage of geotype as PK/FK. I'm right?

[divine]

Aye, I understand your point but working with CastsAttributes will make it a BC with tag 3.6, as it works with Laravel < 7, no? And Laravel 6 is currently on LTS. :(

And my point here is with relation keys, I do not see a usage of geotype as PK/FK. I'm right?

Sorry, missed that this is the really old version. Your PR was pushed to master which is 3.8.

I still don't have a strong positive opinion about merging that PR. I can understand you need to fix this but also don't forget there are a lot of people using it. Making such changes for fixing a little bit of question that it might create some bugs later.

Let me ask kindly @Smolevich for his thoughts.

Thanks!

[stephandesouza]

No problem, we're here to find the best solution for everyone!

I created to PR to master, because I see you backporting PRs to older versions as well.

[divine]

No problem, we're here to find the best solution for everyone!

I created to PR to master, because I see you backporting PRs to older versions as well.

Yup, not everyone could upgrade to the next Laravel version in one click. It takes time and there is no reason why fixes should be in a newer version only.

Thanks!

[trip-somers]

VERSION: 3.8.5

We have noticed the exact same issue. Our _id values for our Organization models come from an external system, and whereHas() is indeed broken for us when the value is an integer string. After discovering this issue and finding the exact cause of the problem, we were able to work around it by reversing the approach to querying the relationship.

Previous attempt, broken:

// App\Models\User.php
public function getOrganizationsAttribute() {
    return Organization::whereHas('userRoles', function ($userRoles) {
        $userRoles->where('user_id', $this->id);
    })->get()->sortBy('name');
}

New attempt, functional:

// App\Models\User.php
public function getOrganizationsAttribute() {
    return $this->userRoles->pluck('organization')->unique()->sortBy('name');
}

Essentially, don't use whereHas() if the model under query has integer strings as IDs.