Skip to content

Commit

Permalink
Fix potential multiple enumerations (microsoft#742)
Browse files Browse the repository at this point in the history
There are more of these in unit tests and samples, where I think it's
not a big deal. The PR fixes code in the SDK (one false positive simply
resolved using ICollection) and code that could be copied into other
projects.

Co-authored-by: Adrian Bonar <56417140+adrianwyatt@users.noreply.github.com>
  • Loading branch information
2 people authored and awharrison-28 committed May 1, 2023
1 parent b900c46 commit ac4963a
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 18 deletions.
2 changes: 1 addition & 1 deletion dotnet/src/SemanticKernel/Memory/VolatileMemoryStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public Task RemoveBatchAsync(string collectionName, IEnumerable<string> keys, Ca
return AsyncEnumerable.Empty<(MemoryRecord, double)>();
}

IEnumerable<MemoryRecord>? embeddingCollection = null;
ICollection<MemoryRecord>? embeddingCollection = null;
if (this.TryGetCollection(collectionName, out var collectionDict))
{
embeddingCollection = collectionDict.Values;
Expand Down
9 changes: 6 additions & 3 deletions samples/apps/copilot-chat-app/importdocument/Program.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft. All rights reserved.

using System;
using System.Collections.Generic;
using System.CommandLine;
using System.IO;
using System.Linq;
Expand Down Expand Up @@ -67,14 +68,16 @@ public static void Main(string[] args)
.WithRedirectUri(config.RedirectUri)
.Build();
var result = await app.AcquireTokenInteractive(scopes).ExecuteAsync();
var accounts = await app.GetAccountsAsync();
if (!accounts.Any())
IEnumerable<IAccount>? accounts = await app.GetAccountsAsync();
IAccount? first = accounts.FirstOrDefault();

if (first is null)
{
Console.WriteLine("Error: No accounts found");
return null;
}

return accounts.First().HomeAccountId.Identifier;
return first.HomeAccountId.Identifier;
}
catch (Exception ex) when (ex is MsalServiceException || ex is MsalClientException)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,22 +130,16 @@ public async Task<IActionResult> GetAllChatSessionsAsync(string userId)
[FromQuery] int startIdx = 0,
[FromQuery] int count = -1)
{
// TODO: the code mixes strings and Guid without being explicit about the serialization format
var chatMessages = await this._chatMessageRepository.FindByChatIdAsync(chatId.ToString());
if (chatMessages == null)
{
return this.NotFound($"No messages found for chat of id {chatId}.");
return this.NotFound($"No messages found for chat id '{chatId}'.");
}

if (startIdx >= chatMessages.Count())
{
return this.BadRequest($"Start index {startIdx} is out of range.");
}
else if (startIdx + count > chatMessages.Count() || count == -1)
{
count = chatMessages.Count() - startIdx;
}
chatMessages = chatMessages.OrderByDescending(m => m.Timestamp).Skip(startIdx);
if (count >= 0) { chatMessages = chatMessages.Take(count); }

chatMessages = chatMessages.OrderByDescending(m => m.Timestamp).Skip(startIdx).Take(count);
return this.Ok(chatMessages);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,13 @@ public Task<IEnumerable<ChatMessage>> FindByChatIdAsync(string chatId)

public async Task<ChatMessage> FindLastByChatIdAsync(string chatId)
{
var messages = await this.FindByChatIdAsync(chatId);
if (!messages.Any())
var chatMessages = await this.FindByChatIdAsync(chatId);
var first = chatMessages.MaxBy(e => e.Timestamp);
if (first is null)
{
throw new KeyNotFoundException($"No messages found for chat {chatId}.");
throw new KeyNotFoundException($"No messages found for chat '{chatId}'.");
}

return messages.OrderByDescending(e => e.Timestamp).First();
return first;
}
}

0 comments on commit ac4963a

Please sign in to comment.