Skip to content

Conversation

@nguyenduy
Copy link
Contributor

@nguyenduy nguyenduy commented Dec 20, 2024

Summary by Sourcery

Initialize search clients during application startup to ensure they are available when needed.

Bug Fixes:

  • Fixed an issue where the search index would break if it did not initially exist.

Enhancements:

  • Improved error handling during the initialization of Azure credentials.

@nguyenduy nguyenduy requested a review from gidich as a code owner December 20, 2024 17:37
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Dec 20, 2024

Reviewer's Guide by Sourcery

This PR reverts some AI suggestions that caused breaks if the search index did not initially exist. It modifies the Azure Cognitive Search implementation to handle missing indexes more gracefully and updates the InfrastructureServicesBuilder to initialize search clients asynchronously.

Sequence diagram for updated Azure Cognitive Search initialization

sequenceDiagram
    participant App
    participant Builder as InfrastructureServicesBuilder
    participant Search as AzCognitiveSearch
    participant Azure as Azure Services

    App->>Builder: initialize()
    activate Builder
    Builder->>Search: new AzCognitiveSearch(endpoint)
    activate Search
    Search->>Azure: create DefaultAzureCredential
    Search->>Azure: create SearchIndexClient
    Search-->>Builder: return instance
    deactivate Search
    Builder->>Search: initializeSearchClients()
    activate Search
    Search->>Azure: listIndexesNames()
    Azure-->>Search: existing indexes
    loop For each index
        Search->>Search: set searchClient in Map
    end
    Search-->>Builder: void
    deactivate Search
    Builder-->>App: void
    deactivate Builder
Loading

Class diagram for updated Cognitive Search interfaces

classDiagram
    class CognitiveSearchBase {
        <<interface>>
        +createIndex(indexDefinition: SearchIndex)
        +createOrUpdateIndex(indexName: string, indexDefinition: SearchIndex)
        +search(indexName: string, searchText: string, options?: any)
        +deleteDocument(indexName: string, document: any)
        +indexDocument(indexName: string, document: any)
        +deleteIndex(indexName: string)
        +indexExists(indexName: string) boolean
        +initializeSearchClients() Promise~void~
    }

    class AzCognitiveSearch {
        -client: SearchIndexClient
        -searchClients: Map
        +constructor(endpoint: string)
        +initializeSearchClients()
        +indexExists(indexName: string)
        +createIndex(indexDefinition: SearchIndex)
        +search(indexName: string, searchText: string, options?: any)
    }

    CognitiveSearchBase <|.. AzCognitiveSearch

    note for AzCognitiveSearch "Modified to handle missing indexes
and initialize clients asynchronously"
Loading

File-Level Changes

Change Details Files
Improved handling of missing search indexes in Azure Cognitive Search implementation
  • Added error handling for DefaultAzureCredential initialization to provide more informative error messages.
  • Introduced an initializeSearchClients method to populate search clients based on existing indexes.
  • Modified indexExists method to check for the presence of the index in the searchClients map.
  • Updated error handling in createOrUpdateIndex, createIndex, updateIndex, search, and deleteDocument methods to include more context about the error.
  • Changed the constructor to handle credential initialization errors and throw a more descriptive error message if credentials are not configured correctly.
  • Added a try-catch block around the initialization of the search client to handle potential errors during the process.
  • Modified the indexExists method to synchronously check if the index exists in the searchClients map.
  • Updated the createIndex method to throw a more informative error message if index creation fails.
  • Updated the updateIndex method to throw a more informative error message if index update fails.
  • Updated the search method to use the search client from the searchClients map.
  • Updated the deleteDocument method to throw a more informative error message if document deletion fails.
data-access/seedwork/services-seedwork-cognitive-search-az/index.ts
Asynchronous initialization of search clients in InfrastructureServicesBuilder
  • Made the initialize method asynchronous and added a call to initializeSearchClients on the cognitiveSearch instance.
  • Added an initializeSearchClients method to the IMemoryCognitiveSearch interface and the MemoryCognitiveSearch implementation.
  • Added an initializeSearchClients method to the CognitiveSearchBase interface.
  • Updated the initializeFunctionApp function to await the initialization of the InfrastructureServicesBuilder.
  • Removed unnecessary blank lines and fixed a minor formatting issue.
  • Added an empty implementation for the initializeSearchClients method in the MemoryCognitiveSearch class.
  • Changed the indexExists method signature to return a boolean instead of a Promise in the IMemoryCognitiveSearch and CognitiveSearchBase interfaces and their implementations.
  • Updated the initialize method to asynchronously initialize the search clients.
data-access/src/infrastructure-services-impl/infrastructure-services-builder.ts
data-access/seedwork/services-seedwork-cognitive-search-in-memory/index.ts
data-access/seedwork/services-seedwork-cognitive-search-interfaces/index.ts
data-access/src/functions/core/initialize-function-app.ts

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@nguyenduy nguyenduy requested review from nnoce14 and removed request for gidich December 20, 2024 17:37
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @nguyenduy - I've reviewed your changes - here's some feedback:

Overall Comments:

  • The change from async indexExists() to sync indexExists() is a breaking change that could affect existing code. Consider either keeping the async version for backward compatibility or documenting this change prominently for users of the library.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Review instructions: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@Nwachimereze1-hub
Copy link

gp

@Nwachimereze1-hub
Copy link

diff

Choose a reason for hiding this comment

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

gp
diff

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.

AI suggestion breaks search if search index initially does not exist

2 participants