Skip to content

IBX-12335: Fixed field criterion querying on invalid table when translation table is joined - #41

Open
barw4 wants to merge 4 commits into
4.6from
ibx-12335-translation-expression-visitor
Open

IBX-12335: Fixed field criterion querying on invalid table when translation table is joined#41
barw4 wants to merge 4 commits into
4.6from
ibx-12335-translation-expression-visitor

Conversation

@barw4

@barw4 barw4 commented Aug 24, 2026

Copy link
Copy Markdown
🎫 Issue IBX-12335

Description:

ExpressionVisitor::walkComparison() routed a column into the translation subquery whenever the _ml table had a column with that name, without checking the main table first. Every _ml table has its own id primary key, so filtering by entity id silently compared translation-row ids instead:

-- FieldValueCriterion('id', 2), expected:
main.id = 2

-- actual:
main.id IN (SELECT t.main_id FROM main_ml t WHERE t.main_id = main.id AND t.id = 2)

With one translation per entity both ids happen to be equal, so results looked correct. They diverge as soon as any entity has a second translation - the query then returns the wrong entity (confirmed via public API in ibexa/payment).

Now a column defined on the main table always targets the main table; only columns existing solely on the translation table are resolved through the translation subquery.

For QA:

Documentation:

@barw4 barw4 self-assigned this Aug 24, 2026
@barw4 barw4 added Bug Something isn't working Ready for review labels Aug 24, 2026
Comment thread src/lib/Gateway/ExpressionVisitor.php Outdated
Comment on lines 96 to 106

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be more correct to "merge" this special case (translation column) with the later logic block (i.e. return instead of throwing an exception if there's a translated column)?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done dda36d7

@barw4
barw4 requested a review from a team August 24, 2026 10:24
Comment thread .github/workflows/backend-ci.yaml Outdated
Comment on lines +62 to +63
- name: Run integration test suite
run: composer run-script test-integration

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we're combining (SQLite) integration tests together with unit tests into single CI job?

If we have already splitted jobs for postgres/mysql, can we do the same with SQLite? IMO it's more consinstent approach

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is core a good example tbh? Do we have more packages following that pattern?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core is a bit of an exception, but the pattern is good and expected - SQLite integration tests usually depend on unit tests passing to conserve github minutes, and execute after - the preceeding setup is almost - if not always - exactly the same, so we don't need to waste extra processing just to checkout and run initial build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working Ready for QA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants