Result is a sealed trait

[Brennan Saeta]

Hey all,

Why is result a sealed trait? Similar to extending Requests (e.g. with WrappedRequest), it'd be great to be able to extend Results. I haven't looked carefully at how difficult extending Results would be, but it'd be very useful if we could.

Does anyone have thoughts / opinions?

Thanks!

-Brennan

[Rich Dougherty]

Hi Brennan

Out of interest, what's the use case you're thinking of?

Cheers

Rich

--
You received this message because you are subscribed to the Google Groups "Play framework dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to play-framework-...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Brennan Saeta]

I'm building a REST toolkit inside of Play. In order to make testing easier, it'd be really nice if we could keep (if nothing else) the type of the returned collection, and ideally the collection itself, as opposed to taking the JSON body, parsing it into JSON and then converting that into the case class / domain object / POSO (Plain-old-scala-object).

This is mostly just for testing purposes.

-Brennan

[Brennan Saeta]

Hey,

Unfortunately it's not so simple. As just one example of many, certain "wiki" pages aren't viewable by certain users. We want to return a HTTP 401 Unauthorized. In other cases, if another user has modified the wiki, we want to return HTTP 409 Conflict. Successful responses need ETag headers, Location headers, and others. We've looked into trying to decompose cleanly between the HTTP layer and the logic layer, but it's not so simple. :-( We could re-implement all of the HTTP Responses, but we'd rather just use the ones Play! already provides. Further, while we probably could build everything inside Play without using Play directly, it'd be much nicer if we could just extend Play!. We would like to share back some of the extra tooling as a library compatible with existing Play! applications. Finally, we don't want to write boiler-plate layers to wrap logic in a translation layer just to be able to test efficiently and cleanly. That's one of the most important reasons for we chose to standardize on Play!: Developer productivity, and a lack of boilerplate.

Let me know if I can further clarify our motivations to have a bit more control over Results. :-)

Best,

-Brennan

On Thu, Apr 25, 2013 at 8:11 PM, Rich Dougherty <ri...@rd.gen.nz> wrote:

(Cc'ing play-framework - I hope you don't mind!)

Brennan, I agree with your general approach, but I think you could do this without embedding a WikiPage into the Result object. Personally I think it's better to keep Result focused on low-level concerns like HTTP headers and bytes, and to keep your business logic in a higher layer.

What I'd suggest is that you break down your request processing logic a bit more so that you can separately unit test the different bits of internal logic within your controller.

Suppose your controller method contained code like:

  val wikiPage: WikiPage = wikiPageLogic.getWikiPage(id)

  wikiPageEncoding.encodeWikiPage(wikiPage) <-- constructs a Result object or a string to go into a Result

You can easily unit test your business logic, i.e. wikiPageLogic.getWikiPage. Your tests would be able to operate on nice objects without needing to worry about the messy details of string encoding.

You can separately unit test your encoding logic, i.e. wikiPageEncoding.encodeWikiPage. Your encoding unit tests would deal with the messy business of making sure that objects are properly encoded into the correct strings.

Finally, if you like, you can write integration tests to make sure the controller does the right thing when it combines the business and encoding logic together. You could test that, for a given fake request, you get the correct Result. Since the encoding logic has been separated out, you can use it to help generate assertions for the expected result:

  assertEquals(wikiPageEncoding.encodeWikiPage(WikiPage(...)), res)

Cheers

Rich

On Fri, Apr 26, 2013 at 1:49 PM, Brennan Saeta <sa...@coursera.org> wrote:

Hey,

Perhaps I wasn't clear. Below is what a test currently looks like:

  @Test

  def pageNotVisibleAdmin() {

    when(wikiStore.getWikiBody(123, "page", None)).thenReturn(

      Some(sampleWikiPage().copy(visible = false), markdownBody))

    val res = controller.getWikiPage(123, "page", None)(RequestHelpers.fullFakeRequest(

      GET, "/123/page?fields=content", 123, "wiki_admin"))

    assertEquals(OK, status(res))

    assertTrue(contentAsString(res).contains("BODY")) // <--- This sucks!

  }

sampleWikiPage() - Helper function that returns a case class - used for mocking out the database.

We don't want to write crufty software with a manager, a controller, a data accesser, a factory, and a factory factory manager. That said, we cannot write "full-application" tests that boot up the whole Play stack. We want to just test our controllers in isolation, mocking out constructor-injected dependencies. Given the result from our controller, we would like to be able to test the response body. Although it's possible to take the contentAsString(res), parse that as Json, and then re-parse that back into our case class, it would be much easier if we could subclass result, and keep around the case class that generated the Json in the first place. (Also to be typesafe! Right now, the type that generated the response is lost, so although we write a bunch of typesafe code for the rest of the stack, we can't here.) This is similar to how we have the WrappedRequest available for subclassing requests (that is used quite effectively in SecureSocial, for example, to add user information, etc.) Does that make sense?

Ideally, a test looks like:

  @Test

  def pageNotVisibleAdmin() {

    when(wikiStore.getWikiBody(123, "page", None)).thenReturn(

      Some(sampleWikiPage().copy(visible = false), markdownBody))

    val res = controller.getWikiPage(123, "page", None)(RequestHelpers.fullFakeRequest(

      GET, "/123/page?fields=content", 123, "wiki_admin"))

    assertEquals(OK, status(res))

    assertEquals(WikiPage(...), restObject(res)) // <-- This is what we're looking for. Ideally it can be type-checked if we were using specs2's `====`

  }

Best,

-Brennan

On Thu, Apr 25, 2013 at 6:15 PM, Rich Dougherty <ri...@rd.gen.nz> wrote:

OK, testing on ordinary classes sounds like a good idea. :)

I think you can achieve what you want without any changes to Play. Actions basically do Request => Result, but you can decompose that into parts for easier testing.

Request => MyRequest

MyRequest => MyResult (business logic goes here)

MyResult => Result

You can even write your own kind of Action to hide the first and last parts of the pipeline.

I did something sort of similar when I wrote a little Play app to handle Github push events.

https://github.com/richdougherty/kitteh-feeder/blob/master/test/app/controllers/GithubSpec.scala

Would that work for you?

Cheers

Rich

[Rich Dougherty]

I wouldn't be shy about making a mini-language* to describe the possible outcome of each action, but I can understand if you don't want to go down that route. :)

If you want to leave all the logic in the controller, you could also consider factoring out and unit testing a few parts of the encoding logic. Then, when testing the controller, you could reuse that encoding logic to check the result.

  assertEquals(wikiPageEncoding.encodeWikiPage(WikiPage(...)), res)

I'll leave others to comment on your original query about making Result extendable.

Cheers
Rich

* E.g.

sealed trait ViewOutcome

case class Display(wikiPage: WikiPage) extends ViewOutcome

case class WrongPage(rightPage: String) extends ViewOutcome

case class Conflict(otherUser: User) extends ViewOutcome

case object Unauthorized extends ViewOutcome