8.0 API Reviews

[bricelam]

As a team, we periodically review changes made to the public API. This issue tracks follow-up items that come out of those reviews for the 8.0 release.

Feb 3

Done

• @ajcvickers Make DbContextOptionsBuilder.ResolveRootApplicationServiceProvider a parameterless overload of UseRootApplicationServiceProvider
• @ajcvickers Make NavigationEntry.LoadWithIdentityResolution an overload of Load with a flags enum with a ForceIdentityResolution value
• @ajcvickers Ensure all interceptor data classes (including MaterializationInterceptionData) have pubternal ctors
• @ajcvickers Move the serviceType parameter to the second position in IConventionEntityType.AddServiceProperty
• @ajcvickers Collapse the overloads of RuntimeEntityType.AddServiceProperty. A serviceType of null means the type is inferred.
• @ajcvickers Ensure all parameter object types (including EntityMaterializerSourceParameters) have pubternal ctors
  • Need to be public for overrides
• @ajcvickers Instead of adding TypeMappingInfo.HasKeySemantics, can we just use IsKeyOrIndex?
• @ajcvickers Instead of adding ignoreNonVirtualNavigations to ProxiesExtensions.UseLazyLoadingProxies, add a nested closure overload with a builder. The builder overload shouldn't include the useproxies parameter.
• @ajcvickers Add Is prefix to RelationalOptionsExtension.ConnectionOwned
• @ajcvickers Make the parameter names of SqlServerDbFunctionsExtensions.DateDiffMillisecond consistent with other functions (e.g. Rename startTime to startTimeSpan)
• @ajcvickers Is there a better name for ConverterMappingHints.Override? We should review its behavior in a design meeting. (Note from triage: WithOverride or OverrideWith.)

May 15

Done

• @ajcvickers Rename IClrPropertyGetter.HasSentinelValue to just HasSentinel
• @roji Is there a better name for InlineQueryRootExpression? It seems too generic. Maybe something with Collection?
  • @roji says: I thought about it some more; a query root by definition represents some sort of collection, so I really think adding Collection is redundant (note that we'd also have to do it for ParameterQueryRootExpression)... For comparison, existing type names for query roots are EntityQueryRootExpression, SqlQueryRootExpression and TableValuedFunctionQueryRootExpression; I think InlineQueryRootExpression and ParameterQueryRootExpression fit well into this and are reasonably descriptive - but let me know if there are strong feelings here and I will change.
  • QueryRootProcessor.ShouldConvertToInlineQueryRoot too
  • TranslateCollection should be renamed to TranslateScalarCollection
    • @roji says: I ended up renaming to TranslatePrimitiveCollection since that's the naming we ended up using e.g. in the user-facing builder API.
• @roji Can you explain QueryableMethodTranslatingExpressionVisitor.Translate and why we didn't just use Visit?
• @roji Is there a better name for QuerySqlGenerator.TryGenerateWithoutWrappingSelect? It doesn't follow the typical Try- pattern.
  • Looking at it, this does return a bool to indicate success/failure - though it indeed has no out var. I think that's OK and in-line with established patterns - DateTime.TryFormat is a good example of an API which behaves just like this. The rest of the name is a bit long/wonky, but I can't really come up with some better... In some cases we have a SelectExpression wrapping something where the SelectExpression doesn't actually need to be generated in SQL (one example is X UNION y, which we represent as wrapped by a SelectExpression although it isn't outputted). This method is what identifies these cases and also writes the SQL out; we could also shorten this to TryGenerateUnwrapped, which is shorter but less descriptive - am open to other ideas.
• @roji OPENJSON instead of OpenJson

July 14

Done

• @AndriySvyryd Re-add and obsolete AnnotatableBuilder.RemoveAnnotation
• @AndriySvyryd Rename IConventionTypeBase.FundamentalEntityType to ContainingEntityType
• @AndriySvyryd Rename IReadOnlyComplexType.IsInDeclarationPath to IsContainedBy
  • And its parameter to type
• @AndriySvyryd Remove ComplexTypePropertyBuilder.IsConcurrencyToken (and IsRowVersion) before GA if not implemented
• @AndriySvyryd Rename IConventionModelBuilder.Complex to ComplexType
• @ajcvickers Can IdValueGeneratorFactory in Cosmos be pubternal?
• @AndriySvyryd Rename RelationalDbFunctionExtensions back to Functions
• @roji Consistently use StructuralType for properties, parameters, new related types, etc. when referring to ITypeBase
• @roji Reexamine applyDefaultTypeMapping on RelationalQueryableMethodTranslatingExpressionVisitor.TranslateExpression
• @roji Is there a better name for SqlNullabilityProcessor.PreferExistsToComplexIn? What makes it complex?
  • Renamed it to PreferExistsToInWithCoalesce, which explains better when this is used (but still sucks).
• @roji Ensure SelectExpression.DebugView is pubternal
• @ajcvickers Undo the source breaking change to RelationalGeometryTypeMapping.ctor (move reader/writer to the end, default to null)
• @ajcvickers Rename WKT to Wkt everywhere
• @AndriySvyryd Think about ways to keep ComplexTypePropertyBuilder provider extensions in sync with PropertyBuilder extensions

July 28

Done

• @ajcvickers Remove Try from TypeMappingSource.TryFindCollectionMapping
• @ajcvickers Rename JsonNullableStuctsCollectionReaderWriter to JsonNullableStructCollectionReaderWriter
• @ajcvickers Seal JsonValueReaderWriter.FromJsonString and ToJsonString
• @ajcvickers Is there a better name for the RelationalTypeMapping.Clone methods? Maybe something with with.

Sept 8

Done

• @ajcvickers Rename ILoggingOptions.ShouldWarnForEnumType to ShouldWarnForStringEnumValueInJson
  • Rename parameter to enumType
• @ajcvickers Rename IClrPropertyGetter.GetClrValue to GetClrValueFromContainingEntityType
  • Rename GetStructuralTypeClrValue to GetClrValue
    • Rename parameter to structuralObject
  • Make IClrPropertySetter behavior and naming consistent
    • Consider splitting clr-based from metadata-based logic
• @ajcvickers Rename the bool parameter on IMutableProperty.ElementType and friends to primitiveCollection
  • How do we get the bool value from IReadOnlyProperty and friends?
• @ajcvickers Make IReadOnlyProperty.GetElementType return IReadOnlyElementType
• @ajcvickers Should ParameterBindingInfo.ctor parameter change from IEntityType to ITypeBase? How is it working for non-entity types?
• @ajcvickers Review ITypeMappingSource.FindMapping calls to make sure they're calling the best overload
• @ajcvickers Document TypeMappingSource.FindCollectionMapping as pubternal
• @ajcvickers Is NullTypeMapping still needed? Is it better to throw?
• @ajcvickers / @AndriySvyryd Are the new methods on ColumnModification (GetCurrentProviderValue, GetOriginalProviderValue, etc.) still needed?
• @ajcvickers Remove the SQLite HasSrid extension methods from primitive collection builders
• @roji @ajcvickers Fix the namespace Microsoft.EntityFrameworkCore.Sqlite.Storage.Internal.Json

Sept 15

Done

• @ajcvickers Should the parameter of CollectionToJsonStringConverter.ctor be named element?
  • It's actually not the element reader writer after all. :-)
• @bricelam Rename SqlServerIndexBuilderExtensions.IsSortedInTempDb and friends to SortInTempDb

Punted

• @ajcvickers Break CoreTypeMapping even more since we're breaking it anyway--clean up any previous breaking change mitigations
• @ajcvickers Think about how our JsonValueReaderWriter relates to STJ's JsonConverter, JsonConverterAttribute, and our existing HierarchyIdJsonConverter implementation
• @AndriySvyryd Should SqlServerDbContextOptionsBuilder.UseAzureSql be an alternative for UseSqlServer instead? If not instead, should we add that as sugar?

[kevinchalet]

It seems a binary breaking change was recently made to the NavigationEntry class and more specifically to the Load(Async) API, whose signature changed from Task LoadAsync(CancellationToken cancellationToken = default) to Task LoadAsync(LoadOptions options = LoadOptions.Default, CancellationToken cancellationToken = default).

I don't see it listed in https://learn.microsoft.com/en-us/ef/core/what-is-new/ef-core-8.0/breaking-changes. Is it an omission?

FWIW, it breaks OpenIddict, that relies on this API: openiddict/openiddict-core#1789

[bkoelman]

The changed NavigationEntry.LoadAsync breaks our build as well. But what's worse: no matter what LoadOptions I pass in, our existing tests keep failing with: "The navigation '(name)' cannot be loaded because one or more of the key or foreign key properties are shadow properties and the entity is not being tracked. Relationships using shadow values can only be loaded for tracked entities."

Is there a suggested workaround to preserve the old behavior? The keys are indeed shadow properties. The parent resource is in the change tracker and we're calling LoadAsync() on the inverse navigation (here and here), so that all required entities end up in the change tracker before calling SaveChanges(). I haven't investigated in detail yet, but it seems odd that loading something into the change tracker requires that it's already in there.

[ajcvickers]

@bkoelman Please file a new issue and attach a small, runnable project or post a small, runnable code listing that reproduces what you are seeing so that we can investigate.

[kevinchalet]

@ajcvickers and my question?

I guess you don't plan on reverting this change to avoid the impact it has on existing projects?

[roji]

@kevinchalet it's difficult to answer your question when you haven't provided a code sample that shows what you're asking about... Please open a new issue with that so we can investigate further.

[kevinchalet]

@roji I'm not sure why you'd need a code sample for such a trivial question. Let me rephrase it:

EF Core 7.0 (and earlier) has the following NavigationEntry.LoadAsync(...) API:

efcore/src/EFCore/ChangeTracking/NavigationEntry.cs

Line 122 in 81a09d5

public abstract Task LoadAsync(CancellationToken cancellationToken = default);

This API was removed in EF Core 8.0 (without being obsoleted first, BTW...) and replaced by:

efcore/src/EFCore/ChangeTracking/NavigationEntry.cs

Line 118 in 098cf04

public abstract Task LoadAsync(LoadOptions options = LoadOptions.None, CancellationToken cancellationToken = default);

You can easily see that the signature has changed in a breaking way (an optional parameter has been added), which causes MemberMissingExceptions when referencing EF Core 8.0 if you depend on projects - like mine - that use this API and are compiled against EF Core 7.0.

So my question is: why wasn't this binary breaking change announced and do you plan to keep it in the final version of EF Core?

[kevinchalet]

Reporting a breaking change in a thread named "API reviews" seemed fine, but as requested by @roji, I opened #31104.

[roji]

I'm not sure why you'd need a code sample for such a trivial question.

Apologies, I confused your message with @bkoelman - always better to open a separate, dedicated issue rather than posting on a common one (and thanks for opening #31104 for that).

[AndriySvyryd]

Should SqlServerDbContextOptionsBuilder.UseAzureSql be an alternative for UseSqlServer instead? If not instead, should we add that as sugar?

From a semantic standpoint Azure SQL is a strict subset of SQL Server and so it makes sense to find UseAzureSql nested at the same level as UseCompatibilityLevel.
From usability standpoint there are already 8 overloads of UseSqlServer and AddSqlServer.
Therefore, I don't think we need to change anything here.

[roji]

Should SqlServerDbContextOptionsBuilder.UseAzureSql be an alternative for UseSqlServer instead? If not instead, should we add that as sugar?

The main point here for me is to make the distinction between SQL Server and Azure SQL clear to users. SQL Server is generally used to denote the on-premise product, so it's quite odd to do UseSqlServer() and specify inside that it happens to be AzureSQL (again, purely from a user perspective).

From usability standpoint there are already 8 overloads of UseSqlServer and AddSqlServer.

I don't think it should be a problem to add those same overloads for a completely different name (i.e. UseAzureSql alongside UseSqlServer), no? the user still makes the choice of UseSqlServer vs. UseAzureSql first.

[AndriySvyryd]

SQL Server is generally used to denote the on-premise product, so it's quite odd to do UseSqlServer() and specify inside that it happens to be AzureSQL (again, purely from a user perspective).

That follows marketing terms more than the underlying technology and it might be confusing for some products, such as Azure SQL Managed Instance, or when the marketing terminology changes.

It might make it clearer if we rename UseAzureSql to something like ApplyAzureSqlDefaults.

[roji]

That follows marketing terms more than the underlying technology and it might be confusing for some products, such as Azure SQL Managed Instance, or when the marketing terminology changes.

That's true, but that confusion is already there without changing UseSqlServer to UseAzureSql (and also if we rename to ApplyAzureSqlDefaults).

Let's maybe have a design discussion on this when you're back...

[ErikEJ]

@roji @AndriySvyryd is this for special casing of for example retry strategies?

[AndriySvyryd]

@roji @AndriySvyryd is this for special casing of for example retry strategies?

For now, yes. But it could also enable certain features in the future.

[ajcvickers]

@bricelam What does, "Fix the namespace Microsoft.EntityFrameworkCore.Sqlite.Storage.Internal.Json" mean?

[bricelam]

@ajcvickers Put the .Internal part last to make sure our analyzer catches it.

[ajcvickers]

Note from triage: use UseAzureSqlDefaults or similar on the nested builder. Don't add a top-level for now.