laravel-mongodb icon indicating copy to clipboard operation
laravel-mongodb copied to clipboard

Allow `has` method with numeric indexes

Open stephandesouza opened this issue 3 years ago • 6 comments

Fix #2182

stephandesouza avatar Jan 15 '21 13:01 stephandesouza

Codecov Report

Merging #2187 (e362198) into master (09fcda8) will increase coverage by 0.05%. The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2187      +/-   ##
============================================
+ Coverage     86.93%   86.99%   +0.05%     
- Complexity      663      666       +3     
============================================
  Files            33       33              
  Lines          1554     1561       +7     
============================================
+ Hits           1351     1358       +7     
  Misses          203      203              
Impacted Files Coverage Δ Complexity Δ
...enssegers/Mongodb/Helpers/QueriesRelationships.php 93.93% <100.00%> (+0.71%) 31.00 <0.00> (+3.00)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 09fcda8...e362198. Read the comment docs.

codecov-io avatar Feb 25 '21 06:02 codecov-io

@stephandesouza taking look at this PR it can create some regression like this one https://github.com/jenssegers/laravel-mongodb/issues/2203

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

What solution we could create? Any thoughts?

Thanks!

divine avatar May 07 '21 10:05 divine

This workaround does not modify the query used by whereHas(), only how the IDs are returned by getConstrainedRelatedIds.

And, for now, we can't run away from array_count_values and array_keys. But we can avoid using array_map when no numeric ID is used, with a "hasNumericIndex" variable.

stephandesouza avatar May 07 '21 11:05 stephandesouza

Codecov Report

Merging #2187 (428eacd) into master (4056a73) will increase coverage by 0.09%. The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2187      +/-   ##
============================================
+ Coverage     86.91%   87.00%   +0.09%     
- Complexity      665      669       +4     
============================================
  Files            33       33              
  Lines          1559     1570      +11     
============================================
+ Hits           1355     1366      +11     
  Misses          204      204              
Impacted Files Coverage Δ Complexity Δ
src/Helpers/QueriesRelationships.php 94.28% <100.00%> (+1.06%) 32.00 <0.00> (+4.00)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4056a73...428eacd. Read the comment docs.

codecov-commenter avatar May 12 '21 18:05 codecov-commenter

Sorry, the test was wrong after suggested review.. This is a problem with the "reverse" foreign key used on this method, and also happens with primaryKey as int

Situation: Author "id:1" has two books Book "id:A", and book "id:B". Author "id:'2'" has two books Book "id:C"

Today: getConstrainedRelatedId() returns

["1", "2"]

Fixes: getConstrainedRelatedId() returns

[1, 2]

And yes, even a "string integer" needs to be converted to int,

stephandesouza avatar May 12 '21 19:05 stephandesouza

Hello,

I'll take a look tomorrow with a blank project to understand better this issue.

Thanks!

divine avatar May 14 '21 20:05 divine