EES-5763 Fetch geog lvls separately so LINQ can translate query #5503
Conversation
| // We cannot fetch results[x].Meta.GeographicLevels in the previous query, because of the JOIN in | ||
| // `JoinFreeText`. That JOIN means we cannot fetch any collection, as that means we don't get a one-to-one | ||
| // matching of search ranks with the results after filtering. So instead we fetch the geographic levels | ||
| // in a separate query below | ||
| var geogLvlsDict = contentDbContext.Files | ||
| .AsNoTracking() | ||
| .Include(f => f.DataSetFileVersionGeographicLevels) | ||
| .Where(file => results | ||
| .Select(r => r.FileId).ToList() | ||
| .Contains(file.Id)) | ||
| .ToDictionary( | ||
| file => file.Id, | ||
| file => file.DataSetFileVersionGeographicLevels | ||
| .Select(gl => gl.GeographicLevel.GetEnumLabel()) | ||
| .Order() | ||
| .ToList()); | ||
| foreach (var result in results) | ||
| { | ||
| result.Meta.GeographicLevels = geogLvlsDict[result.FileId]; | ||
| } |
There was a problem hiding this comment.
Looks good to me.
Did you try using ToDictionaryAsync and also try without the Include? I would have thought the Include is not necessary unless your working with DataSetFileVersionGeographicLevels client side to perform something after its been executed.
Is there any concern about ordering the levels to a specific order, so, for example, 'National' always appears first?
I think I would have made both methods which call BuildDataSetFileSummaryViewModel use a separate query to avoid the includeGeographicLevels flag, but I think that's just personal preference.
There was a problem hiding this comment.
Did you try using ToDictionaryAsync
I've switched it to use the Async method.
and also try without the Include?
The include is required. I actually had it without the include initially and it didn't work: the dictionary values would always be an empty list. The include does make sense with hindsight as we are including DataSetFileVersionGeographicLevels in the result.
I think I would have made both methods which call BuildDataSetFileSummaryViewModel use a separate query to avoid the includeGeographicLevels flag, but I think that's just personal preference.
BuildDataSetFileSummaryViewModel is already only called in one place. I added the flag because it struck me as incredibly easy for future dev to miss how it doesn't include geographic levels and I didn't want to leave such a big gotcha laying around. The only reason I added the flag was to make this clearer.
There was a problem hiding this comment.
Sorry missed the order comment
Is there any concern about ordering the levels to a specific order, so, for example, 'National' always appears first?
I have added Order so they should appear in a consistent order - I was a bit on the fence about whether to bother adding it. Are you suggesting ordering them according to how they're ordered in GeographicLevels.cs perhaps?
There was a problem hiding this comment.
BuildDataSetFileSummaryViewModel is already only called in one place. I added the flag because it struck me as incredibly easy for future dev to miss how it doesn't include geographic levels and I didn't want to leave such a big gotcha laying around. The only reason I added the flag was to make this clearer.
Ok, I guess that led me to think that it was being used by GetDataSetFile with a param of true still, but I see now that it's BuildDataSetFileMetaViewModel which is the common method used between both of them.
In that case then I think that
includeGeographicLevels
? result.Value.File.DataSetFileVersionGeographicLevels
: new List<DataSetFileVersionGeographicLevel>(),is still a bit strange given that we know it won't work if includeGeographicLevels is true, but I see where you are coming from with highlighting from the calling method that it doesn't include them.
There was a problem hiding this comment.
I have added Order so they should appear in a consistent order - I was a bit on the fence about whether to bother adding it. Are you suggesting ordering them according to how they're ordered in GeographicLevels.cs perhaps?
Alphabetical order makes sense. I was thinking we might have a requirement to list them in a particular order, like National, Regional, Local Authority, but I see they are in alphabetical order elsewhere such as data guidance, so I think this is ok.
This PR solves an issue where the updated ListDataSetFiles query wouldn't work when you include a
searchTerm.We believe this occurs because fetching DataSetFileGeographicLevels returns multiple results per data set, and so when this is joined with the free text rank table, there are multiple rows for each data set and that leads to issues. This is why HavingGeographicLevel didn't cause an issue, because even though it joins to DataSetFileGeographicLevels, the
Anymeans it doesn't return multiple rows.Credit goes to Ben for the sleuthing.
The solution we've decided on is to fetch the geographic levels in a separate query afterwards.
We could consider simplifying the ListDataSetFiles in the future by having one query to identify which data sets to return - i.e. could just return ids - and then a separate query to fetch the needed data for those data sets. Although not sure if that's actually worthwhile...