-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update to Play 3.x. Drop Scala 2.12 support. #5
Conversation
1b3f4e4
to
7484c5a
Compare
I believe the CI failure is caused by this change in the test suite. |
@julienrf perhaps it needs new |
In the commit I linked at least, the stub-server wasn’t changed, so it may not need an update. If you look at the commit it changed the behavior of the servers (and their test expectation), so I think you need to apply the same change here. Unfortunately, that part of the code is a bit complex, so feel free to ask any question if needed! That being said, if you want to update the stub-server, I invite you to use the same approach as here, which does not go through the Docker hub (which does not contain up to date images): https://github.com/endpoints4s/xhr/blob/f309656a4186f04474f218b65fff5f7ae247a8fe/.github/workflows/ci.yml#L23 |
@julienrf i fixed the test, but i'm not sure if that's the correct way - what happens if there are several endpoints with same method, but different urls and authentication? will the new implementation match the first endpoint it finds and return failure even if there are unauthenticated endpoints that should match? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5 +/- ##
==========================================
- Coverage 58.25% 56.38% -1.88%
==========================================
Files 21 22 +1
Lines 539 1050 +511
Branches 20 27 +7
==========================================
+ Hits 314 592 +278
- Misses 225 458 +233 ☔ View full report in Codecov by Sentry. |
I am not sure I understand completely the part “different URLs”, because if they have different URLs then they may not match the same incoming request, right? But otherwise, yes, the first endpoint whose method + URL path match the incoming request will handle the request. If that endpoint requires authentication, it will return an |
Left( | ||
Results.Unauthorized | ||
.withHeaders(HeaderNames.WWW_AUTHENTICATE -> "Basic realm=Realm") | ||
if (method.matches(requestHeader)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised that you don’t also need to match the URL path 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be possible to implement it by still calling matchRequestAndParseHeaders
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this is no good, i'll reimplement
can i suggest adding something to testkit that can check if endpoints clash?
like this
"basic auth" should {
"not prevent other endpoints from matching" in {
val response = "Hello!"
import serverApi._
val protectedEndpointWithParameter = authenticatedEndpoint(
Get,
path / "users" / segment[Long]("id"),
ok(textResponse)
)
val unprotectedEndpointWithParameter = endpoint(
get(path / "users"),
ok(textResponse)
)
serveRoutes(
routesFromEndpoints(
protectedEndpointWithParameter.implementedBy(_ => Some(response)),
unprotectedEndpointWithParameter.implementedBy(_ => response),
)
){ port =>
val request = HttpRequest(uri = s"http://localhost:$port/users")
whenReady(sendAndDecodeEntityAsText(request)) { case (_, entity) =>
entity shouldBe response
()
}
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, any addition to the test kit is welcome!
To fix the CI failure you need to relax the compatibility intention to Line 9 in 9b60787
|
@julienrf hm actually it looks like current testkit is wrong endpoints4s/endpoints4s#1222 (the new test fails) |
hm, maybe it's just a problem with pekko implementation, as http4s passes the new test |
@julienrf i fixed pekko, can you please release new testkit? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me apart from a couple of minor comments, thank you @OlegYch!
No description provided.