vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
608 stars 166 forks source link

Parametrized routes #3474

Closed mvysny closed 6 years ago

mvysny commented 6 years ago

When I implement an entity detail page and I want to follow the REST guidelines, I want the URL to appear in the form of http://localhost:8080/car/25 - that is, the car route followed by the car ID. It would be great if I could write @Route("car/{id}") (in the spirit of JAX-RS path params), and then I could retrieve this parameter from some parameter HashMap provided by the AfterNavigationEvent class.

I know I could use http://localhost:8080/car?id=25 and retrieve it via Location.getQueryParameters() but I'd advise against it:

pleku commented 6 years ago

Have you read https://vaadin.com/docs/v10/flow/routing/tutorial-router-url-parameters.html ? Why won't you use HasUrlParameter then instead ? It was designed for this, though the declaration for the parameter different from your proposal.

Closing this as invalid, the same JAX-RS approach for route registration with parameters is already discussed in https://github.com/vaadin/flow/issues/2740.

mvysny commented 6 years ago

I was intuitively expecting path parameters, simply because it is a) simple, b) flexible and c) used in JAX-RS, therefore intuitively expected by Java programmers.

I was unaware of the HasUrlParameter interface existence and HasUrlParameterPair<U, V> and @Wildcard and @RoutePrefix. That looks quite complex - a lot of deus-ex-machina magic which needs to be combined cleverly to achieve something as easy as /orders/{id}/edit. Also those things are undiscoverable unless you read the documentation, which is a "complex API" smell.

Apparently, three developers (me, @heruan and @vlukashov ) intuitively expected parameters in @Route instead of that annotation-, interface- and mixin-mania. It may be enough to consider the API redesign.

Legioth commented 6 years ago

Path parameters are indeed more flexible, but does on the other hand rely on magic strings for matching the name in the @Route value when accessing the parameter value.

Also, the use of an interface gives a natural location for actually receiving the parameter and does additionally make it possible to use a typesafe API for generating URLs to a given navigation target with a matching parameter value.

In either case, there's one non-obvious concept that the developer needs to find out from the documentation. The main benefit of path parameters is familiarity and flexibility whereas the main benefit of an interface based approach is type safety.

mvysny commented 6 years ago

I am all in for type safety, but sometimes it comes with a price, and often that price is an increased complexity. A couple of remarks:

  1. Both Java Pattern matching and JAX-RS also has "magic" parameters. It would be possible to convert those APIs to type safe but it would make them more complex, and thus the type safety was deliberately left out.
  2. Using HasUrlParameterPair does not immediately solve the /orders/{id}/edit case which is quite common in REST world. But I have to consider that perhaps the REST compliancy is actually not that important with Vaadin.
  3. Having both HasUrlParameter and AfterNavigationObserver raises the question of the order in which those callbacks are called. Yes I'm sure it's documented, but it's something I would forget in an instant; I'm afraid that programmers will forget that as well and will either resort to a state automaton, or to a fragile init code split into multiple methods.

What I'm worried is that shooting for type safety will bring in (potentially large) complexity and would kill the developer experience. Yet it seems that this is in line with the apparent path of Java 8 towards complexity, so maybe I'm worried for nothing.

Legioth commented 6 years ago

I don't see how the interface-based approach would be any more complex.

With string-based parameters, you need 3-4 separate pieces: 1) Parameter name embedded inside the @Route value. 2) Navigation target class implements e.g. AfterNavigationObserver to have a method that is triggered when parameter values are available 3) Explicitly retrieve the parameter value with a specific name from the event. 4) (Optional) Convert the string to the type that you business logic uses

With HasUrlParameter, you only have one moving piece. 1) Implement HasUrlParameter with the desired type parameter.

In addition to type safety, another big benefit of this design that it makes it really easy to generate the URL for a given navigation state, (e.g. new RouterLink("Edit", OrderEditView.class, 42)).

mvysny commented 6 years ago

True. However, with HasUrlParameterPair:

  1. You still need to implement the HasUrlParameterPair interface and setParameter() method.
  2. The actual URL is never visible in sources and is always constructed internally by Vaadin code. There is no @Route("person/25/edit"), just @Route("person")
  3. You can't route person/25/edit to one view, person/25/sack to another view
  4. Can you use Enum as the second parameter? E.g. Op.Edit and Op.Sack? If not, it will be just string and therefore type-unsafe, and therefore it would be hard to guess what to put into new RouterLink("Sack", PersonView.class, 42, "?")
  5. You can't simply add third parameter unless Vaadin decides to provide HasUrlParameterTriplet and add a support of that into RouterLink
  6. With Vaadin 8 I used to have public static void sack(long id) and public static void edit(long id) on PersonView which would perform the type-safe navigation (even though it would construct the URL in a type-unsafe manner, I don't mind since it's constrained to the PersonView class). I wonder how to achieve that with RouterLink; perhaps it can be done by using the Router directly...
Legioth commented 6 years ago
  1. Yes, you implement one interface, and then the IDE takes care of suggesting the right method for you to create.
  2. What's the use case for having it visible?
  3. Why do the URLs have to look like that? What's wrong with person/edit/25 and person/sack/25 instead or (edit-person/25 and sack-person/25)?
  4. Enums should be supported, and if they're not, then that's a separate issue.
  5. The original design suggested HasUrlParameter, HasUrlParameterPair and optionally also HasUrlParameterTriplet. For the very rare cases when that wouldn't be enough, it was also suggested that HasUrlParameter would support complex types (i.e. beans) that would be collected from multiple path segments.
  6. Vaadin 8 didn't in any way help you with those methods. You had to call them separately, which is also the case now. Once we add HasUrlParameterPair, there should also be a corresponding helper in both RouterLink. There should probably also be corresponding helper overloads of UI.navigateTo, so that sack(long id) could do something like ui.navigateTo(PersonView.class, id, Op.Sack).

I suspect that we will also want to implement support for defining more complex routes based on URL patterns, but I still believe that the interface way should be the recommended alternative for all the simple cases.

mvysny commented 6 years ago

What's the use case for having it visible?

It is often important to find which view exactly handles given link in a browser, especially if you have 50+ views. (side note: it would be awesome if Designer was able to perform this kind of lookup :). However, it is true that this is possible to look up the view even with HasUrlParameter* scheme. But in case of doubt, it is good to have the URL in @Route represented as most accurately as possible.

What's wrong with person/edit/25 and person/sack/25

  1. Consider a project where a Vaadin 10 app is scheduled to replace a legacy app with fixed URL scheme (e.g. preserving links from external systems such as from a wiki). In that case the URL format is pre-dictated and may be fixed to person/25/edit.
  2. Since sack() is an operation on person, it is intuitive to write person[25].sack() instead of having a sack operation on the list of personnel like person.sack(25). The URL naming scheme just follow this logic. Unfortunately I'm not that skilled with REST so I can't back this feeling by any kind of a solid theory.

https://stackoverflow.com/questions/26357211/rest-url-path-parameter-naming-conventions

For the very rare cases when that wouldn't be enough, it was also suggested that HasUrlParameter would support complex types (i.e. beans) that would be collected from multiple path segments.

I agree, three and more parameters in URLs are pretty uncommon I guess. The 'beans' approach looks scarily complex, but perhaps it may work. However, please do consider that your understanding of the Flow API is the most complete one on this planet - no customer will have that level of understanding and may be confused by the solution.

Vaadin 8 didn't in any way help you with those methods. You had to call them separately, which is also the case now.

True. My point was that I've created my own type-safe API which required some additional work to create, but then it is easier to use and easier to call than calling the (double-)generified RouterLink or navigateTo methods. But maybe not.

The advantage of the parametrized URL schema as I can see it is that it is simple to understand (it can immediately be seen from the code what URL this view maps to); if the practice of creating customized navigational functions (such as sack(long id)) is adopted in the project, then the type safety of the interfaces play no role since all clients will just use the sack(long id) method.

heruan commented 6 years ago

Isn't it possible already in Flow to do something like this?

@Route("users")
public class UserList extends VerticalLayout implements RouterLayout {}

@Route("users")
public class UserCard extends VerticalLayout implements RouterLayout, HasUrlParameter<Long> {}

@Route(value = "profile", layout = UserCard.class)
public class UserProfile extends VerticalLayout {}

Would the router map UserProfile to users/123/profile? If this works, the parameter value can be retrieved from the active chain (possibly with a convenience method).

heruan commented 6 years ago

Tested: not working, it goes 404 with Could not navigate to 'users/123/profile'. But making this work wouldn'd be a valid alternative to magic strings in route paths?

mvysny commented 6 years ago

@heruan I'm not sure if it is a good idea to tie the UI layout nesting with the URL nesting. Also this URL composition would make the act of finding a view from given URL quite hard.

heruan commented 6 years ago

From my perspective, JAX-RS magic is cool but also hardly maintainable without IDE plugins; e.g. Eclipse warns you if you're missing a @PathParam("id") argument from your @Path("/users/{id}") method. It makes refactoring safe-ish, but without IDE help large projects with this type of syntax become quickly hard to maintain (after years of JAX-RS, we have switched all our APIs to GraphQL with a huge gain on maintainability and type-safety).

That's why I would avoid string magic as hell, but still I do need to nest routes under parameterized ones and my use cases would all have also nested layouts, e.g. in the UserCard a top section with user main info and a lower tab-like layout with nested routes for each tab.

So I see three ways out of this:

  1. refactoring the routing declarative approach with magic strings like users/{id}/profile;
  2. leave routing as is and make nested layouts work with parameterized paths;
  3. do not support nesting at all.

Predictably I'd vote for 2 😄

mvysny commented 6 years ago

I personally don't like magic, that's why I tend to use JAX-RS cautiously. But I have yet to see a simple type-safe alternative to JAX-RS. JAX-RS is not one of the nicest ones but it works. In the worst case of parameter misconfiguration the compiler won't unfortunately complain, but JAX-RS will immediately fail in runtime with a helpful message, so it's better than nothing.

Don't get me wrong: I am all in for compiler-enforced type safety; even better if the type system can guide you to use the API correctly. However, if this conflicts with simplicity, I tend to prefer simplicity. Of course others may choose otherwise - Scala guys, having experience by being burned by string magic, etc. There are multiple ways to skin a cat, and it depends on the target user group which should be chosen.

I love simplicity and therefore I selfishly vote for 1 :-)

heruan commented 6 years ago

So your desiderata aren't magic strings, but being able to nest parameterized routes without layout requirements. Then I guess a @ParentRouteTarget annotation would be sufficient, e.g.

@Route("departments")
public class Department extends VerticalLayout implements HasUrlParameter<Long> {}

@Route("employees")
@ParentRouteTarget(Department.class)
public class Employee extends VerticalLayout implements HasUrlParameter<Long> {}

@Route("profile")
@ParentRouteTarget(Employee.class)
public class Profile extends VerticalLayout {}

Then the router should quite easily find Profile for departments/123/employees/456/profile and provide all the parameters in a Map, e.g. Long department = params.get(Department.class).