Skip to content

Conversation

otemnov
Copy link
Contributor

@otemnov otemnov commented Sep 8, 2025

Description:

The change creates indexes if an index with the same key pattern does not already exist.
This ensures backward compatibility with previous versions that created indexes with different names but the same key pattern.

Affected components:

  • IStorageInitializer.MongoDB.cs

@yang-xiaodong
Copy link
Member

Please explain:

  1. Why remove StatusName?
  2. Why need CreateIndexeSifKeyPatternMissingaSync? The CreateManyAsync can be called many times without side effects.

@otemnov
Copy link
Contributor Author

otemnov commented Sep 11, 2025

@yang-xiaodong

  1. StatusName because we have compound index StatusName + ExpiresAt. It will be efficiently used if you filter just by StatusName.
  2. That's true, CreateManyAsync does not fail if the index definition is the same. However, it does fail if an index with the same fields and order but a different name already exists.
    Previous versions of the library created indexes with specified names that did not match the default naming pattern. Reference: 7353d32.
    In current version name of the index is not specified, so it will try to create index with default name pattern.
    This leads to a runtime exception because MongoDB fails due to an index conflict.

@yang-xiaodong yang-xiaodong requested review from Copilot and demorgi and removed request for Copilot September 15, 2025 14:17
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements safe MongoDB index creation to ensure backward compatibility with previous versions that may have created indexes with different names but identical key patterns. The change prevents duplicate indexes with the same key pattern from being created.

  • Replaces direct CreateManyAsync calls with a new helper method that checks for existing index key patterns
  • Removes duplicate StatusName index entries from both published and received message index arrays
  • Adds logic to compare index key patterns rather than index names when determining if an index already exists
Comments suppressed due to low confidence (2)

src/DotNetCore.CAP.MongoDB/IStorageInitializer.MongoDB.cs:1

  • The standalone StatusName index entries are removed, but there's still a composite index StatusName.Ascending(x => x.ExpiresAt) on lines 105 and 123. This removal may break existing queries that rely on the standalone StatusName index for optimal performance.
// Copyright (c) .NET Core Community. All rights reserved.

src/DotNetCore.CAP.MongoDB/IStorageInitializer.MongoDB.cs:1

  • The standalone StatusName index entries are removed, but there's still a composite index StatusName.Ascending(x => x.ExpiresAt) on lines 105 and 123. This removal may break existing queries that rely on the standalone StatusName index for optimal performance.
// Copyright (c) .NET Core Community. All rights reserved.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

var indexes = await indexList.ToListAsync(cancellationToken);

var existingIndexes = new HashSet<string>(
indexes.Select(i => GetIndexKeyPattern(i["key"].AsBsonDocument))
Copy link
Preview

Copilot AI Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessing i["key"] without checking if the key exists could throw a KeyNotFoundException if an index document doesn't have a 'key' field. Add a null check or use TryGetValue to handle malformed index documents safely.

Suggested change
indexes.Select(i => GetIndexKeyPattern(i["key"].AsBsonDocument))
indexes
.Select(i => i.TryGetValue("key", out var keyValue) && keyValue != null
? GetIndexKeyPattern(keyValue.AsBsonDocument)
: null)
.Where(pattern => pattern != null)

Copilot uses AI. Check for mistakes.

demorgi
demorgi previously approved these changes Sep 15, 2025
@demorgi
Copy link
Contributor

demorgi commented Sep 15, 2025

@yang-xiaodong LGTM, this may ba a problem if you are not doing any index maintenance. FYI even in case there will be a runtime exception, it will not break initialization, will just log it.

@demorgi demorgi dismissed their stale review September 15, 2025 16:49

on a second thought...

@demorgi
Copy link
Contributor

demorgi commented Sep 15, 2025

@otemnov, maybe we should make it more KISS, we definitely know how old indexes were named and what they were indexing, so instead of building a pattern-based check, we can just check for old indexes and delete them before creating a new one?

@otemnov
Copy link
Contributor Author

otemnov commented Sep 16, 2025

@demorgi yes, agree. lets make it simple

@otemnov
Copy link
Contributor Author

otemnov commented Sep 18, 2025

@demorgi Closing this PR and opening a new one. #1742

@otemnov otemnov closed this Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants