finatra-http RouteDSL - Support optional route when prefix present

[acheek24]

RouteDSL endpoint definitions should not require a route if a prefix is provided.

Expected behavior

When a valid prefix is provided an empty route should be valid.

prefix("user/:userId/address") {
    get() {}
}

If I wanted to implement a controller for user addresses like above, I'd expect my GET and POST to have empty routes. Currently, I'll have to move "address" out of the prefix to satisfy validation.

Actual behavior

Route is always required to be non-empty, even when the prefix could contain the full route.

Steps to reproduce the behavior

Try to implement an endpoint with a valid prefix and a get() with no route defined.

[jyanJing]

Thank you @acheek24 for reporting, looking!

[acheek24]

@jyanJing The change seems straightforward enough. Just pulled the code down and looking to get out a PR shortly if this seems like a desirable change.

[jyanJing]

Thank you @acheek24 for working on it, sgtm!

[cacoco]

@acheek24 I left a comment on your PR. The example for this issue does not make sense to me. Prefixes are very simple (and should remain as such). Attempting to place an "empty" route under a prefix is generally not something we would ever recommend nor do I think we want to enable.

Can you please explain more the use case for this? We would recommend instead:

get("user/:userId/address") { request: Request =>
   ???
}

post("user/:userId/address") { request: Request =>
  ???
}

which is much more straightforward and easier to reason about. If you want to DRY this, then simply:

private[this] final val UserAddressURI = "user/:userId/address"

get(UserAddressURI) { request: Request =>
   ???
}

post(UserAddressURI) { request: Request =>
  ???
}

[acheek24]

Thanks for responding! I'll just provide you a recent implementation that I tried to add and what I did instead. I agree that it's not the end of the world to do any of the things you suggested (somebody would've suggested a change before now), but I also think it's convenient and completely valid if the full route happens to be the prefix. See below.

What I wanted my controller to look like...

prefix("/participant/:participant_id/activity") {
  get() { participantActivityFind: ParticipantActivityFind =>
    find()
  }

  post() { participantActivityAdd: ParticipantActivityAdd =>
    create()
  }

  put("/:id") { participantActivityUpdate: ParticipantActivityUpdate =>
    update()
  }

  delete("/:id") { participantActivityDelete: ParticipantActivityDelete =>
    delete()
  }
}

But ended up going with...

prefix("/participant/:participant_id") {
  get("/activity") { participantActivityFind: ParticipantActivityFind =>
    find()
  }

  post("/activity") { participantActivityAdd: ParticipantActivityAdd =>
    create()
  }

  put("/activity/:id") { participantActivityUpdate: ParticipantActivityUpdate =>
    update()
  }

  delete("/activity/:id") { participantActivityDelete:
ParticipantActivityDelete =>
    delete()
  }
}

Clearly not a huge deal, but I'm not sure I see why the first option shouldn't be valid.

[jyanJing]

Thanks @acheek24 for the detailed example. I agree with what Chris said that we don't allow empty route definition, this is also mentioned in the finatra user guide where it said "Routes and Prefixes MUST begin with a forward slash (/)."

However, you can make use of the forward slash and take advantage of our trailing slash optional identifier. So for your use case, you could define your controller like:

prefix("/participant/:participant_id/activity") {
  get("/?") { participantActivityFind: ParticipantActivityFind =>
    find()
  }

  post("/?") { participantActivityAdd: ParticipantActivityAdd =>
    create()
  }

  put("/:id") { participantActivityUpdate: ParticipantActivityUpdate =>
    update()
  }

  delete("/:id") { participantActivityDelete: ParticipantActivityDelete =>
    delete()
  }
}

So later on your route get("/participant/:participant_id/activity") will be routed to the find() endpoint.

[acheek24]

Sure, that works too. Since the change isn't desirable, I'll go ahead and close the PR.

[cacoco]

@acheek24 supporting empty routes are not desirable because they would make no real sense outside of the context of being within a prefix and thus currently they are able to work together and independently. I think we'd prefer to keep this flexibility. Thanks!