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.

Sign up for GitHub

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

Jump to bottom

Complete directives take ResponseEntity as a parameter instead of RequestEntity #982

Merged
merged 4 commits into from Apr 26, 2017
Merged

Complete directives take ResponseEntity as a parameter instead of RequestEntity #982

merged 4 commits into from Apr 26, 2017

Conversation

btomala
Copy link
Contributor

@btomala btomala commented Mar 20, 2017

Issue: 641
complete directives take ResponseEntity as a parameter instead of RequestEntity

I think in next steps Marshaller[T, RequestEntity] should be replaced by Marshaller[T, ResponseEntity]. But it requires #642 first.

@akka-ci
Copy link

akka-ci commented Mar 20, 2017

Can one of the repo owners verify this patch?

@jlprat
Copy link
Member

jlprat commented Mar 21, 2017

OK TO TEST

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR that is currently being validated by Jenkins labels Mar 21, 2017
@akka-ci
Copy link

akka-ci commented Mar 21, 2017

Test FAILed.

@jrudolph
Copy link
Member

I guess this change isn't binary compatible. We might have to introduce new overloads and deprecate the old ones for now.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Mar 21, 2017
@akka-ci
Copy link

akka-ci commented Mar 21, 2017

Test PASSed.

@jlprat
Copy link
Member

jlprat commented Mar 21, 2017

Could we maybe add a test using the new overloaded complete methods?

@btomala
Copy link
Contributor Author

btomala commented Mar 21, 2017

I could but I think it doesn't make sense. Because there are test for old API and old API use new one, so it is already tested. Even more when depreciated functions will be remove then test will be use new API. Because we have source compatibility.

@jlprat
Copy link
Member

jlprat commented Mar 21, 2017

Oh yeah sorry, I forgot that RequestEntity extends ResponseEntity!

jlprat
jlprat approved these changes Mar 21, 2017
View reviewed changes
Copy link
Member

@jlprat jlprat left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@btomala
Copy link
Contributor Author

btomala commented Mar 22, 2017

Thanks :)

jrudolph
jrudolph reviewed Mar 27, 2017
View reviewed changes
akka-http/src/main/scala/akka/http/javadsl/server/directives/RouteDirectives.scala Outdated
D.complete(scaladsl.model.HttpResponse(status = status.asScala, entity = entity.asScala, headers = Util.immutableSeq(headers).map(_.asScala))) // TODO avoid the map()
}

/**
* Completes the request using the given status code, headers, and response entity.
*/
@deprecated("This directive is for binary compatibility only", "10.0.6")
Copy link
Member

Choose a reason for hiding this comment

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

I think we should try out in which cases this overload is going to be used. If the most specific overload is picked, then it might be always this one (apart from CloseDelimited which isn't a RequestEntity). What we don't want is that every legitimate usage of complete is flagged with a deprecation warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for long time without response.. Yes you have right, the most specific one is picked. I removed annotation and leave overloaded functions.

jrudolph
jrudolph reviewed Mar 27, 2017
View reviewed changes
akka-http/src/main/scala/akka/http/javadsl/server/directives/RouteDirectives.scala Outdated
*/
@deprecated("This directive is for binary compatibility only", "10.0.6")
def complete(status: StatusCode, headers: java.lang.Iterable[HttpHeader], entity: RequestEntity): RouteAdapter =
complete(status, headers, entity.asInstanceOf[ResponseEntity])
Copy link
Member

Choose a reason for hiding this comment

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

entity: ResponseEntity should work as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Apr 25, 2017
@akka-ci
Copy link

akka-ci commented Apr 25, 2017

Test PASSed.

jrudolph
jrudolph suggested changes Apr 26, 2017
View reviewed changes
Copy link
Member

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

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

I think this should have tests because it's easy to get the forwarding wrong and end up with spinning recursive calls.

@btomala
Copy link
Contributor Author

btomala commented Apr 26, 2017

How you want to test this? It is already tested in CodingDirectivesTest. In test it is called complete for RequestEntity and this call complete for ResponseEntity. What should I additionally test here?

@jrudolph
Copy link
Member

Ah, you are right, all of those go through the new and old methods. Sorry for the noise.

@jrudolph jrudolph merged commit 28fdadd into akka:master Apr 26, 2017
@jrudolph
Copy link
Member

Thanks a lot, @btomala, for fixing.

@jrudolph
Copy link
Member

Fixed #641.

@jrudolph jrudolph mentioned this pull request Apr 26, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrudolph jrudolph jrudolph requested changes

@jlprat jlprat jlprat approved these changes

Assignees
No one assigned
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@btomala @akka-ci @jlprat @jrudolph