search

mongodb / laravel-mongodb

A MongoDB based Eloquent model and Query builder for Laravel (Moloquent)
https://www.mongodb.com/compatibility/mongodb-laravel-integration
MIT License
6.99k stars 1.42k forks source link

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

Open danielsouzaaf opened 3 years ago

danielsouzaaf commented 3 years ago

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.

<details><summary><b>Logs</b>:</summary>
Insert log.txt here (if necessary)
</details>
danielsouzaaf commented 3 years ago

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 commented 3 years ago

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

stephandesouza commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/jenssegers/laravel-mongodb/issues/1974#issuecomment-592857370.

Thanks!

stephandesouza commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 1 year ago

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.