(#195) Update to Elasticsearch 8#200
Open
blairlearn wants to merge 2 commits intodevelopfrom
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the Glossary API and tests to work with Elasticsearch 8 by migrating from the legacy NEST/Newtonsoft stack to Elastic.Clients.Elasticsearch and System.Text.Json, and by expanding integration-test coverage around ES queries.
Changes:
- Migrated Elasticsearch client usage to
Elastic.Clients.Elasticsearch(v8) and updated request/response test harnesses accordingly. - Replaced Newtonsoft JSON usage in tests/models with
System.Text.Json(including newJsonConverterimplementations for polymorphic arrays). - Added/updated Karate integration tests and updated the ES docker image/config for ES 8.
Reviewed changes
Copilot reviewed 122 out of 128 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/Search/Terms_Search_Request_Exact.cs | Switch expected JSON representation to JsonNode; adjust expected request shape |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/Search/Terms_Search_Request_Contains.cs | Switch expected JSON representation to JsonNode; adjust expected request shape |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/Search/Terms_Search_Request_Begins.cs | Switch expected JSON representation to JsonNode; adjust expected request shape |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/Search/Terms_Search_Request_Base.cs | Base expected-request type changed from JObject to JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/GetCount/Terms_GetCount_Request_Genetics_Patient_Spanish.cs | Switch expected JSON representation to JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/GetCount/Terms_GetCount_Request_Genetics_Patient_English.cs | Switch expected JSON representation to JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/GetCount/Terms_GetCount_Request_Genetics_HealthProfessional_Spanish.cs | Switch expected JSON representation to JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/GetCount/Terms_GetCount_Request_Genetics_HealthProfessional_English.cs | Switch expected JSON representation to JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/GetCount/Terms_GetCount_Request_CGov_Patient_Spanish.cs | Switch expected JSON representation to JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/GetCount/Terms_GetCount_Request_CGov_Patient_English.cs | Switch expected JSON representation to JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/GetCount/Terms_GetCount_Request_CGov_HealthProfessional_Spanish.cs | Switch expected JSON representation to JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/GetCount/Terms_GetCount_Request_CGov_HealthProfessional_English.cs | Switch expected JSON representation to JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/GetCount/BaseTermsQueryCountTestData.cs | Base expected-data type changed from JObject to JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/GetAll/GetAll_Request_DefaultFields.cs | Switch expected JSON representation to JsonNode; adjust expected request shape |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/GetAll/GetAll_Request_Base.cs | Base expected-request type changed from JObject to JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/GetAll/GetAll_Request_AdditionalFields.cs | Switch expected JSON representation to JsonNode; adjust expected request shape |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/Expand/ExpandRequestDefaultFields.cs | Switch expected JSON representation to JsonNode; adjust expected request shape |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/Expand/ExpandRequestBase.cs | Base expected-request type changed from JObject to JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/Expand/ExpandRequestAdditionalInfo.cs | Switch expected JSON representation to JsonNode; adjust expected request shape |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESTermsQueryServiceTestObjects/BaseTermsQueryTestData.cs | Remove unused usings |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESAutosuggestRequestTestObjects/BaseAutosuggestRequestTestData.cs | Base expected-data type changed from JObject to JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESAutosuggestRequestTestObjects/Autosuggest_Request_Genetics_Contains_Gene_English_HealthProfessional.cs | Switch expected JSON representation to JsonNode; adjust expected request shape |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESAutosuggestRequestTestObjects/Autosuggest_Request_Genetics_Begin_Dar_English_HealthProfessional.cs | Switch expected JSON representation to JsonNode; adjust expected request shape |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESAutosuggestRequestTestObjects/Autosuggest_Request_CGov_Contains_Cat_English_Patient.cs | Switch expected JSON representation to JsonNode; adjust expected request shape |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESAutosuggestRequestTestObjects/Autosuggest_Request_CGov_Contains_Ablacion_Spanish_Patient.cs | Switch expected JSON representation to JsonNode; adjust expected request shape |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/TestDataObjects/ESAutosuggestRequestTestObjects/Autosuggest_Request_CGov_Begin_Aci_Spanish_Patient.cs | Switch expected JSON representation to JsonNode; adjust expected request shape |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Services/ESTermsQuery_CountTest.cs | Update unit tests to Elastic v8 + JsonNode comparisons |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Services/ESTermsQueryServiceTest.Search.cs | Update Search tests to Elastic v8 transport + JsonNode comparisons |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Services/ESTermsQueryServiceTest.GetByName.cs | Update GetByName tests to Elastic v8 transport + JsonNode comparisons |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Services/ESTermsQueryServiceTest.GetById.cs | Update GetById tests to Elastic v8 transport |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Services/ESTermsQueryServiceTest.GetAll.cs | Update GetAll tests to Elastic v8 transport + JsonNode comparisons |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Services/ESTermsQueryServiceTest.Expand.cs | Update Expand tests to Elastic v8 transport + JsonNode comparisons |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Services/ESTermsQueryServiceTest.Common.cs | Update mock response helper from Stream to string |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Services/ESAutosuggestQueryServiceTest.cs | Update autosuggest tests to Elastic v8 transport + JsonNode comparisons |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Services/ESAutosuggestQueryServiceResponseTest.cs | Update autosuggest response tests to Elastic v8 transport |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Controllers/TermsController_Count_Test.cs | Update controller tests to async Task |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Controllers/TermsControllerTest.cs | Replace Newtonsoft JSON comparisons with System.Text.Json/JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Controllers/TermsControllerTest.Search.cs | Replace Newtonsoft JSON comparisons with System.Text.Json/JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Controllers/TermsControllerTest.GetByName.cs | Replace Newtonsoft JSON comparisons with System.Text.Json/JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Controllers/TermsControllerTest.GetAll.cs | Update controller tests to async Task |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Controllers/TermsControllerTest.Expand.cs | Replace Newtonsoft JSON comparisons with System.Text.Json/JsonNode |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Controllers/HealthCheckControllerTests.cs | Update controller tests to async Task |
| test/NCI.OCPL.Api.Glossary.Tests/Tests/Controllers/AutosuggestControllerTest.cs | Update controller tests to async Task |
| test/NCI.OCPL.Api.Glossary.Tests/TestData/ESTermsQueryData/Search/search_response_results.json | ES response fixture update (remove fields array) |
| test/NCI.OCPL.Api.Glossary.Tests/TestData/ESTermsQueryData/Search/search_response_noresults.json | ES response fixture update (remove fields array) |
| test/NCI.OCPL.Api.Glossary.Tests/TestData/ESTermsQueryData/GetByName/s-phase-fraction.json | ES response fixture update (remove fields array) |
| test/NCI.OCPL.Api.Glossary.Tests/TestData/ESTermsQueryData/GetByName/s-1.json | ES response fixture update (remove fields array) |
| test/NCI.OCPL.Api.Glossary.Tests/TestData/ESTermsQueryData/GetByName/getbyname_multiplehits.json | ES response fixture update (remove fields array) |
| test/NCI.OCPL.Api.Glossary.Tests/TestData/ESTermsQueryData/GetAll/getall_response_results.json | ES response fixture update (remove fields array) |
| test/NCI.OCPL.Api.Glossary.Tests/TestData/ESTermsQueryData/GetAll/getall_response_noresults.json | ES response fixture update (remove fields array) |
| test/NCI.OCPL.Api.Glossary.Tests/TestData/ESTermsQueryData/Expand/expand_response_results.json | ES response fixture update (remove fields array) |
| test/NCI.OCPL.Api.Glossary.Tests/TestData/ESTermsQueryData/Expand/expand_response_noresults.json | ES response fixture update (remove fields array) |
| test/NCI.OCPL.Api.Glossary.Tests/NCI.OCPL.Api.Glossary.Tests.csproj | Target framework + dependency updates for .NET 8 and new common/testing packages |
| src/NCI.OCPL.Api.Glossary/Services/ESAutosuggestQueryService.cs | Migrate autosuggest service to Elastic.Clients.Elasticsearch v8 query DSL |
| src/NCI.OCPL.Api.Glossary/NCI.OCPL.Api.Glossary.csproj | Target framework + dependency updates (Elastic v8, Common v4 alpha) |
| src/NCI.OCPL.Api.Glossary/Models/Video.cs | Migrate model JSON attributes/enums to System.Text.Json |
| src/NCI.OCPL.Api.Glossary/Models/TermOtherLanguage.cs | Remove NEST/Newtonsoft attributes; move to System.Text.Json |
| src/NCI.OCPL.Api.Glossary/Models/Suggestion.cs | Remove NEST attributes; model cleanup for new serializer |
| src/NCI.OCPL.Api.Glossary/Models/RelatedResourceJsonConverter.cs | New System.Text.Json converter for polymorphic related-resources array |
| src/NCI.OCPL.Api.Glossary/Models/README.md | New documentation for serialization conventions |
| src/NCI.OCPL.Api.Glossary/Models/Pronunciation.cs | Remove NEST attributes; move to System.Text.Json |
| src/NCI.OCPL.Api.Glossary/Models/MediaJsonConverter.cs | New System.Text.Json converter for polymorphic media array |
| src/NCI.OCPL.Api.Glossary/Models/MatchType.cs | Enum converter migration to System.Text.Json |
| src/NCI.OCPL.Api.Glossary/Models/LinkResource.cs | Enum converter migration to System.Text.Json; cleanup |
| src/NCI.OCPL.Api.Glossary/Models/ImageSource.cs | Remove NEST attributes; move to System.Text.Json |
| src/NCI.OCPL.Api.Glossary/Models/Image.cs | Enum converter migration to System.Text.Json; cleanup |
| src/NCI.OCPL.Api.Glossary/Models/GlossaryTerm.cs | Swap Json.NET item converters for System.Text.Json converters on arrays |
| src/NCI.OCPL.Api.Glossary/Models/GlossaryResource.cs | Enum converter migration to System.Text.Json; cleanup |
| src/NCI.OCPL.Api.Glossary/Models/Definition.cs | Remove NEST attributes; move to System.Text.Json |
| src/NCI.OCPL.Api.Glossary/Models/AudienceType.cs | Enum converter migration to System.Text.Json |
| integration-tests/queries/search.feature | New Karate feature for ES search query verification |
| integration-tests/queries/search.expected/*.json | New Karate expected outputs for ES search scenarios |
| integration-tests/queries/getby-id.feature | New Karate feature for ES get-by-id query verification |
| integration-tests/queries/getby-id.expected/*.json | New Karate expected outputs for ES get-by-id scenarios |
| integration-tests/queries/get-with-fallback.feature | New Karate feature for fallback search query verification |
| integration-tests/queries/get-with-fallback.expected/*.json | New Karate expected outputs for fallback scenarios |
| integration-tests/queries/get-pretty-url.feature | New Karate feature for pretty-url query verification |
| integration-tests/queries/get-pretty-url.expected/*.expected | New Karate expected outputs for pretty-url scenarios |
| integration-tests/queries/get-count.feature | New Karate feature for ES _count query verification |
| integration-tests/queries/get-all.feature | New Karate feature for ES get-all query verification |
| integration-tests/queries/expand.feature | New Karate feature for ES expand query verification |
| integration-tests/queries/autosuggest.feature | New Karate feature for ES autosuggest query verification |
| integration-tests/queries/autosuggest.expected/*.json | New Karate expected outputs for autosuggest scenarios |
| integration-tests/docker-glossary-api/elasticsearch/Dockerfile | Upgrade ES docker image to 8.19.12; cert install adjustments |
| integration-tests/docker-glossary-api/docker-compose.yml | Disable xpack security for local/integration testing |
| integration-tests/bin/logback.xml | Adjust Karate log output path |
| integration-tests/bin/karate-config.js | Add esHost configuration (env override supported) |
| .gitignore | Reorganize custom ignores; ignore .vscode/ directory |
| .github/copilot-instructions.md | New repo conventions doc (Elastic v8 + System.Text.Json + using-ordering) |
Comments suppressed due to low confidence (1)
src/NCI.OCPL.Api.Glossary/Services/ESAutosuggestQueryService.cs:99
- The error message "errors occured" has a spelling mistake and is also very generic for clients diagnosing failures. Consider fixing to "errors occurred" and including a more actionable message (or at least the failing operation context).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 104 out of 107 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (3)
src/NCI.OCPL.Api.Glossary/Services/ESTermsQueryService.cs:162
- On invalid ES responses, the code logs a detailed
msgbut throwsAPIInternalException("errors occured"), which is not actionable for callers and is misspelled. Consider throwing with the detailed message (or a sanitized but specific message) and standardize on "errors occurred".
string msg = $"Invalid Elasticsearch response for dictionary '{dictionary}', audience '{audience}', language '{language}', pretty URL name '{prettyUrlName}'."
.Replace(Environment.NewLine, String.Empty)
+ $"\n\n{response.DebugInformation}";
_logger.LogError(msg);
throw new APIInternalException("errors occured");
src/NCI.OCPL.Api.Glossary/Services/ESTermsQueryService.cs:243
- This branch throws
new APIErrorException(500, "errors occured"), which is both misspelled and not helpful for debugging (especially since a detailedmsgis already constructed/logged). Prefer using the detailed message (or a consistent, user-safe message) and correct to "errors occurred"; also consider updating the other identical throws in this file for consistency.
String msg = $"Invalid response when getting dictionary '{dictionary}', audience '{audience}', language '{language}', size '{size}', from '{from}'."
.Replace(Environment.NewLine, String.Empty);
_logger.LogError(msg);
throw new APIErrorException(500, "errors occured");
}
src/NCI.OCPL.Api.Glossary/Services/ESAutosuggestQueryService.cs:101
- This error path logs a detailed message but throws
APIErrorException(500, "errors occured"), which is both misspelled and not actionable for callers. Prefer throwing with the detailed message (or a consistent, user-safe message) and standardize on "errors occurred".
string msg = $"Invalid response when searching for dictionary '{dictionary}', audience '{audience}', language '{language}', query '{searchText}', contains '{matchType}', size '{size}'."
.Replace(Environment.NewLine, String.Empty);
_logger.LogError(msg);
throw new APIErrorException(500, "errors occured");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 106 out of 109 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 106 out of 109 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 108 out of 112 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
blairlearn
force-pushed
the
ticket/195-es8
branch
3 times, most recently
from
9fa9997 to
86b8c9d
Compare
blairlearn
force-pushed
the
ticket/195-es8
branch
from
86b8c9d to
e7eb50b
Compare
1. Elasticsearch Client Migration (NEST --> Elastic.Clients.Elasticsearch)
- Replaces IElasticClient with ElasticsearchClient throughout services and tests.
- Rewrites all search queries from NEST's QueryContainerDescriptor<T> fluent API to the new
QueryDescriptor fluent API (e.g., ESAutosuggestQueryService, ESSearchQueryService, and
all four query builders for English/Spanish CGov and Doc searches).
- Updates response handling:
response.IsValid --> response.IsValidResponse
ClusterHealthResponse --> HealthResponse
2. JSON Serialization Migration (Newtonsoft.Json → System.Text.Json)
- Removes Newtonsoft.Json / NEST.JsonNetSerializer dependencies.
Replaces JsonConvert/JObject/JToken usage with System.Text.Json equivalents
(JsonSerializer, JsonDocument, JsonNode).
- Model classes migrated from NEST mapping attributes ([Keyword], [Number], [Nested]).
- MediaJsonConverter and RelatedResourceJsonConverter updated.
3. Project file updates
- Target framework corrected from netcoreapp8.0 to net8.0.
- NEST 7.9.* --> Elastic.Clients.Elasticsearch 8.19.12.
- NCI.OCPL.Api.Common updated from 3.0.1 to 4.0.0.
- Explicit NSwag dependency removed (packaged pulled in via shared components).
4. Test cleanup
- Test mocking migrated to TestingElasticsearchClientSettingsFactory in order
to use the same ElasticsearchClientSettings initialization as executable code.
- JTokenEqualityComparer assertions replaced with JsonNode.DeepEquals.
- Expected query structure JSON files updated to match ES 8 serialization differences
(e.g. single-element arrays become objects).
- Test methodds changed from async void to async Task.
- Add test comments.
5. Add instruction files for Copilot.
Closes #195
blairlearn
force-pushed
the
ticket/195-es8
branch
from
e7eb50b to
f81c863
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Many comments to come.