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

(dsl): Support create and upsert requests #6

Merged
merged 16 commits into from
Nov 28, 2022
Merged

Conversation

dbulaja98
Copy link
Collaborator

No description provided.

private[elasticsearch] final case class GetById(
index: IndexName,
id: DocumentId,
routing: Option[Routing] = None
) extends ElasticRequest[Option[Document]]

private[elasticsearch] final case class PutItem(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use PutDocument. Or maybe even Put. Also, in the future maybe we will have some other Put* and GetById*, so not sure if should we use GetDocumentById and PutDocument.

doc: A,
routing: Option[Routing] = None
)(implicit schema: Schema[A]): ElasticRequest[Unit] =
Put(index, id, Document.apply(doc), routing)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can omit .apply. You can use only Document(doc).

@@ -1,11 +1,17 @@
package zio

import zio.schema.Schema
import zio.schema.codec.DecodeError
import zio.schema.codec.{DecodeError, JsonCodec}
import zio.schema.codec.JsonCodec.JsonDecoder

package object elasticsearch {
private[elasticsearch] final case class Document(json: String) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it makes sense to extract it out of the package object. Create a separate file called Document.

}

private[elasticsearch] object Document {
def apply[A](doc: A)(implicit schema: Schema[A]): Document = Document(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change to from

Copy link
Member

@drmarjanovic drmarjanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After we did some research we identified the following way to add documents to the Elasicsearch:

  • PUT /<index>/_doc/<id>
    This endpoint either updates the document or creates a new one. However, note that we can provide op_type to create and change its behavior of it - it will only create a new document if does not exist or fail.
  • PUT/POST /<index>/_create/<id>
    Creates a new document with a specified ID or fails.
  • POST <index>/_doc
    Creates a new document with auto-generated ID.

Keeping this in mind, it makes sense to me to provide following public API in our library:

def create[A: Schema](index: IndexName, id: DocumentId, doc: A, ...)

def create[A: Schema](index: IndexName, doc: A, ...)

def upsert[A: Schema](index: IndexName, id: DocumentId, doc: A, ...)

Regarding our underlying types, we can go with two options:

  1. Have both final case class Create(id: Option[DocumentId], ...) and final case class CreateOrUpdate(id: DocumentId, ...)`, and depending on that decide what should we do (in the Executor)
  2. Have only `Put(id: Option[DocumentId], create: Boolean, ...)

@@ -24,10 +24,40 @@ object ElasticRequest {
case None => Left(DocumentNotFound)
}

def create[A: Schema](
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add both:
create(index: IndexName, id: DocumentId, doc: A...)
and
create(index: IndexName, doc: A).

We are seeking to provide simpler public API and make our library usage easier, so it makes sense to provide rich public API.


private[elasticsearch] object Document {
def from[A](doc: A)(implicit schema: Schema[A]): Document = Document(
JsonCodec.jsonEncoder(schema).encodeJson(a = doc, indent = None).toString
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove a = as it does not provide additional information.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should 'indent' remain?

Base automatically changed from dsl-get-by-id-request to main November 28, 2022 08:51
@drmarjanovic drmarjanovic changed the title (dsl): Support Put request (dsl): Support create and upsert requests (#6) Nov 28, 2022
@drmarjanovic drmarjanovic changed the title (dsl): Support create and upsert requests (#6) (dsl): Support create and upsert requests Nov 28, 2022
@drmarjanovic drmarjanovic merged commit 85ee86f into main Nov 28, 2022
@drmarjanovic drmarjanovic deleted the dsl-put-request branch November 28, 2022 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants