dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.68k stars 3.17k forks source link

[Potential regression] Entity Framework core allows skip and take without an OrderBy clause #20405

Open GeeWee opened 4 years ago

GeeWee commented 4 years ago

I've been trying to implement pagination with Skip and Take - if these are called without an OrderBy first, the EF core will silently let this pass. I think that 9 out of 10 times this is a mistake, as the rows will be randomly ordered, and you'll just get arbitrary results.

I think it should log a warning or throw an error if you try this.

Looking at issue #13155 where it says it will throw an error, this might be a regression

Steps to reproduce

# Take this model
    public class FooEntity
    {

        [Key] public int Id { get; set; }
    }

# and this DbContext
public class OperationalDbContext : DbContext
{
public DbSet<FooEntity> FooEntities { get; set; } = null!;

public OperationalDbContext(DbContextOptions options) : base(options)
{
}

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder
    .Entity<FooEntity>()
    .ToTable("foo");
}

A query using skip/take without an orderby passes fine but is not ordered

var foo = _dbContext.FooEntities
    .Skip(5)
    .Take(5)
    .ToList();

Returns the following query

SELECT [f].[Id]
FROM [FooEntities] AS [f]
ORDER BY (SELECT 1)
OFFSET @__p_0 ROWS FETCH NEXT @__p_0 ROWS ONLY 

Where ORDER BY (SELECT 1) doesn't enforce any particular order, but just makes SQL Server shut up (1, 2).

Further technical details

EF Core version: 3.1.2 Database provider: Microsoft.EntityFrameworkCore.SqlServer Target framework: NET Core 3.1 Operating system: Fedora 31 IDE: Jetbrains Rider 2019.3.4

ajcvickers commented 4 years ago

/cc @maumar

ajcvickers commented 4 years ago

Discussed this as a team and agreed that a warning for this would be useful.

GeeWee commented 4 years ago

This is excellent news!