diff --git a/docs/configuration.md b/docs/configuration.md index a28480cd..31bd065c 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -24,9 +24,10 @@ public static void RegisterInContainer( ResolveFilters? resolveFilters = null, bool disableTracking = false, bool includeSqlInExceptions = false, - bool splitSideBySideCollections = false) + bool splitSideBySideCollections = false, + bool orderNavigationsByKey = true) ``` -snippet source | anchor +snippet source | anchor ```cs EfGraphQLConventions.RegisterInContainer( @@ -141,6 +142,18 @@ The resolver can not know in advance which collections a request will select, so It defaults to false. +#### OrderNavigationsByKey + +A navigation list or connection field loaded through a projection, with no `orderBy` argument, is ordered by its key so its items come back in a deterministic order. + +Setting `orderNavigationsByKey` to false leaves such a collection unordered, for a model that orders the collections a projection loads by a default of its own. [EfOrderBy](https://github.com/SimonCropp/EntityFramework.OrderBy) 1.4 and later does this. EfOrderBy leaves a collection that is already ordered as it is, so the key ordering otherwise replaces the configured default, and a client that leaves out `orderBy` gets the items in key order rather than in the order the model defines. + + * An `orderBy` argument is still applied, and replaces the default. + * A model with no default for an entity leaves that entity's collections in whatever order the database returns them, so only turn this off where every entity has one, for example with EfOrderBy's `requireOrderingForAllEntities`. + +It defaults to true. + + ### Usage @@ -153,9 +166,10 @@ public static void RegisterInContainer( ResolveFilters? resolveFilters = null, bool disableTracking = false, bool includeSqlInExceptions = false, - bool splitSideBySideCollections = false) + bool splitSideBySideCollections = false, + bool orderNavigationsByKey = true) ``` -snippet source | anchor +snippet source | anchor ```cs EfGraphQLConventions.RegisterInContainer( diff --git a/docs/mdsource/configuration.source.md b/docs/mdsource/configuration.source.md index 159b2bb0..fdfbafa9 100644 --- a/docs/mdsource/configuration.source.md +++ b/docs/mdsource/configuration.source.md @@ -81,6 +81,18 @@ The resolver can not know in advance which collections a request will select, so It defaults to false. +#### OrderNavigationsByKey + +A navigation list or connection field loaded through a projection, with no `orderBy` argument, is ordered by its key so its items come back in a deterministic order. + +Setting `orderNavigationsByKey` to false leaves such a collection unordered, for a model that orders the collections a projection loads by a default of its own. [EfOrderBy](https://github.com/SimonCropp/EntityFramework.OrderBy) 1.4 and later does this. EfOrderBy leaves a collection that is already ordered as it is, so the key ordering otherwise replaces the configured default, and a client that leaves out `orderBy` gets the items in key order rather than in the order the model defines. + + * An `orderBy` argument is still applied, and replaces the default. + * A model with no default for an entity leaves that entity's collections in whatever order the database returns them, so only turn this off where every entity has one, for example with EfOrderBy's `requireOrderingForAllEntities`. + +It defaults to true. + + ### Usage snippet: RegisterInContainer diff --git a/src/Directory.Build.props b/src/Directory.Build.props index 1ea825f7..34492a5b 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -2,7 +2,7 @@ CS1591;NU5104;CS1573;CS9107;NU1608;NU1109 - 36.2.0 + 36.3.0 preview 1.0.0 EntityFrameworkCore, EntityFramework, GraphQL diff --git a/src/GraphQL.EntityFramework/EfGraphQLConventions.cs b/src/GraphQL.EntityFramework/EfGraphQLConventions.cs index 586469db..261227e0 100644 --- a/src/GraphQL.EntityFramework/EfGraphQLConventions.cs +++ b/src/GraphQL.EntityFramework/EfGraphQLConventions.cs @@ -12,6 +12,7 @@ public static class EfGraphQLConventions /// Use for all operations. /// Include the generated sql in exception messages. Off by default, since those messages can reach clients and the sql carries table and column names and, depending on the provider, parameter values. /// Use for a query whose request selects collections side by side, which multiply each other's rows in a single query. A query the resolver already called AsSplitQuery or AsSingleQuery on is left as it is. Needs a relational provider. + /// Order a projected navigation list or connection field that has no orderBy argument by its key, so its items come back in a deterministic order. Pass false when the model orders the collections a projection loads by a default of its own, such as EfOrderBy 1.4 or later does, so that default is used instead of the key. #region RegisterInContainer @@ -22,7 +23,8 @@ public static void RegisterInContainer( ResolveFilters? resolveFilters = null, bool disableTracking = false, bool includeSqlInExceptions = false, - bool splitSideBySideCollections = false) + bool splitSideBySideCollections = false, + bool orderNavigationsByKey = true) #endregion @@ -31,7 +33,7 @@ public static void RegisterInContainer( RegisterScalarsAndArgs(services); services.AddHttpContextAccessor(); services.AddTransient(); - services.AddSingleton(provider => Build(resolveDbContext, model, resolveFilters, provider, disableTracking, includeSqlInExceptions, splitSideBySideCollections)); + services.AddSingleton(provider => Build(resolveDbContext, model, resolveFilters, provider, disableTracking, includeSqlInExceptions, splitSideBySideCollections, orderNavigationsByKey)); services.AddSingleton>(provider => provider.GetRequiredService>()); // The generated where and orderBy input types resolve every registered service, so a // type mapped in any of the contexts is found @@ -45,7 +47,8 @@ static EfGraphQLService Build( IServiceProvider provider, bool disableTracking, bool includeSqlInExceptions, - bool splitSideBySideCollections) + bool splitSideBySideCollections, + bool orderNavigationsByKey) where TDbContext : DbContext { model ??= ResolveModel(provider); @@ -58,7 +61,8 @@ static EfGraphQLService Build( filters, disableTracking, includeSqlInExceptions, - splitSideBySideCollections); + splitSideBySideCollections, + orderNavigationsByKey); } static TDbContext DbContextFromProvider(IServiceProvider provider, IServiceProvider? requestServices) diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService.cs index baca3d76..fa91b261 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService.cs @@ -13,13 +13,15 @@ public partial class EfGraphQLService : /// Use for all operations. /// Include the generated sql in exception messages. Off by default, since those messages can reach clients and the sql carries table and column names and, depending on the provider, parameter values. /// Use for a query whose request selects collections side by side, which multiply each other's rows in a single query. A query the resolver already called AsSplitQuery or AsSingleQuery on is left as it is. Needs a relational provider. + /// Order a projected navigation list or connection field that has no orderBy argument by its key, so its items come back in a deterministic order. Pass false when the model orders the collections a projection loads by a default of its own, such as EfOrderBy 1.4 or later does, so that default is used instead of the key. public EfGraphQLService( IModel model, ResolveDbContext resolveDbContext, ResolveFilters? resolveFilters = null, bool disableTracking = false, bool includeSqlInExceptions = false, - bool splitSideBySideCollections = false) + bool splitSideBySideCollections = false, + bool orderNavigationsByKey = true) { this.resolveFilters = resolveFilters; this.disableTracking = disableTracking; @@ -42,7 +44,7 @@ public EfGraphQLService( .OrderBy(derived => Depth(derived.ClrType)) .Select(derived => derived.ClrType) .ToList()); - includeAppender = new(Navigations, keyNames, foreignKeys, derivedTypes, BackingFieldReader.GetBackingFields(model), splitSideBySideCollections); + includeAppender = new(Navigations, keyNames, foreignKeys, derivedTypes, BackingFieldReader.GetBackingFields(model), splitSideBySideCollections, orderNavigationsByKey); } static int Depth(Type type) diff --git a/src/GraphQL.EntityFramework/IncludeAppender.cs b/src/GraphQL.EntityFramework/IncludeAppender.cs index e428c646..de5f6943 100644 --- a/src/GraphQL.EntityFramework/IncludeAppender.cs +++ b/src/GraphQL.EntityFramework/IncludeAppender.cs @@ -4,7 +4,8 @@ IReadOnlyDictionary> foreignKeys, IReadOnlyDictionary> derivedTypes, IReadOnlyDictionary>? backingFields = null, - bool splitSideBySideCollections = false) + bool splitSideBySideCollections = false, + bool orderNavigationsByKey = true) { /// /// Narrow to the fields the request asked for: a select projection @@ -52,7 +53,7 @@ public IQueryable ApplyProjection( query = query.AsSplitQuery(); } - if (!SelectExpressionBuilder.TryBuild(projection, keyNames, derivedTypes, out var expression, out var includePaths, out var argumentFields, backingFields)) + if (!SelectExpressionBuilder.TryBuild(projection, keyNames, derivedTypes, out var expression, out var includePaths, out var argumentFields, backingFields, orderNavigationsByKey)) { return ApplyTracking(AddIncludesFromProjection(query, projection), disableTracking); } diff --git a/src/GraphQL.EntityFramework/SelectProjection/SelectExpressionBuilder.cs b/src/GraphQL.EntityFramework/SelectProjection/SelectExpressionBuilder.cs index b5d774d5..d5052c94 100644 --- a/src/GraphQL.EntityFramework/SelectProjection/SelectExpressionBuilder.cs +++ b/src/GraphQL.EntityFramework/SelectProjection/SelectExpressionBuilder.cs @@ -107,6 +107,12 @@ public static bool TryBuild( /// The navigation fields whose ids, where and orderBy were applied inside their collection /// subquery, so their resolvers know not to apply them again. /// + /// + /// Whether a projected collection without an orderBy is ordered by key. False leaves it + /// unordered, for a model that orders the collections a projection loads by a default of its + /// own, such as EfOrderBy. Ordering by key there replaced that default, since a collection + /// that is already ordered is left as it is. + /// public static bool TryBuild( FieldProjectionInfo projection, IReadOnlyDictionary> keyNames, @@ -114,11 +120,12 @@ public static bool TryBuild( [NotNullWhen(true)] out Expression>? expression, out IReadOnlyList includePaths, out IReadOnlyList argumentFields, - IReadOnlyDictionary>? backingFields = null) + IReadOnlyDictionary>? backingFields = null, + bool orderNavigationsByKey = true) where TEntity : class { expression = null; - var state = new BuildState(keyNames, derivedTypes, backingFields); + var state = new BuildState(keyNames, derivedTypes, backingFields, orderNavigationsByKey); includePaths = state.IncludePaths; argumentFields = state.ArgumentFields; var entityType = typeof(TEntity); @@ -547,7 +554,8 @@ static MemberAssignment BuildFullNavigationBinding( /// The navigation field's ids, where and orderBy, applied to the collection inside the /// subquery so the database evaluates them against the table rather than the resolver /// evaluating them in memory against the projected columns. Without an orderBy the - /// collection is ordered by key, where , for a deterministic result. + /// collection is ordered by key, where and the build orders + /// navigations by key, for a deterministic result. /// static Expression ApplyArguments( Expression source, @@ -591,6 +599,7 @@ static Expression ApplyArguments( } if (orderByKey && + state.OrderNavigationsByKey && state.KeyNames.TryGetValue(navType, out var keys) && keys.Count > 0 && GetEntityMetadata(navType).Properties.TryGetValue(keys[0], out var keyMetadata)) @@ -619,10 +628,12 @@ static Expression Where(Expression source, ParameterExpression navParam, Type na sealed class BuildState( IReadOnlyDictionary> keyNames, IReadOnlyDictionary> derivedTypes, - IReadOnlyDictionary>? backingFields) + IReadOnlyDictionary>? backingFields, + bool orderNavigationsByKey) { public IReadOnlyDictionary> KeyNames { get; } = keyNames; public IReadOnlyDictionary> DerivedTypes { get; } = derivedTypes; + public bool OrderNavigationsByKey { get; } = orderNavigationsByKey; public List IncludePaths { get; } = []; public List ArgumentFields { get; } = []; diff --git a/src/Tests/IntegrationTests/IntegrationTests.Navigation_with_orderBy_not_ordered_by_key.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Navigation_with_orderBy_not_ordered_by_key.verified.txt new file mode 100644 index 00000000..8a4ea4f5 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Navigation_with_orderBy_not_ordered_by_key.verified.txt @@ -0,0 +1,31 @@ +{ + target: { + Data: { + parentEntities: [ + { + property: Parent, + children: [ + { + property: Child + } + ] + } + ] + } + }, + sql: { + Text: +select p.Id, + c.Id, + c.ParentId, + c.Property, + p.Property +from ParentEntities as p + left outer join + ChildEntities as c + on p.Id = c.ParentId +order by p.Property, + p.Id, + c.Property desc + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Navigation_without_orderBy_not_ordered_by_key.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Navigation_without_orderBy_not_ordered_by_key.verified.txt new file mode 100644 index 00000000..478493cb --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Navigation_without_orderBy_not_ordered_by_key.verified.txt @@ -0,0 +1,30 @@ +{ + target: { + Data: { + parentEntities: [ + { + property: Parent, + children: [ + { + property: Child + } + ] + } + ] + } + }, + sql: { + Text: +select p.Id, + c.Id, + c.ParentId, + c.Property, + p.Property +from ParentEntities as p + left outer join + ChildEntities as c + on p.Id = c.ParentId +order by p.Property, + p.Id + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Navigation_without_orderBy_ordered_by_key.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Navigation_without_orderBy_ordered_by_key.verified.txt new file mode 100644 index 00000000..576781b3 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Navigation_without_orderBy_ordered_by_key.verified.txt @@ -0,0 +1,31 @@ +{ + target: { + Data: { + parentEntities: [ + { + property: Parent, + children: [ + { + property: Child + } + ] + } + ] + } + }, + sql: { + Text: +select p.Id, + c.Id, + c.ParentId, + c.Property, + p.Property +from ParentEntities as p + left outer join + ChildEntities as c + on p.Id = c.ParentId +order by p.Property, + p.Id, + c.Id + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.cs b/src/Tests/IntegrationTests/IntegrationTests.cs index 45c13c33..45dd9763 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.cs +++ b/src/Tests/IntegrationTests/IntegrationTests.cs @@ -3462,6 +3462,7 @@ static async Task RunQuery( bool disableTracking, object[] entities, bool includeSqlInExceptions = false, + bool orderNavigationsByKey = true, [CallerFilePath] string sourceFile = "") { var dbContext = database.Context; @@ -3482,7 +3483,7 @@ static async Task RunQuery( ExecutionResult result; try { - result = await QueryExecutor.ExecuteQuery(query, services, context, inputs, filters, disableTracking, includeSqlInExceptions); + result = await QueryExecutor.ExecuteQuery(query, services, context, inputs, filters, disableTracking, includeSqlInExceptions, orderNavigationsByKey); } catch (ExecutionError executionError) { diff --git a/src/Tests/IntegrationTests/IntegrationTests_navigation_key_order.cs b/src/Tests/IntegrationTests/IntegrationTests_navigation_key_order.cs new file mode 100644 index 00000000..117afbcd --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests_navigation_key_order.cs @@ -0,0 +1,68 @@ +// A navigation list without an orderBy is ordered by key by default. With orderNavigationsByKey +// off it is left unordered, so an ordering the model applies to the collections a projection +// loads, such as EfOrderBy's default ordering, is used instead. ChildEntity defaults to Property. +public partial class IntegrationTests +{ + const string parentChildQuery = + """ + { + parentEntities + { + property + children + { + property + } + } + } + """; + + static object[] ParentWithChild() + { + var parent = new ParentEntity + { + Property = "Parent" + }; + var child = new ChildEntity + { + Property = "Child", + Parent = parent + }; + parent.Children.Add(child); + return [parent, child]; + } + + [Fact] + public async Task Navigation_without_orderBy_ordered_by_key() + { + await using var database = await sqlInstance.Build(); + await RunQuery(database, parentChildQuery, null, null, false, ParentWithChild()); + } + + [Fact] + public async Task Navigation_without_orderBy_not_ordered_by_key() + { + await using var database = await sqlInstance.Build(); + await RunQuery(database, parentChildQuery, null, null, false, ParentWithChild(), orderNavigationsByKey: false); + } + + [Fact] + public async Task Navigation_with_orderBy_not_ordered_by_key() + { + var query = + """ + { + parentEntities + { + property + children(orderBy: {property: descending}) + { + property + } + } + } + """; + await using var database = await sqlInstance.Build(); + await RunQuery(database, query, null, null, false, ParentWithChild(), orderNavigationsByKey: false); + } +} diff --git a/src/Tests/IntegrationTests/QueryExecutor.cs b/src/Tests/IntegrationTests/QueryExecutor.cs index 6c2287c9..7190c704 100644 --- a/src/Tests/IntegrationTests/QueryExecutor.cs +++ b/src/Tests/IntegrationTests/QueryExecutor.cs @@ -7,7 +7,8 @@ public static async Task ExecuteQuery( Inputs? inputs, Filters? filters, bool disableTracking, - bool includeSqlInExceptions = false) + bool includeSqlInExceptions = false, + bool orderNavigationsByKey = true) where TDbContext : DbContext { EfGraphQLConventions.RegisterInContainer( @@ -16,7 +17,8 @@ public static async Task ExecuteQuery( data.Model, _ => filters, disableTracking, - includeSqlInExceptions); + includeSqlInExceptions, + orderNavigationsByKey: orderNavigationsByKey); await using var provider = services.BuildServiceProvider(); using var schema = new Schema(provider); var executer = new EfDocumentExecuter();