Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • Nov 27 19:59
    julienrf synchronize #963
  • Nov 27 19:38
    julienrf commented #963
  • Nov 27 19:38
    julienrf synchronize #963
  • Nov 27 19:23
    ptrdom commented #963
  • Nov 27 19:08
    julienrf synchronize #963
  • Nov 27 14:21
    julienrf commented #963
  • Nov 27 13:58
    ptrdom commented #963
  • Nov 27 13:39
    julienrf synchronize #963
  • Nov 27 13:36
    julienrf commented #948
  • Nov 27 13:04
    julienrf commented #963
  • Nov 27 12:56
    julienrf commented #963
  • Nov 26 21:23
    ptrdom commented #963
  • Nov 26 21:11
    codecov[bot] commented #934
  • Nov 26 20:59
    codecov[bot] commented #934
  • Nov 26 20:30
    codecov[bot] commented #934
  • Nov 26 20:30
    ptrdom synchronize #934
  • Nov 26 19:18

    julienrf on master

    Fix http4s byte chunked entitie… (compare)

  • Nov 26 19:18
    julienrf closed #961
  • Nov 26 16:59
    ptrdom commented #963
  • Nov 26 16:46
    codecov[bot] commented #934
Milos Nedeljkovic
@minedeljkovic

Encoding of heterogenous arrays (tuples) in OpenApi docs seems against OpenApi spec.
Example, this definition

  val foo: Endpoint[Unit, (String, Long)] =
    endpoint(
      request = get(path / "foo"),
      response = ok(jsonResponse[(String, Long)])
    )

produces this OpenApi definition for response content (items is an array of SchemaObject)

"content": {
  "application/json": {
    "schema": {
      "type": "array",
      "items": [
        {
          "type": "string"
        },
        {
          "type": "integer",
          "format": "int64"
        }
      ]
    }
  }
}

but, OpenApi spec is explicitly against that. In this part, it's stated:

items - Value MUST be an object and not an array. Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema. items MUST be present if the type is array.

Now, OpenApi 3.0 does not have a proper encoding for heterogenous arrays, unfortunately, afaik.
But the question is, is producing definition against the spec the right choice, or it might be better to produce (not perfectly precise) approximation that conforms spec?

Julien Richard-Foy
@julienrf
@minedeljkovic I used this source to model the schema of tuples: https://json-schema.org/understanding-json-schema/reference/array.html#tuple-validation but now I learn about prefixItems. Unfortunately, OpenAPI 3.0 does not support them, as you noticed, so yes, I agree that it is better to produce something else, that conforms to the spec.
Milos Nedeljkovic
@minedeljkovic
Dominic Egger
@GrafBlutwurst
Good day everyone, I was wondering if I'm right in the assumption that chunked encoding for files is not yet a thing on http4s-server?
Julien Richard-Foy
@julienrf
Hi @GrafBlutwurst, indeed we didn’t implement it yet. See endpoints4s/endpoints4s#510 We had one issue with the strategy used by the other interpreters
If we fix that issue, we need to update all the other interpreters to be consistent
Would you be interested in contributing to this effort?
Dominic Egger
@GrafBlutwurst
ahhh I looked into the issues but not the PRs, sorry about that.
Yeah I might have a crack at it, gotta check with my co-workers. But imo it would be the best solution for what we need.
Dominic Egger
@GrafBlutwurst
so i merged lucas' branch to mine and got the tests passing, but I must be understanding something wrong with chunking. What does it mean that implementedByEffecttakes a parameter of Stream[IO, Req] => IO[Resp]?
server tests that is
Julien Richard-Foy
@julienrf
It’s for the case of the request entity is streamed, but not the response
Dominic Egger
@GrafBlutwurst
right so it can be assumed that by the time the stream hits implementedByEffect it has been reassembled and the stream only ever contains one element?
Julien Richard-Foy
@julienrf
Yes, that’s the problem we have with the current way of framing chunks
Dominic Egger
@GrafBlutwurst
right, now I understand the conversation much better.
Dominic Egger
@GrafBlutwurst
but that issue is not specific to the http4s implementation is it? it seems to be the same in akka where we get Source[Req, _]
Julien Richard-Foy
@julienrf
Yes
Well, we do the same assumptions on all the interpreters
It turns out that the akka-http interpreters work well with this assumption (chunks are not reframed)
same for play-server
Dominic Egger
@GrafBlutwurst
you wouldn't happen to have a log/dump of the particular errors you encountered due to reframing anywhere?
Julien Richard-Foy
@julienrf
No :( it seems that Travis CI does not have the logs of the tests either
but if you managed to get the tests pass, then it means that it works, no?
Dominic Egger
@GrafBlutwurst

weeeell. the server ones are but there's an exception that doesn't fail the test somehow. client tests are not passing. I just got them compiling. I suspect this bit from http4s docs might be pertinent though:

Next, we want streaming JSON. Because http4s is streaming in its bones, it relies on jawn for streaming JSON support. Most popular JSON libraries, including Circe, provide jawn support, as the code below shows. What’s left is to integrate jawn’s streaming parsing with fs2’s Stream. That’s done by jawn-fs2, which http4s’ Circe module depends on transitively.

I think you hit the nail on the head with what you wrote in the PR
I don't see a way to address this without baking in circe or at least jawn-fs2 into the http4s-client
Julien Richard-Foy
@julienrf
Ah, if that’s a problem we can create separate modules (e.g., http4s-streaming-server)
but I guess there is a high chance that most of the users of http4s also use the http4s-circe module?
Dominic Egger
@GrafBlutwurst
I would suspect most. I'll do some more tinkering to confirm if that's the issue
Dominic Egger
@GrafBlutwurst
hahaha I think we lazified ourselfs to death because we have an F[Stream[F,Resp]] within a Resource.use i just wrote some truly heretical code, but all tests are green. maybe someone smarter than I knows how to do this properly
Dominic Egger
@GrafBlutwurst
https://github.com/rivero-ag/endpoints4s/blob/http4s-chunked-encoding/http4s/client/src/main/scala/endpoints4s/http4s/client/ChunkedEntities.scala#L55 this is some truly cursed code but makes the tests green. If somebody has some good input on how to address this, I'd be very glad. I'm out for the day though
Dominic Egger
@GrafBlutwurst
PR is up to be looked at. It has a couple open points that I don't know how to tackle so I'd be extremely happy for some help
Julien Richard-Foy
@julienrf
@GrafBlutwurst great, I will have a look ASAP, thanks!
Dominic Egger
@GrafBlutwurst
thank you!
Julien Richard-Foy
@julienrf
@GrafBlutwurst I am not familiar with http4s but I have left a couple of comments, let me know what you think
Dominic Egger
@GrafBlutwurst
:+1: thanks!
Dominic Egger
@GrafBlutwurst
you were indeed right @julienrf the other streams need to be forced as well, it was missing tests. I'm currently adding them. Regarding the change of the Endpointsignature, how much of a problem is that gonna be for users since we're gonna break signature compatibility
Dominic Egger
@GrafBlutwurst
also changing the effect type to be Resource wrapped does work!
Julien Richard-Foy
@julienrf
I think it’s okay to introduce a breaking change in the http4s interpreters, if it brings useful features
So, which type did you change, and how, to get something that works?
Dominic Egger
@GrafBlutwurst

as you proposed I changed in http4s-client this definition

  //#endpoint-type
  type EffectResource[A] = Resource[Effect, A]
  type Endpoint[A, B] = Kleisli[EffectResource, A, B]
  //#endpoint-type

(for some reason it doesn't accept * hence the type alias indirection)

which then allowed me to no longer force streams in ChunkedEntitiesand rather consume them as intended on the tests
Julien Richard-Foy
@julienrf
Good! Do you think that type also makes sense for non-streaming requests?
Dominic Egger
@GrafBlutwurst
I think it generally makes sense, because we can't know on a level where we only have a type param Respwhether it's fine to consume the resource or not. If you depend lazily on the Response in any way shape or form you're gonna be bitten the same way
though I think you have a point that these usecases are probably pretty sparse
MR is updated
Julien Richard-Foy
@julienrf
Just an idea: instead of a type alias to Kleisli, we could define a proper type Endpoint[A, B] with a convenient operation for invoking a non-chunked endpoint:
trait Endpoint[A, B] {
  def lowLevelApply(a: A): Resource[Effect, B]
  def convenientApply(a: A): Effect[B] = ... 
}
Dominic Egger
@GrafBlutwurst
that is probably a good idea. Namingwise what do you think? perhaps .resourceand .use ?
Dominic Egger
@GrafBlutwurst
done