Support index pattern #316

milicns · 2023-08-30T13:41:34Z

Closes #310

Refresh, Search and GetById requests currently adjusted for handling IndexSelectors, i am not sure about GetById completely.

drmarjanovic

Can you provide more integration tests with using multiple indexes?

drmarjanovic · 2023-08-30T13:48:39Z

modules/integration/src/test/scala/zio/elasticsearch/HttpExecutorSpec.scala

+            } yield assert(res)(equalTo(true))
+          },
+          test("successfully refresh all indices") {
+            assertZIO(Executor.execute(ElasticRequest.refresh(IndexPattern("_all"))))(equalTo(true))


You can use IndexPattern.All or whatever we choose to refactor it to.

drmarjanovic · 2023-08-30T13:50:12Z

modules/library/src/main/scala-2/zio/elasticsearch/MultiIndex.scala

+import zio.prelude.Newtype
+
+trait IndexSelector[A] {
+  def selectorString(a: A): String


I would use toSelector or asSelector.

drmarjanovic · 2023-08-30T13:50:32Z

modules/library/src/main/scala-2/zio/elasticsearch/MultiIndex.scala

+object IndexSelector {
+
+  implicit object IndexNameSelector extends IndexSelector[IndexName] {
+    def selectorString(a: IndexName): String = IndexName.unwrap(a)


I would use toSelector(name: IndexName)...

drmarjanovic · 2023-08-30T13:50:41Z

modules/library/src/main/scala-2/zio/elasticsearch/MultiIndex.scala

+  }
+
+  implicit object IndexPatternSelector extends IndexSelector[IndexPattern] {
+    def selectorString(a: IndexPattern): String = IndexPattern.unwrap(a)


pattern instead of a.

drmarjanovic · 2023-08-30T13:51:10Z

modules/library/src/main/scala-2/zio/elasticsearch/MultiIndex.scala

+  }
+
+  implicit object MultiIndexSelector extends IndexSelector[MultiIndex] {
+    def selectorString(a: MultiIndex): String = a.indices.mkString(",")


Probably multi instead of a.

drmarjanovic · 2023-08-30T13:52:08Z

modules/library/src/main/scala-2/zio/elasticsearch/MultiIndex.scala

+}
+
+final case class MultiIndex private (indices: Chunk[String]) { self =>
+  def names(indexNames: IndexName*): MultiIndex =


Does it make sense to support empty? Maybe def names(name: IndexName, names: IndexNames).

drmarjanovic · 2023-08-30T13:52:15Z

modules/library/src/main/scala-2/zio/elasticsearch/MultiIndex.scala

+  def names(indexNames: IndexName*): MultiIndex =
+    self.copy(indices = indices ++ Chunk.fromIterable(indexNames.map(IndexName.unwrap)))
+
+  def patterns(indexPatterns: IndexPattern*): MultiIndex =


drmarjanovic · 2023-08-30T13:52:27Z

modules/library/src/main/scala-2/zio/elasticsearch/MultiIndex.scala

+}
+
+object MultiIndex {
+  def names(indexNames: IndexName*): MultiIndex =


drmarjanovic · 2023-08-30T13:52:34Z

modules/library/src/main/scala-2/zio/elasticsearch/MultiIndex.scala

+  def names(indexNames: IndexName*): MultiIndex =
+    new MultiIndex(Chunk.fromIterable(indexNames.map(IndexName.unwrap)))
+
+  def patterns(indexPatterns: IndexPattern*): MultiIndex =


drmarjanovic · 2023-08-31T07:24:36Z

modules/integration/src/test/scala/zio/elasticsearch/HttpExecutorSpec.scala

+            for {
+              _   <- Executor.execute(ElasticRequest.createIndex(createIndexTestName))
+              res <- Executor.execute(ElasticRequest.refresh(MultiIndex.names(index, createIndexTestName)))
+            } yield assert(res)(equalTo(true))


Let's replace all equalTo(true) with isTrue.

drmarjanovic · 2023-08-31T07:25:00Z

modules/integration/src/test/scala/zio/elasticsearch/HttpExecutorSpec.scala

+            } yield assert(res)(equalTo(true))
+          },
+          test("successfully refresh all indices") {
+            assertZIO(Executor.execute(ElasticRequest.refresh(IndexPatternAll)))(equalTo(true))
          },
          test("return false if index does not exists") {
            assertZIO(Executor.execute(ElasticRequest.refresh(refreshFailIndex)))(
              equalTo(false)


I guess you can use isFalse here.

drmarjanovic · 2023-08-31T07:25:34Z

modules/integration/src/test/scala/zio/elasticsearch/IntegrationSpec.scala

@@ -54,6 +54,8 @@ trait IntegrationSpec extends ZIOSpecDefault {

  val refreshFailIndex: IndexName = IndexName("refresh-fail")

+  val IndexPatternAll: IndexPattern = IndexPattern("_all")


Can we use a constant?

drmarjanovic · 2023-08-31T07:26:37Z

modules/library/src/main/scala-3/zio/elasticsearch/IndexPatternValidator.scala

+        )
+      )
+    }
+  })


Missing new line.

dbulaja98 · 2023-08-31T07:21:45Z

docs/overview/requests/elastic_request_get_by_id.md

+If you want to create `GetById` request with `IndexPattern`, you can do that in the following manner:
+```scala
+val requestWithIndexPattern: GetByIdRequest = getById(index = IndexPattern("index*"), id = DocumentId("111"))
+```
+


Maybe put this after this part To create a GetById request do the following:.

dbulaja98 · 2023-08-31T07:45:10Z

modules/integration/src/test/scala/zio/elasticsearch/HttpExecutorSpec.scala

+                for {
+                  _       <- Executor.execute(ElasticRequest.deleteByQuery(firstSearchIndex, matchAll))
+                  _       <- Executor.execute(ElasticRequest.deleteByQuery(secondSearchIndex, matchAll))
+                  document = firstDocument.copy(stringField = s"this is test")


Since you named copy of secondDocument: secondDocumentCopy, you should call this one firstDocumentCopy.

dbulaja98 · 2023-08-31T07:47:28Z

modules/library/src/main/scala-2/zio/elasticsearch/MultiIndex.scala

+object IndexSelector {
+
+  implicit object IndexNameSelector extends IndexSelector[IndexName] {
+    def toSelector(name: IndexName): String = IndexName.unwrap(name)


Put implementation in the new line.

dbulaja98 · 2023-08-31T07:47:40Z

modules/library/src/main/scala-2/zio/elasticsearch/MultiIndex.scala

+  }
+
+  implicit object IndexPatternSelector extends IndexSelector[IndexPattern] {
+    def toSelector(pattern: IndexPattern): String = IndexPattern.unwrap(pattern)


dbulaja98 · 2023-08-31T07:48:15Z

modules/library/src/main/scala-2/zio/elasticsearch/MultiIndex.scala

+  }
+
+  implicit object MultiIndexSelector extends IndexSelector[MultiIndex] {
+    def toSelector(multi: MultiIndex): String = multi.indices.mkString(",")


dbulaja98 · 2023-08-31T08:23:15Z

modules/library/src/main/scala-3/zio/elasticsearch/MultiIndex.scala

+    new MultiIndex(Chunk.fromIterable(name.toString +: names.map(IndexName.unwrap)))
+
+  def patterns(pattern: IndexPattern, patterns: IndexPattern*): MultiIndex =
+    new MultiIndex(Chunk.fromIterable(pattern.toString +: patterns.map(IndexPattern.unwrap)))


dbulaja98 · 2023-08-31T08:32:01Z

modules/library/src/main/scala-3/zio/elasticsearch/MultiIndex.scala

+
+  def patterns(pattern: IndexPattern, patterns: IndexPattern*): MultiIndex =
+    new MultiIndex(Chunk.fromIterable(pattern.toString +: patterns.map(IndexPattern.unwrap)))
+}


Put an empty line at the end.

dbulaja98 · 2023-08-31T08:33:50Z

modules/library/src/main/scala/zio/elasticsearch/ElasticRequest.scala

@@ -211,19 +212,19 @@ object ElasticRequest {
   * @return
   *   an instance of [[GetByIdRequest]] that represents get by id operation to be performed.
   */
-  final def getById(index: IndexName, id: DocumentId): GetByIdRequest =
-    GetById(index = index, id = id, refresh = None, routing = None)
+  final def getById[I: IndexSelector](index: I, id: DocumentId): GetByIdRequest =


Should we update scaladoc for this method too? (index or more indices)

dbulaja98 · 2023-08-31T08:35:03Z

modules/library/src/main/scala/zio/elasticsearch/ElasticRequest.scala

@@ -235,11 +236,11 @@ object ElasticRequest {
   * @return
   *   an instance of [[SearchRequest]] that represents search operation to be performed.
   */
-  final def search(index: IndexName, query: ElasticQuery[_]): SearchRequest =
+  final def search[I: IndexSelector](index: I, query: ElasticQuery[_]): SearchRequest =


Also here? (index or more indices)

dbulaja98 · 2023-08-31T08:35:20Z

modules/library/src/main/scala/zio/elasticsearch/ElasticRequest.scala

@@ -261,16 +262,16 @@ object ElasticRequest {
   * @return
   *   an instance of [[SearchAndAggregateRequest]] that represents search and aggregate operations to be performed.
   */
-  final def search(
-    index: IndexName,
+  final def search[I: IndexSelector](


Also here? (index or more indices)

milicns · 2023-08-31T10:34:40Z

GetById request adjusted back to supporting only IndexName, because it seems that it doesn't support IndexPattern https://www.elastic.co/guide/en/elasticsearch/reference/7.17/docs-get.html

drmarjanovic · 2023-08-31T13:29:28Z

modules/integration/src/test/scala/zio/elasticsearch/HttpExecutorSpec.scala

+                for {
+                  _                <- Executor.execute(ElasticRequest.deleteByQuery(firstSearchIndex, matchAll))
+                  _                <- Executor.execute(ElasticRequest.deleteByQuery(secondSearchIndex, matchAll))
+                  firstDocumentCopy = firstDocument.copy(stringField = s"this is test")


Suggested change

firstDocumentCopy = firstDocument.copy(stringField = s"this is test")

firstDocumentCopy = firstDocument.copy(stringField = "this is test")

drmarjanovic · 2023-08-31T13:29:43Z

modules/integration/src/test/scala/zio/elasticsearch/HttpExecutorSpec.scala

+                        .upsert[TestDocument](firstSearchIndex, firstDocumentId, firstDocumentCopy)
+                        .refreshTrue
+                    )
+                  secondDocumentCopy = secondDocument.copy(stringField = s"this is test")


Suggested change

secondDocumentCopy = secondDocument.copy(stringField = s"this is test")

secondDocumentCopy = secondDocument.copy(stringField = "this is test")

drmarjanovic · 2023-08-31T13:30:01Z

modules/integration/src/test/scala/zio/elasticsearch/HttpExecutorSpec.scala

+                for {
+                  _                <- Executor.execute(ElasticRequest.deleteByQuery(firstSearchIndex, matchAll))
+                  _                <- Executor.execute(ElasticRequest.deleteByQuery(secondSearchIndex, matchAll))
+                  firstDocumentCopy = firstDocument.copy(stringField = s"this is test")


Suggested change

firstDocumentCopy = firstDocument.copy(stringField = s"this is test")

firstDocumentCopy = firstDocument.copy(stringField = "this is test")

drmarjanovic · 2023-08-31T13:30:13Z

modules/integration/src/test/scala/zio/elasticsearch/HttpExecutorSpec.scala

+                        .upsert[TestDocument](firstSearchIndex, firstDocumentId, firstDocumentCopy)
+                        .refreshTrue
+                    )
+                  secondDocumentCopy = secondDocument.copy(stringField = s"this is test")


Suggested change

secondDocumentCopy = secondDocument.copy(stringField = s"this is test")

secondDocumentCopy = secondDocument.copy(stringField = "this is test")

drmarjanovic · 2023-08-31T13:35:01Z

modules/library/src/main/scala/zio/elasticsearch/ElasticRequest.scala

@@ -717,7 +718,7 @@ object ElasticRequest {
    aggregation: ElasticAggregation,
    excluded: Chunk[String],
    included: Chunk[String],
-    index: IndexName,
+    index: String,


I'm thinking about renaming this to "selector" or "selectors" and using Chunk[String] instead. Also, I would probably use the same name and type for all underlying private classes in the ElasticRequest.

@arnoldlacko @dbulaja98

Maybe we should consider using both "selector" and "selectors" for places we have one index, and places where we can have more than one.

dbulaja98 · 2023-09-08T09:15:32Z

docs/overview/requests/elastic_request_aggregate.md

@@ -9,11 +9,21 @@ To create a `Aggregate` request do the following:
 ```scala
 import zio.elasticsearch.ElasticRequest.AggregateRequest
 import zio.elasticsearch.ElasticRequest.aggregate
-// this import is required for using `IndexName`
+// this import is required for using `IndexName, IndexPattern, MultiIndex`


Suggested change

// this import is required for using `IndexName, IndexPattern, MultiIndex`

// this import is required for using `IndexName, IndexPattern and MultiIndex`

dbulaja98 · 2023-09-08T11:09:52Z

docs/overview/requests/elastic_request_search.md

@@ -13,11 +13,11 @@ To create a `Search` request do the following:
 ```scala
 import zio.elasticsearch.ElasticRequest.SearchRequest
 import zio.elasticsearch.ElasticRequest.search
-// this import is required for using `IndexName`
+// this import is required for using `IndexName, IndexPattern, MultiIndex`


Suggested change

// this import is required for using `IndexName, IndexPattern, MultiIndex`

// this import is required for using `IndexName, IndexPattern and MultiIndex`

dbulaja98 · 2023-09-08T11:12:48Z

docs/overview/requests/elastic_request_search.md

+
+If you want to create `Search` request with `MultiIndex`, do the following:
+```scala
+val requestWithMultiIndex: SearchRequest = search(selectors = MultiIndex.names(IndexName("index1"), IndexName("index2")), query = matchAll)
 ```

 You can find more information about `Search` and `SearchAndAggregate` requests [here](https://www.elastic.co/guide/en/elasticsearch/reference/7.17/search-search.html).


Can you add new line at the end of the file?

dbulaja98 · 2023-09-08T14:41:48Z

modules/library/src/main/scala-2/zio/elasticsearch/MultiIndex.scala

+
+}
+
+final case class MultiIndex private[elasticsearch] (indices: Chunk[String]) { self =>


Can you make this private[elasticsearch]?

dbulaja98 · 2023-09-08T15:03:56Z

modules/library/src/main/scala-3/zio/elasticsearch/IndexPatternValidator.scala

+object IndexPatternValidator
+  extends Validator[String](pattern => {


Can this be in one line?

dbulaja98 · 2023-09-08T15:04:29Z

modules/library/src/main/scala-3/zio/elasticsearch/MultiIndex.scala

+
+}
+
+final case class MultiIndex private[elasticsearch] (indices: Chunk[String]) { self =>


Same here - private[es].

dbulaja98 · 2023-09-08T15:05:28Z

modules/library/src/main/scala/zio/elasticsearch/ElasticRequest.scala

-   * @param index
-   *   the name of the Elasticsearch index to aggregate on
+   * @param selectors
+   *   the name of the index or more indices to be refreshed


Suggested change

* the name of the index or more indices to be refreshed

* the name of the index or more indices to aggregate on

dbulaja98 · 2023-09-08T15:32:02Z

modules/library/src/test/scala/zio/elasticsearch/ElasticRequestSpec.scala

+          assert(getByIdRequest)(
+            equalTo(GetById(index = Index, id = DocId, refresh = None, routing = None))


Revert this to one line.

dbulaja98 · 2023-09-08T15:32:16Z

modules/library/src/test/scala/zio/elasticsearch/ElasticRequestSpec.scala

+            equalTo(
+              GetById(index = Index, id = DocId, refresh = Some(true), routing = Some(RoutingValue))
+            )


Vasilije Milic added 2 commits August 29, 2023 17:38

Implement basic idea

ec9299c

Support index pattern and multi index

b6485b0

milicns changed the title ~~Task support index pattern~~ Support index pattern Aug 30, 2023

drmarjanovic reviewed Aug 30, 2023

View reviewed changes

Vasilije Milic added 3 commits August 30, 2023 15:58

Fix for scala 3

4b2ccc4

Add more integration tests

eae71b5

Fix code remarks

ccb226f

milicns requested a review from drmarjanovic August 30, 2023 16:48

drmarjanovic requested changes Aug 31, 2023

View reviewed changes

dbulaja98 requested changes Aug 31, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

Vasilije Milic added 3 commits August 31, 2023 12:00

Fix code remarks

0a0d319

Fix doc and make factory method

3b8af18

Remove factory method

5d969e3

milicns requested review from drmarjanovic and dbulaja98 August 31, 2023 10:36

drmarjanovic requested changes Aug 31, 2023

View reviewed changes

Vasilije Milic added 2 commits September 1, 2023 11:05

Change index to selector

2a8ca4b

Update doc

f304e76

milicns requested a review from drmarjanovic September 1, 2023 10:03

drmarjanovic previously approved these changes Sep 2, 2023

View reviewed changes

Update aggregate request

b154c99

milicns dismissed drmarjanovic’s stale review via b154c99 September 7, 2023 19:03

dbulaja98 requested changes Sep 8, 2023

View reviewed changes

dbulaja98 merged commit a03ace4 into lambdaworks:main Sep 20, 2023
13 checks passed

		@@ -54,6 +54,8 @@ trait IntegrationSpec extends ZIOSpecDefault {

		val refreshFailIndex: IndexName = IndexName("refresh-fail")

		val IndexPatternAll: IndexPattern = IndexPattern("_all")

	firstDocumentCopy = firstDocument.copy(stringField = s"this is test")
	firstDocumentCopy = firstDocument.copy(stringField = "this is test")

	secondDocumentCopy = secondDocument.copy(stringField = s"this is test")
	secondDocumentCopy = secondDocument.copy(stringField = "this is test")

	// this import is required for using `IndexName, IndexPattern, MultiIndex`
	// this import is required for using `IndexName, IndexPattern and MultiIndex`


		}

		final case class MultiIndex private[elasticsearch] (indices: Chunk[String]) { self =>

		object IndexPatternValidator
		extends Validator[String](pattern => {

	* the name of the index or more indices to be refreshed
	* the name of the index or more indices to aggregate on

		assert(getByIdRequest)(
		equalTo(GetById(index = Index, id = DocId, refresh = None, routing = None))

Support index pattern #316

Support index pattern #316

Conversation

milicns commented Aug 30, 2023 • edited by drmarjanovic Loading

drmarjanovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

milicns commented Aug 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbulaja98 Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milicns commented Aug 30, 2023 •

edited by drmarjanovic

Loading

dbulaja98 Sep 1, 2023 •

edited

Loading