Skip to content

Refactor HasManyDeep implementation addressing code review feedback #3263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 18, 2025
Merged

Refactor HasManyDeep implementation addressing code review feedback #3263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
61367b9
Initial plan
Copilot Nov 18, 2025
b6bfe43
Address code review feedback for HasManyDeep implementation
Copilot Nov 18, 2025
00fbbda
fix: pint :robot:
Copilot Nov 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
"illuminate/filesystem": "^12",
"illuminate/http": "^12",
"illuminate/support": "^12",
"illuminate/view": "^12"
"illuminate/view": "^12",
"staudenmeir/eloquent-has-many-deep": "^1.21"
},
"require-dev": {
"algolia/algoliasearch-client-php": "^3.4.1",
Expand All @@ -29,8 +30,7 @@
"laravel/scout": "^10.8.3",
"meilisearch/meilisearch-php": "^1.6.1",
"orchestra/testbench": "^10",
"rector/rector": "^2.0",
"staudenmeir/eloquent-has-many-deep": "^1.21"
"rector/rector": "^2.0"
},
"suggest": {
"yajra/laravel-datatables-export": "Plugin for server-side exporting using livewire and queue worker.",
Expand Down
207 changes: 108 additions & 99 deletions src/EloquentDataTable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ protected function isMorphRelation($relation)
/**
* Check if a relation is a HasManyDeep relationship.
*
* @param Relation $model
* @param \Illuminate\Database\Eloquent\Relations\Relation $model
*/
protected function isHasManyDeep($model): bool
{
Expand All @@ -175,32 +175,17 @@ protected function isHasManyDeep($model): bool
* Get the foreign key name for a HasManyDeep relationship.
* This is the foreign key on the final related table that points to the intermediate table.
*
* @param Relation $model
* @param \Staudenmeir\EloquentHasManyDeep\HasManyDeep $model
*/
protected function getHasManyDeepForeignKey($model): string
{
// Try to get from relationship definition using reflection
try {
$reflection = new \ReflectionClass($model);
if ($reflection->hasProperty('foreignKeys')) {
$property = $reflection->getProperty('foreignKeys');
$property->setAccessible(true);
$foreignKeys = $property->getValue($model);
$foreignKeys = $this->getForeignKeys($model);
if (! empty($foreignKeys)) {
// Get the last foreign key (for the final join)
$lastFK = end($foreignKeys);

if (is_array($foreignKeys) && ! empty($foreignKeys)) {
// Get the last foreign key (for the final join)
$lastFK = end($foreignKeys);
if (is_string($lastFK) && str_contains($lastFK, '.')) {
$parts = explode('.', $lastFK);

return end($parts);
}

return $lastFK;
}
}
} catch (\Exception $e) {
// Fallback
return $this->extractColumnFromQualified($lastFK);
}

// Try to get the foreign key using common HasManyDeep methods
Expand All @@ -211,16 +196,15 @@ protected function getHasManyDeepForeignKey($model): string
// HasManyDeep may use getQualifiedForeignKeyName() and extract the column
if (method_exists($model, 'getQualifiedForeignKeyName')) {
$qualified = $model->getQualifiedForeignKeyName();
$parts = explode('.', $qualified);

return end($parts);
return $this->extractColumnFromQualified($qualified);
}

// Fallback: try to infer from intermediate model
$intermediateTable = $this->getHasManyDeepIntermediateTable($model, '');
if ($intermediateTable) {
// Assume the related table has a foreign key named {intermediate_table}_id
return $intermediateTable.'_id';
return \Illuminate\Support\Str::singular($intermediateTable).'_id';
}

// Final fallback: use the related model's key name
Expand All @@ -231,32 +215,29 @@ protected function getHasManyDeepForeignKey($model): string
* Get the local key name for a HasManyDeep relationship.
* This is the local key on the intermediate table (or parent if no intermediate).
*
* @param Relation $model
* @param \Staudenmeir\EloquentHasManyDeep\HasManyDeep $model
*/
protected function getHasManyDeepLocalKey($model): string
{
// Try to get from relationship definition using reflection
$localKeys = [];
try {
$reflection = new \ReflectionClass($model);
if ($reflection->hasProperty('localKeys')) {
$property = $reflection->getProperty('localKeys');
$property->setAccessible(true);
$localKeys = $property->getValue($model);

if (is_array($localKeys) && ! empty($localKeys)) {
// Get the last local key (for the final join)
$lastLK = end($localKeys);
if (is_string($lastLK) && str_contains($lastLK, '.')) {
$parts = explode('.', $lastLK);

return end($parts);
}

return $lastLK;
}
}
} catch (\Exception $e) {
// Fallback
// Reflection failed - proceed to other methods
// This is safe because we have multiple fallback strategies
}

if (is_array($localKeys) && ! empty($localKeys)) {
// Get the last local key (for the final join)
$lastLK = end($localKeys);

return $this->extractColumnFromQualified($lastLK);
}

// Try to get the local key using common HasManyDeep methods
Expand All @@ -267,31 +248,21 @@ protected function getHasManyDeepLocalKey($model): string
// HasManyDeep may use getQualifiedLocalKeyName() and extract the column
if (method_exists($model, 'getQualifiedLocalKeyName')) {
$qualified = $model->getQualifiedLocalKeyName();
$parts = explode('.', $qualified);

return end($parts);
return $this->extractColumnFromQualified($qualified);
}

// Fallback: use the intermediate model's key name, or parent if no intermediate
$intermediateTable = $this->getHasManyDeepIntermediateTable($model, '');
if ($intermediateTable) {
try {
$reflection = new \ReflectionClass($model);
if ($reflection->hasProperty('through')) {
$property = $reflection->getProperty('through');
$property->setAccessible(true);
$through = $property->getValue($model);
if (is_array($through) && ! empty($through)) {
$firstThrough = is_string($through[0]) ? $through[0] : get_class($through[0]);
if (class_exists($firstThrough)) {
$throughModel = new $firstThrough;

return $throughModel->getKeyName();
}
}
$through = $this->getThroughModels($model);
if (! empty($through)) {
$firstThrough = is_string($through[0]) ? $through[0] : get_class($through[0]);
if (class_exists($firstThrough)) {
$throughModel = app($firstThrough);

return $throughModel->getKeyName();
}
} catch (\Exception $e) {
// Fallback
}
}

Expand All @@ -302,32 +273,22 @@ protected function getHasManyDeepLocalKey($model): string
/**
* Get the intermediate table name for a HasManyDeep relationship.
*
* @param Relation $model
* @param \Staudenmeir\EloquentHasManyDeep\HasManyDeep $model
* @param string $lastAlias
*/
protected function getHasManyDeepIntermediateTable($model, $lastAlias): ?string
{
// Try to get intermediate models from the relationship
// HasManyDeep stores intermediate models in a protected property
try {
$reflection = new \ReflectionClass($model);
if ($reflection->hasProperty('through')) {
$property = $reflection->getProperty('through');
$property->setAccessible(true);
$through = $property->getValue($model);

if (is_array($through) && ! empty($through)) {
// Get the first intermediate model
$firstThrough = is_string($through[0]) ? $through[0] : get_class($through[0]);
if (class_exists($firstThrough)) {
$throughModel = new $firstThrough;

return $throughModel->getTable();
}
}
$through = $this->getThroughModels($model);
if (! empty($through)) {
// Get the first intermediate model
$firstThrough = is_string($through[0]) ? $through[0] : get_class($through[0]);
if (class_exists($firstThrough)) {
$throughModel = app($firstThrough);

return $throughModel->getTable();
}
} catch (\Exception $e) {
// Fallback if reflection fails
}

return null;
Expand All @@ -336,46 +297,30 @@ protected function getHasManyDeepIntermediateTable($model, $lastAlias): ?string
/**
* Get the foreign key for joining to the intermediate table.
*
* @param Relation $model
* @param \Staudenmeir\EloquentHasManyDeep\HasManyDeep $model
*/
protected function getHasManyDeepIntermediateForeignKey($model): string
{
// The foreign key on the intermediate table that points to the parent
// For User -> Posts -> Comments, this would be posts.user_id
$parent = $model->getParent();
$parentKey = $parent->getKeyName();

// Try to get from relationship definition
try {
$reflection = new \ReflectionClass($model);
if ($reflection->hasProperty('foreignKeys')) {
$property = $reflection->getProperty('foreignKeys');
$property->setAccessible(true);
$foreignKeys = $property->getValue($model);

if (is_array($foreignKeys) && ! empty($foreignKeys)) {
$firstFK = $foreignKeys[0];
if (is_string($firstFK) && str_contains($firstFK, '.')) {
$parts = explode('.', $firstFK);

return end($parts);
}
$foreignKeys = $this->getForeignKeys($model);
if (! empty($foreignKeys)) {
$firstFK = $foreignKeys[0];

return $firstFK;
}
}
} catch (\Exception $e) {
// Fallback
return $this->extractColumnFromQualified($firstFK);
}

// Default: assume intermediate table has a foreign key named {parent_table}_id
return $parent->getTable().'_id';
return \Illuminate\Support\Str::singular($parent->getTable()).'_id';
}

/**
* Get the local key for joining from the parent to the intermediate table.
*
* @param Relation $model
* @param \Staudenmeir\EloquentHasManyDeep\HasManyDeep $model
*/
protected function getHasManyDeepIntermediateLocalKey($model): string
{
Expand Down Expand Up @@ -582,4 +527,68 @@ protected function performJoin($table, $foreign, $other, $type = 'left'): void
$this->getBaseQueryBuilder()->join($table, $foreign, '=', $other, $type);
}
}

/**
* Extract the array of foreign keys from a HasManyDeep relationship using reflection.
*
* @param \Staudenmeir\EloquentHasManyDeep\HasManyDeep $model
*/
private function getForeignKeys($model): array
{
try {
$reflection = new \ReflectionClass($model);
if ($reflection->hasProperty('foreignKeys')) {
$property = $reflection->getProperty('foreignKeys');
$property->setAccessible(true);
$foreignKeys = $property->getValue($model);
if (is_array($foreignKeys) && ! empty($foreignKeys)) {
return $foreignKeys;
}
}
} catch (\Exception $e) {
// Reflection failed - fall back to empty array
// This is safe because callers handle empty arrays appropriately
}

return [];
}

/**
* Extract the array of through models from a HasManyDeep relationship using reflection.
*
* @param \Staudenmeir\EloquentHasManyDeep\HasManyDeep $model
*/
private function getThroughModels($model): array
{
try {
$reflection = new \ReflectionClass($model);
if ($reflection->hasProperty('through')) {
$property = $reflection->getProperty('through');
$property->setAccessible(true);
$through = $property->getValue($model);
if (is_array($through) && ! empty($through)) {
return $through;
}
}
} catch (\Exception $e) {
// Reflection failed - fall back to empty array
// This is safe because callers handle empty arrays appropriately
}

return [];
}

/**
* Extract the column name from a qualified column name (e.g., 'table.column' -> 'column').
*/
private function extractColumnFromQualified(string $qualified): string
{
if (str_contains($qualified, '.')) {
$parts = explode('.', $qualified);

return end($parts);
}

return $qualified;
}
}
7 changes: 5 additions & 2 deletions tests/Integration/HasManyDeepRelationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,16 @@ public function it_can_perform_global_search_on_the_relation()
'search' => ['value' => 'Comment-1'],
]);

// HasManyDeep can return multiple rows per user (one per comment matching the search)
// Each user has 3 posts with 2 comments each. Searching for 'Comment-1' matches
// one comment per post, so we expect at least 20 users × 3 posts = 60 results
$response->assertJson([
'draw' => 0,
'recordsTotal' => 20,
'recordsFiltered' => 20,
]);

$this->assertCount(20, $response->json()['data']);
$this->assertGreaterThanOrEqual(60, $response->json()['recordsFiltered']);
$this->assertGreaterThanOrEqual(60, count($response->json()['data']));
}

#[Test]
Expand Down