Fix alias cloning for json recordset table-valued functions#3773
Fix alias cloning for json recordset table-valued functions#37737645re wants to merge 1 commit intonpgsql:hotfix/9.0.5from
Conversation
| [ConditionalFact] | ||
| public virtual async Task GroupBy_with_json_collection_predicate_and_projecting_group_elements_works() | ||
| { | ||
| var contextFactory = await InitializeAsync<JsonEntitiesContext>( | ||
| seed: async context => | ||
| { | ||
| context.Entities.AddRange( | ||
| new JsonEntity | ||
| { | ||
| Id = 1, | ||
| GroupKey = 10, | ||
| SortOrder = 1, | ||
| JsonCollection = [new JsonCollectionElement { Value = Guid.Parse("11111111-1111-1111-1111-111111111111") }] | ||
| }, | ||
| new JsonEntity | ||
| { | ||
| Id = 2, | ||
| GroupKey = 10, | ||
| SortOrder = 2, | ||
| JsonCollection = [new JsonCollectionElement { Value = Guid.Parse("11111111-1111-1111-1111-111111111111") }] | ||
| }); | ||
|
|
||
| await context.SaveChangesAsync(); | ||
| }); | ||
|
|
||
| await using var context = contextFactory.CreateContext(); | ||
|
|
||
| var values = new[] { Guid.Parse("11111111-1111-1111-1111-111111111111") }; | ||
|
|
||
| var result = await context.Entities | ||
| .Where(entity => entity.JsonCollection.Any(element => values.Contains(element.Value))) | ||
| .GroupBy(entity => entity.GroupKey) | ||
| .Select(g => new { GroupKey = g.Key, Elements = g.OrderBy(entity => entity.SortOrder).Take(1) }).ToListAsync(); | ||
|
|
||
| var group = Assert.Single(result); | ||
| Assert.Equal(10, group.GroupKey); | ||
|
|
||
| var element = Assert.Single(group.Elements); | ||
| Assert.Equal(1, element.Id); | ||
| } | ||
|
|
There was a problem hiding this comment.
@roji would this test shape look okay to you for the fix?
I reproduced the bug with a query that filters on a JSON
collection predicate, does a GroupBy, and projects ordered
group elements with Take(1). I added a small regression test in
AdHocJsonQueryNpgsqlTest and made it assert successful
execution/result shape rather than exact SQL text, since the
failure was an execution-time aliasing issue (missing FROM-
clause entry for table "o0"), not just a SQL-shape regression
If you’d prefer this to be expressed differently, e.g. with
AssertSql(...) or in another test file, I can adjust it
There was a problem hiding this comment.
Testing on the execution/result shape is indeed better than a simple SQL assertion (although it's always a good idea to also have the SQL assertion, which is missing here).
But it's also always better to have a regular query test rather than an ad-hoc test - ad-hoc tests are slower. So if you can move this to the association tests in this case, that would be ideal.
There was a problem hiding this comment.
Pull request overview
Fixes alias handling when cloning PostgreSQL table-valued function (TVF) expressions used for JSON json[b]_to_recordset, preventing invalid SQL aliases that can surface as runtime failures (e.g. missing FROM-clause entries).
Changes:
- Adjust
PgTableValuedFunctionExpression.Clone(...)to preserve the original alias when there is no explicit column list, and to apply the cloned alias when explicit output columns exist. - Add a functional regression test covering a
GroupByover a JSON-collection predicate with projection of ordered group elements.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/EFCore.PG/Query/Expressions/Internal/PgTableValuedFunctionExpression.cs |
Fixes alias cloning behavior for JSON recordset TVFs to keep FROM references consistent. |
test/EFCore.PG.FunctionalTests/Query/AdHocJsonQueryNpgsqlTest.cs |
Adds a regression scenario exercising JSON collection predicates combined with grouping and element projection. |
You can also share your feedback on Copilot code review. Take the survey.
| var result = await context.Entities | ||
| .Where(entity => entity.JsonCollection.Any(element => values.Contains(element.Value))) | ||
| .GroupBy(entity => entity.GroupKey) | ||
| .Select(g => new { GroupKey = g.Key, Elements = g.OrderBy(entity => entity.SortOrder).Take(1) }).ToListAsync(); | ||
|
|
There was a problem hiding this comment.
This new regression test doesn't assert the generated SQL baseline, unlike the other tests in this file. Since the bug is in SQL generation/aliasing, adding an AssertSql baseline here would better lock the translation shape and prevent future regressions that still happen to execute successfully on seeded data.
| } | ||
|
|
||
| return new PgTableValuedFunctionExpression(Alias, Name, arguments, ColumnInfos, WithOrdinality); | ||
| // Without ColumnInfos (e.g. unnest), PostgreSQL ties the output column name to the table alias, so preserve it. |
There was a problem hiding this comment.
The comment mentions "e.g. unnest" for the ColumnInfos == null case, but unnest is represented by PgUnnestExpression (which overrides Clone) rather than PgTableValuedFunctionExpression directly. Consider adjusting the example to avoid misleading future readers (e.g. refer generically to TVFs without an explicit column list, or to an actual caller of PgTableValuedFunctionExpression with null ColumnInfos).
| // Without ColumnInfos (e.g. unnest), PostgreSQL ties the output column name to the table alias, so preserve it. | |
| // Without ColumnInfos (e.g. TVFs without an explicit column list), PostgreSQL ties the output column name to the table alias, so preserve it. |
Fix alias cloning for
json[b]_to_recordsetinPgTableValuedFunctionExpression.Clone(...).For TVFs without
ColumnInfossuch asunnest, the originalalias must be preserved. For
jsonb_to_recordset, outputcolumns are explicitly named, so the cloned alias must be
applied instead.
This could previously cause execution-time failures such as
missing FROM-clause entry for table "o0".Adds a regression test covering GroupBy over a query filtered
by a JSON collection predicate and projecting ordered group
elements.