(#195) Update to Elasticsearch 8#200
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.
9fa9997 to
86b8c9d
Compare
e7eb50b to
f81c863
Compare
fd37031 to
4700f4d
Compare
1479895 to
3ef493f
Compare
3ef493f to
444d84c
Compare
- Replace NEST client with Elastic.Clients.Elasticsearch.
- Migrate from Newtonsoft.Json to System.Text.Json.
- Refactored serialization:
- Deserializing from Elasticsearch to POCO uses JsonNamingPolicy.SnakeCaseLower via
NCI.OCPL.Api.Common NuGet component.
- Serializing from POCO to the wire uses default .Net naming policy with overrides
as needed via custom JsonConverter implmentations.
- Model classes migrated from NEST mapping attributes ([Keyword], [Number], [Nested])
to custom JsonConverters.
- Tests:
- Test mocking migrated to TestingElasticsearchClientSettingsFactory in order
to use the same ElasticsearchClientSettings initialization as executable code.
- Add xunit.analyzers package to allow xunit to properly discover test objects
with JsonNode properties.
- Modify expected query structure JSON files to match ES 8 serialization changes
(e.g. arrays with only one element become objects).
- Switch tests from async void to async Task.
- Remove unsupported (garbage) fields from test data files.
- Add test descriptions.
- Project file updates
- Target framework corrected from netcoreapp8.0 to net8.0.
- Bump NCI.OCPL.Api.Common packages to 4.0.0.
- Remove explicit NSwag dependency
- Miscellaneous
- Cleaned up unused using statements across many files.
- Update location for NIH TLS certificates.
- Correct typos.
Closes #195
444d84c to
eb8da1f
Compare
Replace NEST client with Elastic.Clients.Elasticsearch.
Migrate from Newtonsoft.Json to System.Text.Json.
Refactored serialization:
NCI.OCPL.Api.Common NuGet component.
as needed via custom JsonConverter implmentations.
to custom JsonConverters.
Tests:
to use the same ElasticsearchClientSettings initialization as executable code.
(e.g. arrays with only one element become objects).
Project file updates
Miscellaneous
Closes #195