Skip to content

Fix alias cloning for json recordset table-valued functions #3773

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

Open
7645re wants to merge 1 commit into
base: hotfix/9.0.5
Choose a base branch
from
Open

Fix alias cloning for json recordset table-valued functions #3773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion src/EFCore.PG/Query/Expressions/Internal/PgTableValuedFunctionExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,9 @@ public override TableExpressionBase Clone(string? alias, ExpressionVisitor cloni
arguments[i] = (SqlExpression)cloningExpressionVisitor.Visit(Arguments[i]);
}

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.
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
// 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.

Copilot uses AI. Check for mistakes.
// With explicit ColumnInfos (e.g. jsonb_to_recordset), apply the clone alias to keep FROM references consistent.
return new PgTableValuedFunctionExpression(ColumnInfos is null ? Alias : alias!, Name, arguments, ColumnInfos, WithOrdinality);
}

/// <inheritdoc />
Expand Down
65 changes: 65 additions & 0 deletions test/EFCore.PG.FunctionalTests/Query/AdHocJsonQueryNpgsqlTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,47 @@ LIMIT 2
}
}

[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();

Comment on lines +278 to +282
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
var group = Assert.Single(result);
Assert.Equal(10, group.GroupKey);

var element = Assert.Single(group.Elements);
Assert.Equal(1, element.Id);
}

Comment on lines +249 to +289
Copy link
Author

@7645re 7645re Mar 10, 2026

Choose a reason for hiding this comment

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

@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

Copy link
Member

@roji roji Mar 10, 2026

Choose a reason for hiding this comment

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

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.

protected class TypesDbContext(DbContextOptions options) : DbContext(options)
{
public DbSet<TypesContainerEntity> Entities { get; set; }
Expand All @@ -266,6 +307,30 @@ public class TypesJsonEntity
public TimeSpan Interval { get; set; }
}

protected class JsonEntitiesContext(DbContextOptions options) : DbContext(options)
{
public DbSet<JsonEntity> Entities { get; set; }

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<JsonEntity>().Property(x => x.Id).ValueGeneratedNever();
modelBuilder.Entity<JsonEntity>().OwnsMany(x => x.JsonCollection).ToJson();
}
}

public class JsonEntity
{
public int Id { get; set; }
public int GroupKey { get; set; }
public int SortOrder { get; set; }
public List<JsonCollectionElement> JsonCollection { get; set; }
}

public class JsonCollectionElement
{
public Guid Value { get; set; }
}

protected void AssertSql(params string[] expected)
=> TestSqlLoggerFactory.AssertBaseline(expected);
}