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

Preserved request metadata in response #2146

Merged
merged 5 commits into from
Apr 25, 2024
Merged

Conversation

Pask423
Copy link
Contributor

@Pask423 Pask423 commented Apr 23, 2024

closes #1719 #2106

@Pask423 Pask423 changed the title Preserved request metadata in response [WIP] Preserved request metadata in response Apr 23, 2024
import sttp.client4.Response
import sttp.model._

private[sttp] object TestResponse {
Copy link
Member

Choose a reason for hiding this comment

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

maybe let's call it ResponseStub, keeping the terminology consistent with BackendStub? Also, this should be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah good idea with naming, as for public hmmm not sure if we want it to be expose to users ?

Copy link
Member

Choose a reason for hiding this comment

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

yeah we do, it was exposed so far, and the idea is to use it when defining the behavior of a backend stub

/** Mainly useful in tests using [[sttp.client4.testing.SttpBackendStub]], when creating stub responses.
*/
val ExampleGet: RequestMetadata = new RequestMetadata {
val emptyGet: RequestMetadata = new RequestMetadata {
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should remove all of these methods from Response once TestResposne is there?

Copy link
Member

Choose a reason for hiding this comment

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

ah sorry, we should only keep the helper methods, but with requestMetadata: RequestMetadata. We should move emptyGet, though, which kind of is the point of the issue that is being fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a problem here with the methods here as we use them in AbstractStubBackend when request.metadata is not computed yet, I think that we will have to leave it
image

emptyGet should not be available outside sttp package as Respone is package scope, I thought it should be ok from user perspective

Copy link
Member

Choose a reason for hiding this comment

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

emptyGet or whatever the name should still live in ResponseStub/TestResponse. The point of the issue is that this shouldn't be exposed in Response, but explicitly marked as a test-only thing. In a backend stub, we can of course use a ResponseStub

@@ -77,7 +77,7 @@ abstract class AbstractBackendStub[F[_], P](
def thenRespond[T](body: T, statusCode: StatusCode): Self = thenRespond(Response[T](body, statusCode))
def thenRespond[T](resp: => Response[T]): Self = {
val m: PartialFunction[GenericRequest[_, _], F[Response[_]]] = {
case r if p(r) => monad.eval(resp)
case r if p(r) => monad.eval(resp.copy(request = r.onlyMetadata))
Copy link
Member

Choose a reason for hiding this comment

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

would be good to have a test for this

@Pask423 Pask423 changed the title [WIP] Preserved request metadata in response Preserved request metadata in response Apr 24, 2024
# Conflicts:
#	observability/prometheus-backend/src/test/scala/sttp/client4/prometheus/PrometheusBackendTest.scala
@adamw
Copy link
Member

adamw commented Apr 25, 2024

Great, thanks!

@adamw adamw merged commit 8bf12f9 into master Apr 25, 2024
15 checks passed
@adamw adamw deleted the request-metadata-cleanup branch April 25, 2024 13:43
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.

Make it harder to use ExampleGet and connected Reponse factory methods
2 participants