Skip to content

Commit

Permalink
[ODS-5174] Total count and paging is off when API Client is associate…
Browse files Browse the repository at this point in the history
…d with Edorgs at multiple level (#392)

* Fix all problems related to duplicates in counts and paged data.

* Provided strongly-typed expression-based projection for the CountDistinct to avoid forcing a downstream conversions from string to int on final result processing.

* Fixed typo in variable name.

* Renamed GetKeyColumnProjections to GetCountColumnProjections and converted to local function for clarity of intent and scope of usage.

* Renamed method and added comments for better clarity.

* Change all usages of the total count from long to int.
  • Loading branch information
gmcelhanon authored Nov 8, 2021
1 parent 9b630c5 commit ef15092
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ async Task<SpecificationResult> GetPagedAggregateIdsAsync()
var idQueryCriteria = _pagedAggregateIdsCriteriaProvider.GetCriteriaQuery(specification, queryParameters);
SetChangeQueriesCriteria(idQueryCriteria);

queryBatch.Add<Guid>(idQueryCriteria);
queryBatch.Add<object[]>(idQueryCriteria);
}

// If requested, get a total count of available records
Expand All @@ -100,17 +100,19 @@ async Task<SpecificationResult> GetPagedAggregateIdsAsync()
var countQueryCriteria = _totalCountCriteriaProvider.GetCriteriaQuery(specification, queryParameters);
SetChangeQueriesCriteria(countQueryCriteria);

queryBatch.Add<long>(countQueryCriteria);
queryBatch.Add<int>(countQueryCriteria);
}

int resultIndex = 0;

var ids = ItemsRequested()
? await queryBatch.GetResultAsync<Guid>(resultIndex++, cancellationToken)
: new Guid[0];
? (await queryBatch.GetResultAsync<object[]>(resultIndex++, cancellationToken))
.Select(r => (Guid) r[0])
.ToArray()
: Array.Empty<Guid>();

var totalCount = CountRequested()
? (await queryBatch.GetResultAsync<long>(resultIndex, cancellationToken)).First()
? (await queryBatch.GetResultAsync<int>(resultIndex, cancellationToken)).First()
: 0;

return new SpecificationResult
Expand Down Expand Up @@ -141,7 +143,7 @@ void SetChangeQueriesCriteria(ICriteria criteria)
private class SpecificationResult
{
public IList<Guid> Ids { get; set; }
public long TotalCount { get; set; }
public int TotalCount { get; set; }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0.
// See the LICENSE and NOTICES files in the project root for more information.

using System;
using EdFi.Ods.Common.Caching;
using NHibernate;
using NHibernate.Criterion;
Expand All @@ -18,8 +19,24 @@ public class PagedAggregateIdsCriteriaProvider<TEntity> : AggregateRootCriteriaP
where TEntity : class
{
public PagedAggregateIdsCriteriaProvider(ISessionFactory sessionFactory, IDescriptorsCache descriptorsCache)
: base(sessionFactory, descriptorsCache) { }
: base(sessionFactory, descriptorsCache)
{
_identifierColumnNames = new Lazy<string[]>(
() =>
{
var persister = (AbstractEntityPersister) SessionFactory.GetClassMetadata(typeof(TEntity));
if (persister.IdentifierColumnNames != null && persister.IdentifierColumnNames.Length > 0)
{
return persister.IdentifierColumnNames;
}
return new[] { "Id" };
});
}

private readonly Lazy<string[]> _identifierColumnNames;

/// <summary>
/// Get a <see cref="NHibernate.ICriteria"/> query that retrieves the Ids for the next page of data.
/// </summary>
Expand All @@ -29,7 +46,7 @@ public PagedAggregateIdsCriteriaProvider(ISessionFactory sessionFactory, IDescri
public ICriteria GetCriteriaQuery(TEntity specification, IQueryParameters queryParameters)
{
var idQueryCriteria = Session.CreateCriteria<TEntity>("aggregateRoot")
.SetProjection(Projections.Property("Id"))
.SetProjection(Projections.Distinct(GetColumnProjectionsForDistinctWithOrderBy()))
.SetFirstResult(queryParameters.Offset ?? 0)
.SetMaxResults(queryParameters.Limit ?? 25);

Expand All @@ -42,22 +59,29 @@ public ICriteria GetCriteriaQuery(TEntity specification, IQueryParameters queryP
ProcessQueryParameters(idQueryCriteria, queryParameters);

return idQueryCriteria;
}

private void AddDefaultOrdering(ICriteria queryCriteria)
{
var persister = (AbstractEntityPersister) SessionFactory.GetClassMetadata(typeof(TEntity));

if (persister.IdentifierColumnNames != null && persister.IdentifierColumnNames.Length > 0)

IProjection GetColumnProjectionsForDistinctWithOrderBy()
{
foreach (var identifierColumnName in persister.IdentifierColumnNames)
var projections = Projections.ProjectionList();

// Add the resource identifier (this is the value we need for the secondary "page" query)
projections.Add(Projections.Property("Id"));

// Add the order by (primary key) columns (required when using DISTINCT with ORDER BY)
foreach (var identifierColumnName in _identifierColumnNames.Value)
{
queryCriteria.AddOrder(Order.Asc(identifierColumnName));
projections.Add(Projections.Property(identifierColumnName));
}

return projections;
}
else
}

private void AddDefaultOrdering(ICriteria queryCriteria)
{
foreach (var identifierColumnName in _identifierColumnNames.Value)
{
queryCriteria.AddOrder(Order.Asc("Id"));
queryCriteria.AddOrder(Order.Asc(identifierColumnName));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
// See the LICENSE and NOTICES files in the project root for more information.

using EdFi.Ods.Common.Caching;
using EdFi.Ods.Common.Models.Domain;
using NHibernate;
using NHibernate.Criterion;

Expand All @@ -14,7 +15,7 @@ namespace EdFi.Ods.Common.Providers.Criteria
/// </summary>
/// <typeparam name="TEntity">The type of the entity to which criteria is being applied.</typeparam>
public class TotalCountCriteriaProvider<TEntity> : AggregateRootCriteriaProviderBase<TEntity>, ITotalCountCriteriaProvider<TEntity>
where TEntity : class
where TEntity : AggregateRootWithCompositeKey
{
public TotalCountCriteriaProvider(ISessionFactory sessionFactory, IDescriptorsCache descriptorsCache)
: base(sessionFactory, descriptorsCache) { }
Expand All @@ -29,7 +30,7 @@ public ICriteria GetCriteriaQuery(TEntity specification, IQueryParameters queryP
{
var countQueryCriteria = Session
.CreateCriteria<TEntity>("aggregateRoot")
.SetProjection(Projections.RowCountInt64());
.SetProjection(Projections.CountDistinct<TEntity>(x => x.Id));

// Add specification-based criteria
ProcessSpecification(countQueryCriteria, specification);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public class GetBySpecificationResult<TEntity>

public class ResultMetadata
{
public long TotalCount { get; set; }
public int TotalCount { get; set; }

public string CurrentPage { get; set; }

Expand Down

0 comments on commit ef15092

Please sign in to comment.