Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: or-1894 return bad request instead of server error when scroll timed out #1246

Merged
merged 1 commit into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/OrganisationRegistry.Api/ElasticSearchFacade.cs
Original file line number Diff line number Diff line change
Expand Up @@ -286,9 +286,14 @@ public IActionResult BuildApiSearchResult<T>(ISearchResponse<T> searchResults) w

if (!searchResults.IsValid)
{
_logger.LogCritical("Elasticsearch error occured on search! {Error}", searchResults.FormatError());
const string logMessage = "Er is een probleem opgetreden bij het uitvoeren van de zoekopdracht.";

throw new ApiException("Er is een probleem opgetreden bij het uitvoeren van de zoekopdracht.");
_logger.LogCritical(logMessage + " {Error}", searchResults.FormatError());

throw searchResults.ServerError.Error.Type.Equals("search_phase_execution_exception")
// throw searchResults.Hits.Count.Equals(0) && string.IsNullOrEmpty(searchResults.ScrollId) // Parameters for identifying a timed out scroll
? new ElasticsearchScrollTimeoutException(logMessage)
: new ApiException(logMessage);
}

_httpContext.Response.AddElasticsearchMetaDataResponse(new ElasticsearchMetaData<T>(searchResults));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
namespace OrganisationRegistry.Api.Infrastructure;

using System;

public class ElasticsearchScrollTimeoutException : DomainException
{
public ElasticsearchScrollTimeoutException()
{
}

public ElasticsearchScrollTimeoutException(string message) : base(message)
{
}

public ElasticsearchScrollTimeoutException(string message, Exception inner) : base(message, inner)
{
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
namespace OrganisationRegistry.ElasticSearch.Tests;

using FluentAssertions;
using Infrastructure.Events;
using Microsoft.Extensions.Logging;
using Projections.Infrastructure;
using Scenario;
using Xunit;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Api;
using AutoFixture;
using Location;
using Location.Events;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging.Abstractions;
using Moq;
using OpenSearch.Net;
using Organisation.Events;
using OrganisationRegistry.Tests.Shared;
using OrganisationRegistry.Tests.Shared.Stubs;
using Organisations;
using Osc;
using Projections.Organisations;
using Scenario.Specimen;
using SqlServer.Infrastructure;

[Collection(nameof(ElasticSearchFixture))]
public class OrganisationSearchTests
{
private readonly ElasticSearchFixture _fixture;
private readonly TestEventProcessor _eventProcessor;

Check warning on line 36 in test/OrganisationRegistry.ElasticSearch.Tests/OrganisationSearchTests.cs

View workflow job for this annotation

GitHub Actions / Run Tests (test/OrganisationRegistry.ElasticSearch.Tests)

The field 'OrganisationSearchTests._eventProcessor' is never used
private readonly LocationUpdated? lastLocationEvent;

Check warning on line 37 in test/OrganisationRegistry.ElasticSearch.Tests/OrganisationSearchTests.cs

View workflow job for this annotation

GitHub Actions / Run Tests (test/OrganisationRegistry.ElasticSearch.Tests)

The field 'OrganisationSearchTests.lastLocationEvent' is never used

public OrganisationSearchTests(ElasticSearchFixture fixture)

Check warning on line 39 in test/OrganisationRegistry.ElasticSearch.Tests/OrganisationSearchTests.cs

View workflow job for this annotation

GitHub Actions / Run Tests (test/OrganisationRegistry.ElasticSearch.Tests)

Non-nullable field '_eventProcessor' must contain a non-null value when exiting constructor. Consider declaring the field as nullable.
{
_fixture = fixture;
}

// [Fact]
[Fact(Skip = "Manual only")]
public async Task ElasticSearchClient_Search_With_Scroll_Timedout_Does_Not_Throw_Exception()
{
var client = _fixture.Elastic.ReadClient;
var indexName = "scroll-timeout-test-index";

try
{
var indexResponse = await _fixture.Elastic.WriteClient.Indices.CreateAsync(indexName);

var scrollTimeout = "1s";
var scroll = await client.SearchAsync<OrganisationDocument>(
s => s
.Index(indexResponse.Index)
.From(0)
.Size(5)
.Scroll(scrollTimeout));

await Task.Delay(5000);
scroll = await client.ScrollAsync<OrganisationDocument>(scrollTimeout, scroll.ScrollId);

Assert.Equal(scroll.ServerError.Error.Type, "search_phase_execution_exception");

Check warning on line 66 in test/OrganisationRegistry.ElasticSearch.Tests/OrganisationSearchTests.cs

View workflow job for this annotation

GitHub Actions / Run Tests (test/OrganisationRegistry.ElasticSearch.Tests)

The literal or constant value "search_phase_execution_exception" should be passed as the 'expected' argument in the call to 'Assert.Equal(expected, actual)' in method 'ElasticSearchClient_Search_With_Scroll_Timedout_Does_Not_Throw_Exception' on type 'OrganisationSearchTests'.
}
finally
{
await _fixture.Elastic.WriteClient.Indices.DeleteAsync(indexName);
}
}
}
Loading