Skip to content

Fix: sortable for ambiguous columns by qualifying them#725

Merged
binaryk merged 2 commits intoBinarCode:10.xfrom
george-todica:fix-sortable-qualified-column
Apr 1, 2026
Merged

Fix: sortable for ambiguous columns by qualifying them#725
binaryk merged 2 commits intoBinarCode:10.xfrom
george-todica:fix-sortable-qualified-column

Conversation

@george-todica
Copy link
Copy Markdown
Contributor

The Problem

When a Restify repository has searchable BelongsTo relations, the RepositorySearchService adds LEFT JOINs:

SELECT posts.* FROM posts
LEFT JOIN users ON posts.user_id = users.id    -- added by searchable BelongsTo
WHERE UPPER(users.name) LIKE '%john%'
ORDER BY id ASC                                 -- AMBIGUOUS! Both posts.id and users.id exist

MySQL throws: Column 'id' in order clause is ambiguous

The Fix: Two changes in Restify

1. SortCollection::qualifyColumns(Model $model) — mirrors the existing SearchablesCollection::qualifyColumns()

Iterates sort filters and prefixes unqualified columns with the model's table name:

  • idposts.id
  • created_atposts.created_at
  • users.name → left as-is (already has a dot)

2. RepositorySearchService::prepareOrders() — calls qualifyColumns() before apply()

This is the only place where sorts hit the database. The call happens AFTER collectSorts() (which does normalization, hydration, authorization) and BEFORE apply() (which executes the ORDER BY).

Why NOT other locations?

Why not fix in SortableFilter::filter() at line 61?

$query->orderBy($this->column, $value);  // Could qualify here instead

This would avoid mutating $this->column, but:

  • It's inconsistent with how searchables work (they qualify at the collection level)
  • Other code paths reading $this->column during apply() would still get unqualified names
  • The searchables pattern (SearchablesCollection::qualifyColumns()) is already proven and understood in this codebase

Why not fix in InteractWithSearch::collectSorts()?

$requestSorts = (new SortCollection(...))
    ->normalize()
    ->hydrateDefinition(...)  // sets closures/relations
    ->authorized(...)
    ->inRepository(...)       // compares columns against repo sorts
    ->hydrateRepository(...);
// Could add ->qualifyColumns() here?

No, because inRepository() at line 57 compares $filter->column against repository-defined sort keys:

$collection->contains('column', '=', $filter->column)

Repository sort keys are unqualified ('id', 'date'). If we qualify before this step, posts.id !== id and the sort gets filtered out.

prepareOrders() is the correct location — after the full collectSorts() pipeline resolves everything, but before apply() hits the database.

What about Serializer, NaturalSortFilter, closures?

Serializer (sortBy($this->sort->column())) — creates its OWN SortableFilter via sortAsc()/sortDesc(). Completely separate pipeline, never goes through qualifyColumns(). Not affected.

NaturalSortFilter — receives $this->column() (now qualified) in raw SQL:

$query->orderByRaw("CAST({$column} AS UNSIGNED) {$value}, {$column} {$value}");
// CAST(posts.name AS UNSIGNED) — valid SQL in both MySQL and SQLite

Actually beneficial — prevents the same ambiguity in raw SQL.

Closure/invokable sorts — also receive the qualified column. Example:

public function __invoke($request, $query, $value, string $column): void
{
    $query->orderBy($column, $order);  // $column is now 'posts.name' — better!
}

This is a net improvement — closures that use orderBy directly would have hit the same ambiguity bug without qualification.

All 413 Restify tests pass, confirming no breakages.

@what-the-diff
Copy link
Copy Markdown

what-the-diff bot commented Mar 31, 2026

PR Summary

  • Improvement of Data Sorting
    A new functionality has been added to the code which improves the way we classify columns in a database using a provided model. This ensures greater accuracy in data classification.

  • Optimized Data Filtering
    The method that prepares orders of data within our repository has been adjusted to use the column qualifying method beforehand. This results in more precise data filtering based on the provided model.

  • Testing Enhancements for Sortable Filters
    To assure the correct functioning of these new changes, we have introduced a new test procedure. This test is designed to ensure that the column sorting function works as intended, particularly when data from different sources are brought together. This minimizes ambiguity and leads to more accurate data handling.

  • Introduction of Additional Dependents
    In order to support these new tests, we've folded in a few new classes into the testing procedure. This should ensure that the tests are comprehensive and truly reflective of the software's capabilities.

@arthurkirkosa arthurkirkosa requested a review from binaryk March 31, 2026 12:06
@binaryk binaryk merged commit a4499a6 into BinarCode:10.x Apr 1, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants