Skip to content

Commit

Permalink
Scala 2.13: Fix ReverseRouting exhaustive match compilation error
Browse files Browse the repository at this point in the history
The Play Framework has had reverse routing problems with match exhaustiveness before: playframework/playframework#465

Unfortunately the fix in that PR doesn't fix the compilation erros because Scala 2.13 cannot understand that this generated code does have an exhaustive match: https://gist.github.com/ioannakok/3dcf08ebffa4ebd7deb23aa1cda3f35b

```
[error] /Users/ioanna_kokkini/Projects/frontend/identity/conf/routes:23:1: match may not be exhaustive.
[error] It would fail on the following input: (_, _)
[error] GET         /form/:formReference                    controllers.FormstackController.formstackForm(formReference: String, composer: Boolean = false)
[error] /Users/ioanna_kokkini/Projects/frontend/applications/conf/routes:51:1: match may not be exhaustive.
[error] It would fail on the following input: (_, _, _, _, _)
[error] GET        /embed/atom/:atomType/:id                                            controllers.AtomPageController.render(atomType: String, id: String, isJsEnabled: Boolean = true, hasVerticalScrollbar: Boolean = false, inApp: Boolean = false)
```

One fix is to make sure that the routes are pointing to specific methods so that reverse routing doesn't try to pattern match on the parameters. We've introduced wrapper methods with different names so that the choice is explicit.

We're removing the route /embed/atom/:atomType/inapp because it became obsolete in 2019: https://github.com/guardian/frontend/pull/21897/files#r941555632

We have verified that this endpoint is not called anymore: https://logs.gutools.co.uk/s/dotcom/goto/4f0cdb70-1801-11ed-b499-9b0038f50029

Co-authored-by: Roberto Tyley <roberto.tyley@guardian.co.uk>
  • Loading branch information
ioannakok and rtyley committed Aug 9, 2022
1 parent 0d1510d commit 4cb721e
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 6 deletions.
5 changes: 4 additions & 1 deletion applications/app/controllers/AtomPageController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,15 @@ class AtomPageController(
)
}

def renderNoJs(atomType: String, id: String): Action[AnyContent] = render(atomType, id, false, false)

def renderNoJsVerticalScroll(atomType: String, id: String): Action[AnyContent] = render(atomType, id, false, true)

def render(
atomType: String,
id: String,
isJsEnabled: Boolean,
hasVerticalScrollbar: Boolean,
inApp: Boolean,
): Action[AnyContent] =
Action.async { implicit request =>
lookup(s"atom/$atomType/$id") map {
Expand Down
7 changes: 3 additions & 4 deletions applications/conf/routes
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,9 @@ GET /index/contributors
GET /index/contributors/*contributor controllers.TagIndexController.contributor(contributor)

GET /embed/video/*path controllers.EmbedController.render(path)
GET /embed/atom/:atomType/:id controllers.AtomPageController.render(atomType: String, id: String, isJsEnabled: Boolean = true, hasVerticalScrollbar: Boolean = false, inApp: Boolean = false)
GET /embed/atom/:atomType/:id/nojs controllers.AtomPageController.render(atomType: String, id: String, isJsEnabled: Boolean = false, hasVerticalScrollbar: Boolean = false, inApp: Boolean = false)
GET /embed/atom/:atomType/:id/nojs/scroll-y controllers.AtomPageController.render(atomType: String, id: String, isJsEnabled: Boolean = false, hasVerticalScrollbar: Boolean = true, inApp: Boolean = false)
GET /embed/atom/:atomType/:id/inapp controllers.AtomPageController.render(atomType: String, id: String, isJsEnabled: Boolean = true, hasVerticalScrollbar: Boolean = true, inApp: Boolean = true)
GET /embed/atom/:atomType/:id controllers.AtomPageController.render(atomType: String, id: String, isJsEnabled: Boolean = true, hasVerticalScrollbar: Boolean = false)
GET /embed/atom/:atomType/:id/nojs controllers.AtomPageController.renderNoJs(atomType: String, id: String)
GET /embed/atom/:atomType/:id/nojs/scroll-y controllers.AtomPageController.renderNoJsVerticalScroll(atomType: String, id: String)
POST /story-questions/answers/signup controllers.AtomPageController.signup()
OPTIONS /story-questions/answers/signup controllers.AtomPageController.options()

Expand Down
3 changes: 3 additions & 0 deletions identity/app/controllers/FormstackController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ class FormstackController(

val page = IdentityPage("/form", "Form")

def formstackFormEmbed(formReference: String): Action[AnyContent] =
formstackForm(formReference, true)

def formstackForm(formReference: String, composer: Boolean): Action[AnyContent] =
fullAuthAction.async { implicit request =>
if (Switches.IdentityFormstackSwitch.isSwitchedOn) {
Expand Down
2 changes: 1 addition & 1 deletion identity/conf/routes
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ GET /user/:vanityUrl/:activityType controllers.PublicProfileCon
########################################################################################################################
GET /form/complete controllers.FormstackController.complete
GET /form/:formReference controllers.FormstackController.formstackForm(formReference: String, composer: Boolean = false)
GET /form/embed/:formReference controllers.FormstackController.formstackForm(formReference: String, composer: Boolean = true)
GET /form/embed/:formReference controllers.FormstackController.formstackFormEmbed(formReference: String)

########################################################################################################################
# Account deletion
Expand Down

0 comments on commit 4cb721e

Please sign in to comment.