Add function to list all indexes #1693
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| */ | ||
| public class IndexInformation { | ||
| private final String name; | ||
| private final Settings settings; |
There was a problem hiding this comment.
This class should not contain Elasticsearch classes as properties and expose them to the user. For the settings and mapping we should use a Document like it is returned from ReactiveIndexOperations.getSettings()and ReactiveIndexOperations.getMapping(). For the aliases check the ReactiveIndexOperations.getAliasesForIndex() function – the non reactive functions return Map<String, Object>, but a Document is just an extension of this.
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { |
There was a problem hiding this comment.
Do we need an equals() method for this class? If so – never override equals() without overriding hashcode() as well. But I think this is not needed.
| private final List<AliasMetadata> aliases; | ||
|
|
||
|
|
||
| public static List<IndexInformation> createList(GetIndexResponse getIndexResponse) { |
There was a problem hiding this comment.
we should not have these factory methods in this class. IndexInformation should just be a data class containing the returned information. Please move the mapping / factory code into the operations implementations, have a look at DefaultReactiveIndexOperations.getMapping() or the two non-reactive implementations as reference.
| @@ -0,0 +1,120 @@ | |||
| /* | |||
| * Copyright 2018-2020 the original author or authors. | |||
There was a problem hiding this comment.
| * Copyright 2018-2020 the original author or authors. | |
| * Copyright 2021 the original author or authors. |
| } | ||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
If we need a toString() it should contain the content of the properties as well.
| @@ -317,5 +318,7 @@ default boolean deleteTemplate(String templateName) { | |||
| */ | |||
| IndexCoordinates getIndexCoordinates(); | |||
|
|
|||
| List<IndexInformation> getInformation(); | |||
There was a problem hiding this comment.
please add Javadoc with a @since 4.2 tag
| @@ -284,5 +286,6 @@ | |||
| */ | |||
| IndexCoordinates getIndexCoordinates(); | |||
|
|
|||
| Mono<List<IndexInformation>> getInformation(); | |||
There was a problem hiding this comment.
we should rather return a Flux<IndexInformation> than a Mono of a list here.
Please add Javadoc and @since 4.2
| getIndexRequest.indices(index.getIndexNames()); | ||
|
|
||
| GetIndexResponse response = client.admin().indices().getIndex(getIndexRequest).actionGet(); | ||
|
|
There was a problem hiding this comment.
here the mapping should be done from Elasticsearch types to our data
|
|
||
| return restTemplate.execute( | ||
| client -> { | ||
| GetIndexResponse getIndexResponse = client.indices().get(request, RequestOptions.DEFAULT); |
There was a problem hiding this comment.
here the mapping should be done from Elasticsearch types to our data
| public Mono<List<IndexInformation>> getInformation() { | ||
| org.elasticsearch.client.indices.GetIndexRequest getIndexRequest = requestFactory.getIndexRequest(getIndexCoordinates()); | ||
|
|
||
| return Mono.from(operations.executeWithIndicesClient(client -> client.getIndex(HttpHeaders.EMPTY, getIndexRequest) |
There was a problem hiding this comment.
here the mapping should be done from Elasticsearch types to our data
Original pull request spring-projects#1693
sothawo
changed the title
| assertThat(indexInformation.getAliases()).isInstanceOf(Set.class); | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
You should create an index with settings, mappings and alias data and check that it is really returned (you can use existing settings and mappings definition, must not really match the Entity):
@SpringIntegrationTest
@ContextConfiguration(classes = { ElasticsearchRestTemplateConfiguration.class })
public class IndexOperationTests {
public static final String INDEX_NAME = "random-index-name";
@Autowired ElasticsearchOperations operations;
@Test // #1646
void shouldReturnInformationList() {
String indexName = INDEX_NAME;
IndexOperations indexOps = operations.indexOps(Entity.class);
indexOps.create();
indexOps.putMapping();
indexOps.alias(new AliasActions(
new AliasAction.Add(AliasActionParameters.builder().withIndices(INDEX_NAME).withAliases("alias").build())));
List<IndexInformation> indexInformationList = indexOps.getInformation();
assertThat(indexInformationList.size()).isEqualTo(1);
IndexInformation indexInformation = indexInformationList.get(0);
// !!!!!!
// TODO: check the returned content here
assertThat(indexInformation).isNotNull();
assertThat(indexInformation.getName()).isNotNull();
assertThat(indexInformation.getName()).isEqualTo(indexName);
assertThat(indexInformation.getMappings())
.isInstanceOf(org.springframework.data.elasticsearch.core.document.Document.class);
assertThat(indexInformation.getSettings())
.isInstanceOf(org.springframework.data.elasticsearch.core.document.Document.class);
assertThat(indexInformation.getAliases()).isInstanceOf(Set.class);
}
@Data
@Document(indexName = INDEX_NAME)
@Setting(settingPath = "settings/test-settings.json")
@Mapping(mappingPath = "mappings/test-mappings.json")
private static class Entity {
@Id String id;
}
}In addition, add another test class:
@ContextConfiguration(classes = { ElasticsearchTemplateConfiguration.class })
public class IndexOperationTransportTests extends IndexOperationTests {}This will inherit the test from the base class, but use the transport client to run the tests. If you then run this test, you will find the error in RequestFactory
| }) | ||
| .verifyComplete(); | ||
| } | ||
|
|
There was a problem hiding this comment.
see my comment for the non-reactive code in regards to checking the returned data
There was a problem hiding this comment.
Is there a file/helper where i could put the following as a static method instead of copy/pasting the same lines?
private void assertIndexInformationIsCorrect(IndexInformation indexInformation, String indexName) {
System.out.println(indexInformation.getMappings());
assertThat(indexInformation.getSettings().get("index.number_of_shards")).isEqualTo("1");
assertThat(indexInformation.getSettings().get("index.number_of_replicas")).isEqualTo("0");
assertThat(indexInformation.getSettings().get("index.analysis.analyzer.emailAnalyzer.type")).isEqualTo("custom");
assertThat(indexInformation.getMappings().get("properties.text.type")).isEqualTo("text");
assertThat(indexInformation.getMappings().get("properties.text.analyzer")).isEqualTo("synonym_analyzer");
assertThat(indexInformation.getName()).isEqualTo(indexName);
assertThat(indexInformation.getMappings()).isInstanceOf(org.springframework.data.elasticsearch.core.document.Document.class);
assertThat(indexInformation.getSettings()).isInstanceOf(org.springframework.data.elasticsearch.core.document.Document.class);
assertThat(indexInformation.getAliases()).isInstanceOf(List.class);
}|
|
||
|
|
||
| if (getIndexResponse.getMappings().containsKey(indexName)) { | ||
| MappingMetadata mappings = getIndexResponse.getMappings().get(indexName).get(indexName); |
There was a problem hiding this comment.
| MappingMetadata mappings = getIndexResponse.getMappings().get(indexName).get(indexName); | |
| MappingMetadata mappings = getIndexResponse.getMappings().get(indexName); |
one too many
| for (String indexName : getIndexResponse.getIndices()) { | ||
| Document settings = requestFactory.settingsFromGetIndexResponse(getIndexResponse, indexName); | ||
| Document mappings = requestFactory.mappingsFromGetIndexResponse(getIndexResponse, indexName); | ||
| Set<AliasData> indexAliases = aliases.get(indexName); | ||
|
|
||
| indexInformationList.add(IndexInformation.create(indexName, settings, mappings, indexAliases)); | ||
| } |
There was a problem hiding this comment.
Couldn't this whole loop be moved as well into the RequestFactory? Something like
List<IndexInformation> indexInformationList = requestFactory.fromGetIndexResponse(getIndexResponse);There was a problem hiding this comment.
I also didn't really like writing the same thing 3 times. But shouldn't request factory create only Request objects?
Is that a good place to add a function which returns a list of IndexInformation?
There was a problem hiding this comment.
fair point. The problem is that the reactive and non-reactive templates have no common base. Now that there are more than just one method for response data it might make sense to add a ResponseConverter class next to the RequestFactory that contains the methods for processing response data and that is used like the Requestfactoryis.
| IndexCoordinates indexCoordinates = getIndexCoordinates(); | ||
| GetIndexRequest request = requestFactory.getIndexRequest(indexCoordinates); | ||
|
|
||
| Map<String, Set<AliasData>> aliases = getAliasesForIndex(indexCoordinates.getIndexNames()); |
There was a problem hiding this comment.
this is executing a separate call to Elasticsearch for the aliases. But alias information is return in the GetIndex request, no need for a separate call here
| org.elasticsearch.client.indices.GetIndexRequest getIndexRequest = requestFactory.getIndexRequest(getIndexCoordinates()); | ||
|
|
||
| return Mono.from(operations.executeWithIndicesClient(client -> client.getIndex(HttpHeaders.EMPTY, getIndexRequest) | ||
| .flatMap(getIndexResponse -> getAliasesForIndex(getIndexCoordinates().getIndexNames()).map(aliases -> { |
There was a problem hiding this comment.
no separate call to get the alias information. This is returned in the GetIndex call
| for (String indexName : getIndexResponse.getIndices()) { | ||
| Document settings = requestFactory.settingsFromGetIndexResponse(getIndexResponse, indexName); | ||
| Document mappings = requestFactory.mappingsFromGetIndexResponse(getIndexResponse, indexName); | ||
| Set<AliasData> indexAliases = aliases.get(indexName); | ||
|
|
||
| indexInformationList.add(IndexInformation.create(indexName, settings, mappings, indexAliases)); | ||
| } |
There was a problem hiding this comment.
same comment as in the non-reactive code
|
|
||
| getIndexRequest.indices(index.getIndexNames()); | ||
|
|
||
| Map<String, Set<AliasData>> aliases = getAliases(index.getIndexNames()); |
There was a problem hiding this comment.
no separate call for aliases, check my other comments
| for (String indexName : getIndexResponse.getIndices()) { | ||
| Document settings = requestFactory.settingsFromGetIndexResponse(getIndexResponse, indexName); | ||
| Document mappings = requestFactory.mappingsFromGetIndexResponse(getIndexResponse, indexName); | ||
| Set<AliasData> indexAliases = aliases.get(indexName); | ||
|
|
||
| indexInformationList.add(IndexInformation.create(indexName, settings, mappings, indexAliases)); | ||
| } |
…r class to map Elastic responses to an internal class.
| Document settings = requestFactory.settingsFromGetIndexResponse(getIndexResponse, indexName); | ||
| Document mappings = requestFactory.mappingsFromGetIndexResponse(getIndexResponse, indexName); |
There was a problem hiding this comment.
These functions should be moved from the RequestFactory to the ResponseConverteras well. This will also remove the dependency on the RequestFactory.
| /** | ||
| * extract the index settings information from a given index | ||
| * @param getIndexResponse the elastic GetIndexResponse | ||
| * @param indexName the index name | ||
| * @return a document that represents {@link Settings} | ||
| */ | ||
| public Document settingsFromGetIndexResponse(GetIndexResponse getIndexResponse, String indexName) { | ||
| Document document = Document.create(); | ||
|
|
||
| Settings indexSettings = getIndexResponse.getSettings().get(indexName); | ||
|
|
||
| if (!indexSettings.isEmpty()) { | ||
| for (String key : indexSettings.keySet()) { | ||
| document.put(key, indexSettings.get(key)); | ||
| } | ||
| } | ||
|
|
||
| return document; | ||
| } | ||
|
|
||
| /** | ||
| * extract the mappings information from a given index | ||
| * @param getIndexResponse the elastic GetIndexResponse | ||
| * @param indexName the index name | ||
| * @return a document that represents {@link MappingMetadata} | ||
| */ | ||
| public Document mappingsFromGetIndexResponse(GetIndexResponse getIndexResponse, String indexName) { | ||
| Document document = Document.create(); | ||
|
|
||
| if (getIndexResponse.getMappings().containsKey(indexName)) { | ||
| MappingMetadata mappings = getIndexResponse.getMappings().get(indexName); | ||
| document = Document.from(mappings.getSourceAsMap()); | ||
| } | ||
|
|
||
| return document; | ||
| } | ||
|
|
||
| public Document settingsFromGetIndexResponse(org.elasticsearch.action.admin.indices.get.GetIndexResponse getIndexResponse, String indexName) { | ||
| Document document = Document.create(); | ||
|
|
||
| if (getIndexResponse.getSettings().containsKey(indexName)) { | ||
| Settings indexSettings = getIndexResponse.getSettings().get(indexName); | ||
|
|
||
| for (String key : indexSettings.keySet()) { | ||
| document.put(key, indexSettings.get(key)); | ||
| } | ||
| } | ||
|
|
||
| return document; | ||
| } | ||
|
|
||
| public Document mappingsFromGetIndexResponse(org.elasticsearch.action.admin.indices.get.GetIndexResponse getIndexResponse, String indexName) { | ||
| Document document = Document.create(); | ||
|
|
||
| if (getIndexResponse.getMappings().containsKey(indexName)) { | ||
| MappingMetadata mappings = getIndexResponse.getMappings().get(indexName).get("_doc"); | ||
| Map<String, Object> mappingsAsMap = mappings.getSourceAsMap(); | ||
|
|
||
| for (String key : mappingsAsMap.keySet()) { | ||
| document.put(key, mappingsAsMap); | ||
| } | ||
| } | ||
|
|
||
| return document; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
These should be moved to ResponseConverter.
|
|
||
| @ContextConfiguration(classes = { ElasticsearchTemplateConfiguration.class }) | ||
| @SpringIntegrationTest | ||
| public class IndexOperationTransportTests { |
There was a problem hiding this comment.
It's not necessary to implement anything in this class. Just like I commented last time, all that's needed is a derived class with a different @ContextConfiguration annotation. When the tests are run, this derived class will set up the ElasticsearchOperations using the transport client and run the tests with this client.
The IndexOperationsTests class just uses the methods from ElasticsearchOperations not caring which implementation it gets.
There was a problem hiding this comment.
I tried to do it in this way, but the the tests when were run all together were failing with the following exception:
Elasticsearch exception [type=resource_already_exists_exception, ... for test-index-rest-information-list index.
I removed one of the entities and left just one with index name: test-index-information-list but the test still failed.
I have tried to remove indexOps.create() from one of the methods, but it didn't work either.
If the test is run alone, it passes. So that's why i didn't extend the IndexOperationTests class in previous commit.
There was a problem hiding this comment.
forgot that. You need a method
@BeforeEach
void setUp() {
operations.indexOps(EntityWithSettingsAndMappingsRest.class).delete();
}in the base class that deletes the index on test start when it exists.
| assertThat(indexInformation.getName()).isEqualTo("test-index-rest-information-list"); | ||
| assertThat(indexInformation.getMappings()).isInstanceOf(org.springframework.data.elasticsearch.core.document.Document.class); | ||
| assertThat(indexInformation.getSettings()).isInstanceOf(org.springframework.data.elasticsearch.core.document.Document.class); | ||
| assertThat(indexInformation.getAliases()).isInstanceOf(List.class); |
There was a problem hiding this comment.
please check the content of the returned alias information
| * @since 4.0 | ||
| */ | ||
| class DefaultIndexOperations extends AbstractDefaultIndexOperations implements IndexOperations { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(DefaultIndexOperations.class); | ||
|
|
||
| private final ElasticsearchRestTemplate restTemplate; | ||
| private final ResponseConverter responseConverter; |
There was a problem hiding this comment.
this can be moved to the parent AbstractDefaultIndexOperations and be protected
| * @since 4.0 | ||
| */ | ||
| class DefaultTransportIndexOperations extends AbstractDefaultIndexOperations implements IndexOperations { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(DefaultTransportIndexOperations.class); | ||
|
|
||
| private final ResponseConverter responseConverter; |
| assertThat(indexInformation.getName()).isEqualTo(indexName); | ||
| assertThat(indexInformation.getMappings()).isInstanceOf(org.springframework.data.elasticsearch.core.document.Document.class); | ||
| assertThat(indexInformation.getSettings()).isInstanceOf(org.springframework.data.elasticsearch.core.document.Document.class); | ||
| assertThat(indexInformation.getAliases()).isInstanceOf(List.class); |
There was a problem hiding this comment.
here, check the content of the returned alias data
|
|
||
| @Test // #1646 | ||
| @DisplayName("should return info of all indices using TRANSPORT template with no aliases") | ||
| public void shouldReturnInformationListOfAllIndicesNoAliases() throws JSONException { |
There was a problem hiding this comment.
this method is not needed. It does the same as the one in the base class. The purpose of the IndexOperationTransportTests class is to run exactly the same tests as in IndexOperationTests just having a different implementation of ElasticsearchOperations.
| } | ||
|
|
||
| @Test // #1646 | ||
| @DisplayName("should return info of all indices using REST template without aliases") |
There was a problem hiding this comment.
why without aliases? The test should cover that alias information is returned as well.
| @Nullable | ||
| private final Document mappings; | ||
| @Nullable | ||
| private final List<AliasMetadata> aliases; |
There was a problem hiding this comment.
I just now noticed that here we use AliasMetadata from Elasticsearch. We should use org.springframework.data.elasticsearch.core.index.AliasData, the same class that is used in the return values of org.springframework.data.elasticsearch.core.AbstractDefaultIndexOperations.getAliasesForIndex()
There was a problem hiding this comment.
I'm searching for the way to conver AliasMetadata to AliasData but something looks off.
The filter atribute of AliasData is Document which essentialy is a Map, but the filter attribute of AliasMetadata is CompressedXContent. So how do i convert it to a Document?
There was a problem hiding this comment.
there already is org.springframework.data.elasticsearch.core.RequestFactory.convertAliasMetadata() - which probably could be moved to ResponseConverter as well
| for (String indexName : getIndexResponse.getIndices()) { | ||
| Document settings = settingsFromGetIndexResponse(getIndexResponse, indexName); | ||
| Document mappings = mappingsFromGetIndexResponse(getIndexResponse, indexName); | ||
| List<AliasMetadata> aliases = getIndexResponse.getAliases().get(indexName) != null |
There was a problem hiding this comment.
Use org.springframework.data.elasticsearch.core.index.AliasData in these functions
| void shouldReturnInformationListOfAllIndices() { | ||
| String indexName = "test-index-reactive-information-list"; | ||
| ReactiveIndexOperations indexOps = template.indexOps(EntityWithSettingsAndMappingsReactive.class); | ||
|
|
There was a problem hiding this comment.
set and test alias information as well
|
and please rebase your changes on the current master branch |
moved some methods from request factory to resposne converter and added more checks to the test.
This PR is for #1646.
I don't know if there is a chance where the new
IndexInformationclass would be delivered directly to a client, but if there is a such chance, this will fail.For some reason, Jackson can not encode the
settingsattrribute (Settings class) with the following message:for the following json:
{ "INDEX_NAME": { "aliases": {}, "mappings": {}, "settings": { "index": { "refresh_interval": "1s", "number_of_shards": "1", "provided_name": "X", "creation_date": "1613299161583", "store": { "type": "fs" }, "number_of_replicas": "1", "uuid": "swcy7LHKSEC8kJpWBkGIJw", "version": { "created": "6080099" } } } } }Also,
IndexInformationhas some kind of duplicated code, but that's because of the TransportClient GetIndexRequest.When it get's deprecated, i can clean it.
I would be happy to hear comments since this is my first PR.