-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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.
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
Add Constants.Sql.MaxParameterCount and update query logic #11369
Changes from all commits
c5ba23a
a6b04a9
88a20cb
1bc3290
60be71d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
namespace Umbraco.Core | ||
{ | ||
public static partial class Constants | ||
{ | ||
public static class Sql | ||
{ | ||
/// <summary> | ||
/// The maximum amount of parameters that can be used in a query. | ||
/// </summary> | ||
/// <remarks> | ||
/// The actual limit is 2100 (https://docs.microsoft.com/en-us/sql/sql-server/maximum-capacity-specifications-for-sql-server), | ||
/// but we want to ensure there's room for additional parameters if this value is used to create groups/batches. | ||
/// </remarks> | ||
public const int MaxParameterCount = 2000; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -768,7 +768,7 @@ private void CopyTagData(int? sourceLanguageId, int? targetLanguageId, IReadOnly | |
// note: important to use SqlNullableEquals for nullable types, cannot directly compare language identifiers | ||
|
||
var whereInArgsCount = propertyTypeIds.Count + (contentTypeIds?.Count ?? 0); | ||
if (whereInArgsCount > 2000) | ||
if (whereInArgsCount > Constants.Sql.MaxParameterCount) | ||
throw new NotSupportedException("Too many property/content types."); | ||
|
||
// delete existing relations (for target language) | ||
|
@@ -906,7 +906,7 @@ private void CopyPropertyData(int? sourceLanguageId, int? targetLanguageId, IRea | |
// note: important to use SqlNullableEquals for nullable types, cannot directly compare language identifiers | ||
// | ||
var whereInArgsCount = propertyTypeIds.Count + (contentTypeIds?.Count ?? 0); | ||
if (whereInArgsCount > 2000) | ||
if (whereInArgsCount > Constants.Sql.MaxParameterCount) | ||
throw new NotSupportedException("Too many property/content types."); | ||
|
||
//first clear out any existing property data that might already exists under the target language | ||
|
@@ -1005,7 +1005,7 @@ private void RenormalizeDocumentEditedFlags(IReadOnlyCollection<int> propertyTyp | |
//based on the current variance of each item to see if it's 'edited' value should be true/false. | ||
|
||
var whereInArgsCount = propertyTypeIds.Count + (contentTypeIds?.Count ?? 0); | ||
if (whereInArgsCount > 2000) | ||
if (whereInArgsCount > Constants.Sql.MaxParameterCount) | ||
throw new NotSupportedException("Too many property/content types."); | ||
|
||
var propertySql = Sql() | ||
|
@@ -1094,14 +1094,20 @@ private void RenormalizeDocumentEditedFlags(IReadOnlyCollection<int> propertyTyp | |
} | ||
} | ||
|
||
//lookup all matching rows in umbracoDocumentCultureVariation | ||
var docCultureVariationsToUpdate = editedLanguageVersions.InGroupsOf(2000) | ||
.SelectMany(_ => Database.Fetch<DocumentCultureVariationDto>( | ||
Sql().Select<DocumentCultureVariationDto>().From<DocumentCultureVariationDto>() | ||
.WhereIn<DocumentCultureVariationDto>(x => x.LanguageId, editedLanguageVersions.Keys.Select(x => x.langId).ToList()) | ||
.WhereIn<DocumentCultureVariationDto>(x => x.NodeId, editedLanguageVersions.Keys.Select(x => x.nodeId)))) | ||
//convert to dictionary with the same key type | ||
.ToDictionary(x => (x.NodeId, (int?)x.LanguageId), x => x); | ||
// lookup all matching rows in umbracoDocumentCultureVariation | ||
// fetch in batches to account for maximum parameter count (distinct languages can't exceed 2000) | ||
var languageIds = editedLanguageVersions.Keys.Select(x => x.langId).Distinct().ToArray(); | ||
var nodeIds = editedLanguageVersions.Keys.Select(x => x.nodeId).Distinct(); | ||
var docCultureVariationsToUpdate = nodeIds.InGroupsOf(Constants.Sql.MaxParameterCount - languageIds.Length) | ||
.SelectMany(group => | ||
{ | ||
var sql = Sql().Select<DocumentCultureVariationDto>().From<DocumentCultureVariationDto>() | ||
.WhereIn<DocumentCultureVariationDto>(x => x.LanguageId, languageIds) | ||
.WhereIn<DocumentCultureVariationDto>(x => x.NodeId, group); | ||
|
||
return Database.Fetch<DocumentCultureVariationDto>(sql); | ||
}) | ||
.ToDictionary(x => (x.NodeId, (int?)x.LanguageId), x => x); //convert to dictionary with the same key type | ||
Comment on lines
+1097
to
+1110
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in #10677 (comment), we can only retrieve valid results by always including either all language or node IDs. Because we'll have less languages than nodes in almost all cases, we can group the node IDs into batches of Because we're using distinct language and node IDs, the dictionary key should always contain unique values. |
||
|
||
var toUpdate = new List<DocumentCultureVariationDto>(); | ||
foreach (var ev in editedLanguageVersions) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -260,13 +260,12 @@ public IEnumerable<IDictionaryItem> GetDictionaryItemDescendants(Guid? parentId) | |
|
||
Func<Guid[], IEnumerable<IEnumerable<IDictionaryItem>>> getItemsFromParents = guids => | ||
{ | ||
//needs to be in groups of 2000 because we are doing an IN clause and there's a max parameter count that can be used. | ||
return guids.InGroupsOf(2000) | ||
.Select(@group => | ||
return guids.InGroupsOf(Constants.Sql.MaxParameterCount) | ||
.Select(group => | ||
{ | ||
var sqlClause = GetBaseQuery(false) | ||
.Where<DictionaryDto>(x => x.Parent != null) | ||
.Where($"{SqlSyntax.GetQuotedColumnName("parent")} IN (@parentIds)", new { parentIds = @group }); | ||
.WhereIn<DictionaryDto>(x => x.Parent, group); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using |
||
|
||
var translator = new SqlTranslator<IDictionaryItem>(sqlClause, Query<IDictionaryItem>()); | ||
var sql = translator.Translate(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,44 +38,41 @@ public PermissionRepository(IScopeAccessor scopeAccessor, AppCaches cache, ILogg | |
/// <param name="entityIds"></param> | ||
/// <returns></returns> | ||
/// <remarks> | ||
/// This method will not support passing in more than 2000 group Ids | ||
/// This method will not support passing in more than 2000 group IDs when also passing in entity IDs. | ||
/// </remarks> | ||
public EntityPermissionCollection GetPermissionsForEntities(int[] groupIds, params int[] entityIds) | ||
{ | ||
var result = new EntityPermissionCollection(); | ||
|
||
foreach (var groupOfGroupIds in groupIds.InGroupsOf(2000)) | ||
if (entityIds.Length == 0) | ||
{ | ||
//copy local | ||
var localIds = groupOfGroupIds.ToArray(); | ||
|
||
if (entityIds.Length == 0) | ||
foreach (var group in groupIds.InGroupsOf(Constants.Sql.MaxParameterCount)) | ||
{ | ||
var sql = Sql() | ||
.SelectAll() | ||
.From<UserGroup2NodePermissionDto>() | ||
.Where<UserGroup2NodePermissionDto>(dto => localIds.Contains(dto.UserGroupId)); | ||
.Where<UserGroup2NodePermissionDto>(dto => group.Contains(dto.UserGroupId)); | ||
|
||
var permissions = AmbientScope.Database.Fetch<UserGroup2NodePermissionDto>(sql); | ||
foreach (var permission in ConvertToPermissionList(permissions)) | ||
{ | ||
result.Add(permission); | ||
} | ||
} | ||
else | ||
} | ||
else | ||
{ | ||
foreach (var group in entityIds.InGroupsOf(Constants.Sql.MaxParameterCount - groupIds.Length)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly to the language/node IDs above, this always includes all group IDs and batches the entity IDs. |
||
{ | ||
//iterate in groups of 2000 since we don't want to exceed the max SQL param count | ||
foreach (var groupOfEntityIds in entityIds.InGroupsOf(2000)) | ||
var sql = Sql() | ||
.SelectAll() | ||
.From<UserGroup2NodePermissionDto>() | ||
.Where<UserGroup2NodePermissionDto>(dto => groupIds.Contains(dto.UserGroupId) && group.Contains(dto.NodeId)); | ||
|
||
var permissions = AmbientScope.Database.Fetch<UserGroup2NodePermissionDto>(sql); | ||
foreach (var permission in ConvertToPermissionList(permissions)) | ||
{ | ||
var ids = groupOfEntityIds; | ||
var sql = Sql() | ||
.SelectAll() | ||
.From<UserGroup2NodePermissionDto>() | ||
.Where<UserGroup2NodePermissionDto>(dto => localIds.Contains(dto.UserGroupId) && ids.Contains(dto.NodeId)); | ||
var permissions = AmbientScope.Database.Fetch<UserGroup2NodePermissionDto>(sql); | ||
foreach (var permission in ConvertToPermissionList(permissions)) | ||
{ | ||
result.Add(permission); | ||
} | ||
result.Add(permission); | ||
} | ||
} | ||
} | ||
|
@@ -133,11 +130,10 @@ public void ReplacePermissions(int groupId, IEnumerable<char> permissions, param | |
|
||
var db = AmbientScope.Database; | ||
|
||
//we need to batch these in groups of 2000 so we don't exceed the max 2100 limit | ||
var sql = "DELETE FROM umbracoUserGroup2NodePermission WHERE userGroupId = @groupId AND nodeId in (@nodeIds)"; | ||
foreach (var idGroup in entityIds.InGroupsOf(2000)) | ||
foreach (var group in entityIds.InGroupsOf(Constants.Sql.MaxParameterCount)) | ||
{ | ||
db.Execute(sql, new { groupId, nodeIds = idGroup }); | ||
db.Execute(sql, new { groupId, nodeIds = group }); | ||
} | ||
|
||
var toInsert = new List<UserGroup2NodePermissionDto>(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,8 +37,9 @@ protected override IRedirectUrl PerformGet(Guid id) | |
|
||
protected override IEnumerable<IRedirectUrl> PerformGetAll(params Guid[] ids) | ||
{ | ||
if (ids.Length > 2000) | ||
throw new NotSupportedException("This repository does not support more than 2000 ids."); | ||
if (ids.Length > Constants.Sql.MaxParameterCount) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can also fix this by using grouping/batching. |
||
throw new NotSupportedException($"This repository does not support more than {Constants.Sql.MaxParameterCount} ids."); | ||
|
||
var sql = GetBaseQuery(false).WhereIn<RedirectUrlDto>(x => x.Id, ids); | ||
var dtos = Database.Fetch<RedirectUrlDto>(sql); | ||
return dtos.WhereNotNull().Select(Map); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -186,17 +186,17 @@ public IEnumerable<TEntity> GetMany(params TId[] ids) | |
|
||
// can't query more than 2000 ids at a time... but if someone is really querying 2000+ entities, | ||
// the additional overhead of fetching them in groups is minimal compared to the lookup time of each group | ||
const int maxParams = 2000; | ||
if (ids.Length <= maxParams) | ||
if (ids.Length <= Constants.Sql.MaxParameterCount) | ||
Comment on lines
187
to
+189
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is a bit confusing, as this is probably not done to prevent the overhead of grouping, but to return all entities in case |
||
{ | ||
return CachePolicy.GetAll(ids, PerformGetAll); | ||
} | ||
|
||
var entities = new List<TEntity>(); | ||
foreach (var groupOfIds in ids.InGroupsOf(maxParams)) | ||
foreach (var group in ids.InGroupsOf(Constants.Sql.MaxParameterCount)) | ||
{ | ||
entities.AddRange(CachePolicy.GetAll(groupOfIds.ToArray(), PerformGetAll)); | ||
entities.AddRange(CachePolicy.GetAll(group.ToArray(), PerformGetAll)); | ||
} | ||
|
||
return entities; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might be able to rewrite this to support more property/content types by introducing the grouping/batching, but that will require more testing (something for another PR).