(api): Restructure errors #36
Conversation
| @@ -6,6 +6,8 @@ import zio.prelude.AssertionError.failure | |||
| import zio.prelude.Newtype | |||
|
|
|||
| package object elasticsearch { | |||
| case class ElasticException(message: String) extends Throwable | |||
There was a problem hiding this comment.
We are missing final here.
There was a problem hiding this comment.
Also, maybe it makes sense to make it private. I guess we don't want other to extend this? @arnoldlacko
There was a problem hiding this comment.
I agree with making it private to zio.elasticsearch, but I would not make it a final case class as I think we'll be extending it ourselves. I think it could be just a class.
There was a problem hiding this comment.
Also I think it would make sense to extend RuntimeException(message) instead of Throwable
There was a problem hiding this comment.
This error is vague. As a case class, it should be final (extending case classes is bad practice). The remark about extending RuntimeException is valid.
There was a problem hiding this comment.
I agree with not extending case classes, my thinking was to make it a regular class (not a case class) assuming we might want to extend it with some more specific exception types later (it could even become abstract then). I'm also fine with a final case class for now, given that we don't yet have other exception types.
modules/library/src/it/scala/zio/elasticsearch/HttpExecutorSpec.scala
Outdated
Show resolved
Hide resolved
modules/library/src/it/scala/zio/elasticsearch/HttpExecutorSpec.scala
Outdated
Show resolved
Hide resolved
modules/library/src/it/scala/zio/elasticsearch/HttpExecutorSpec.scala
Outdated
Show resolved
Hide resolved
modules/library/src/it/scala/zio/elasticsearch/HttpExecutorSpec.scala
Outdated
Show resolved
Hide resolved
| ).flatMap { response => | ||
| response.code match { | ||
| case CreatedCode => | ||
| response.body.map(a => DocumentId.apply(a.id)) match { |
There was a problem hiding this comment.
I suppose you can use response.body.map(DocumentId(_.id)).
Also, is there any chance to say this is unsafe? We are sure this is correct.
Instead of pattern matching I would use .fold.
There was a problem hiding this comment.
What about using StatusCode.Created instead?
There was a problem hiding this comment.
I can not use response.body.map(DocumentId(_.id)).
modules/library/src/main/scala/zio/elasticsearch/HttpElasticExecutor.scala
Outdated
Show resolved
Hide resolved
| response.code match { | ||
| case CreatedCode => ZIO.succeed(Created) | ||
| case Conflict => ZIO.succeed(AlreadyExists) | ||
| case _ => ZIO.fail(ElasticException(s"Unexpected response from Elasticsearch: $response")) |
There was a problem hiding this comment.
Should we use this text? @arnoldlacko
There was a problem hiding this comment.
I think we need to offer some way for the user to understand what the actual response is, but probably $response contains some other things as well that are not necessarily relevant for the user. @dbulaja98 could you please reproduce a failure and paste the message we would get?
A different concern is whether there could be sensitive data coming in the response, but it's on the user to decide what they log or not, so I think this way we're safer than if we actually did the logging ourselves.
There was a problem hiding this comment.
message = "Unexpected response from Elasticsearch: Response(Left(sttp.client3.HttpError: statusCode: 404, response: {"_index":"users","_type":"_doc","_id":"000000000K","found":false}),404,,List(content-type: application/json; charset=UTF-8, content-encoding: gzip, content-length: 79, warning: 299 Elasticsearch-7.17.7-78dcaaa8cee33438b91eca7f5c7f56a70fec9e80 "Elasticsearch built-in security features are not enabled. Without authentication, your cluster could be accessible to anyone. See https://www.elastic.co/guide/en/elasticsearch/reference/7.17/security-minimal-setup.html to enable security.", x-elastic-product: Elasticsearch),List(),RequestMetadata(GET,http://localhost:9200/users/_doc/000000000K,Vector(Accept-Encoding: gzip, deflate)))"
There was a problem hiding this comment.
This is reproduced message from exception: 404 NotFound in getById (We will never see this error, as it is regular case for get).
modules/library/src/main/scala/zio/elasticsearch/HttpElasticExecutor.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/HttpElasticExecutor.scala
Outdated
Show resolved
Hide resolved
| sealed trait ElasticRequest[+A, ERT <: ElasticRequestType] { self => | ||
|
|
||
| final def execute: RIO[ElasticExecutor, A] = | ||
| ZIO.serviceWithZIO[ElasticExecutor](_.execute(self)) | ||
|
|
||
| final def map[B](f: A => B): ElasticRequest[B, ERT] = ElasticRequest.Map(self, f) | ||
| final def map[B](f: A => Try[B]): ElasticRequest[B, ERT] = ElasticRequest.Map(self, f) |
There was a problem hiding this comment.
Let's not model things around Try.
There was a problem hiding this comment.
This one was my suggestion in order to avoid having a throw in the mapper.
Given that there is no side-effect involved, using Task seemed too much, while an Either[Throwable, B] could also be ok, just a bit more verbose than Try.
Do you think we should opt for one of these or did you maybe have something else in mind?
There was a problem hiding this comment.
Since we just introduced ElasticException, probably an Either[ElasticException, B] would make more sense than a Try
| trait CreateIndex extends ElasticRequestType | ||
| trait Create extends ElasticRequestType | ||
| trait CreateWithId extends ElasticRequestType | ||
| trait DeleteById extends ElasticRequestType | ||
| trait DeleteIndex extends ElasticRequestType | ||
| trait Exists extends ElasticRequestType | ||
| trait GetById extends ElasticRequestType | ||
| trait GetByQuery extends ElasticRequestType | ||
| trait Upsert extends ElasticRequestType |
There was a problem hiding this comment.
Why are they traits? Still in progress?
There was a problem hiding this comment.
We will never be instantiating them (if I'm not mistaken they would be called "phantom-types"), as their intention is only to help with restrictions within the API, so that methods like .routing and .refresh could not be used on requests that don't actually allow them (instead of just making them have no effect).
They were introduced recently in this PR: https://github.com/lambdaworks/zio-elasticsearch/pull/33/files
modules/library/src/main/scala/zio/elasticsearch/HttpElasticExecutor.scala
Outdated
Show resolved
Hide resolved
No description provided.