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.

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

Router: add support for routes like /orders/:id/edit where the parameter is in the middle #2740

Closed
vlukashov opened this issue Oct 20, 2017 · 29 comments · Fixed by #7608
Closed
Assignees
Milestone

Comments

@vlukashov
Copy link

When defining a routes map of an application, as a developer, I want to use routes where the route parameter is not the last segment of the URL (e.g. /orders/:id/edit). With the new Router API that is not supported at the moment (flow-1.0.0.alpha6).

@caalador
Copy link
Contributor

Would this actually be the same as HasUrlParameterPair<T, U> when using @Route("orders") and HasUrlParameterPair<Integer, String>

@Legioth
Copy link
Member

Legioth commented Oct 24, 2017

This functionality was left out from the design in order to keep it simple.

The simplest workaround is probably to change the URL structure to /orders/edit/:id instead.

The suggested workaround of using HasUrlParameterPair<Integer, String> has some limitations when there are multiple different "sub routes", e.g. /orders/:id/edit and /orders/:id/details.

Some ideas for this could be supported:

  1. Allow a router layout (or section) to have URL parameters (with some restrictions, e.g. no @Optional or @Wildcard).
  2. Reuse the idea of using HasUrlParameterPair<Integer, String>, but add some syntax for pattern matching on string parameters to resolve ambiguity between multiple navigation targets with the same base @Route.
  3. Add something like @RoutePrefix("edit") that is used to resolve between multiple navigation targets with the same @Route.
  4. Support arbitrary patterns with placeholders for routes (e.g. @Route("orders/:id/edit")). With this scheme, there's the possibility of having a mismatch between the number of placeholders in the URL and the arity supported by the navigation target class itself.
    Regardless of the approach, one additional problem is that only one of the router layout and the navigation target will receive the id parameter in a typesafe way. If the other party also needs to know the id, it would either have to manually extract and convert it or the classes would have to explicitly pass the received parameter to the other instance.

@heruan
Copy link
Member

heruan commented Oct 25, 2017

On this regard, I find very intuitive and productive the JAX-RS approach, i.e.

@Path("orders/{id}/edit")
public void editOrder(@PathParam("id") Long id) {
    // ...
}
  1. it uses the URI Template syntax (RFC6570), which can easily render URI strings given the template and the arguments;
  2. it supports regex matching, e.g. @Path("orders/{id: [0-9]+}/edit");
  3. it can be placed on both classes and methods, to create hierarchies easily:
@Path("orders")
public class OrderRoute {

    @Path("{id}")
    public Component orderCard(@PathParam("id") Long id) {
        // ...
    }

    @Path("{id}/edit")
    public Component orderForm(@PathParam("id") Long id) {
        // ...
    }
}

BTW I'm not sure this approach, which fits well for stateless APIs, would fit just as well in Flow.

@Legioth
Copy link
Member

Legioth commented Oct 26, 2017

If we are going to support a placeholder syntax in @Route values, then it seems like the URI Template syntax would be a good fit.

The overall problem with any such approach is that it's based on boilerplate annotations with magic strings (e.g. @PathParam("id")) for mapping between URI parameters and method parameters. Those annotations could be avoided in most cases with a "magic" convention that falls back to the order in which the parameters are defined.

It might still be that that boilerplate annotations or a magic convention is the price that must be paid for supporting this level of flexibility. It would still not be needed in simpler cases where the parameters are at the end of the URL.

There is a separate discussion about allowing @Route on methods in #2666.

@heruan
Copy link
Member

heruan commented Feb 16, 2018

Moving here some thoughts from #3474:

@heruan said:

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 said:

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 said:

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).

One of the apps I'm porting (from Aurelia) to Flow is growing fast and this would be really necessary for a smooth port. Could the scenario I described in my last comment above be approachable and fit all the use cases?

@Legioth
Copy link
Member

Legioth commented Feb 16, 2018

@heruan I'm not sure I understand exactly how the different parts would work together in your example. I assume that e.g. the Profile class would need to know which employee it should show, but how would it find that information?

@heruan
Copy link
Member

heruan commented Feb 16, 2018

My example is a bit hermetic, I get it 😃 Instead of (or in addition to) a @ParentRouteTarget annotation we could have a HasParentRoute<T, C extends HasUrlParameter<C>> and use it like this:

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

@Route("employees")
public class Employee extends VerticalLayout implements
        HasUrlParameter<Long>, HasParentRoute<Long, Department> {
    // override setParameter
    @Override
    public void setParentRouteParameter(Long departmentId) {
    }
}

@Route("profile")
public class Profile extends VerticalLayout implements HasParentRoute<Long, Employee> {
    // override setParameter
    @Override
    public void setParentRouteParameter(Long employeeId) {
    }
}

Or, for more complex case (e.g. one need a parameter deeper in the hierarchy) provide a map to obtain the parameter from the component class, e.g. params.get(Employee.class).

@heruan
Copy link
Member

heruan commented Feb 19, 2018

The more use cases I'm facing while porting a structured app to Flow, the more it seems to me this could be combined to router layouts without refactoring the entire routing system, staying simple and straightforward for both developers and users.

The culprit would be supporting HasUrlParameter<T> on parent layouts, and a HasParentLayout<L extends RouterLayout> with a method to provide the parent layout instance.

For example, say I have a RouterLayout which provides a navigation bar with router links, e.g.

@RoutePrefix("users")
public class UserSectionLayout implements RouterLayout, HasUrlParameter<Long> {
                                                        ^^^^^^^^^^^^^^^
    HorizontalLaytour navbar = new HorizontalLayout();

    VerticalLayout content = new VerticalLayout();

    Long id;

    public UserSectionLayout() {
        this.add(navbar, content);
    }

    public Long getUserId() {
        return this.id;
    }

    @Override
    public void setParameter(BeforeEvent event, Long id) {
        this.id = id;
        this.navbar.add(new RouterLink(UserProfileRoute.class, id));
        this.navbar.add(new RouterLink(UserOtherRoute.class, id));
    }

    @Override
    public void showRouterLayoutContent(HasElement content) {
        this.content.getElement().appendChild(content.getElement());
    }

}

@Route(value = "profile", layout = UserSectionLayout.class)
public class UserProfileCard extends VerticalLayout implements HasParentLayout<UserSectionLayout> {

    @Override
    public void setParentLayout(UserSectionLayout parentLayout) {
        Long id = parentLayout.getUserId();
        User user = UserBackend.getUser(id);
    }

}

@Route(value = "other", layout = UserSectionLayout.class)
public class UserOtherCard extends VerticalLayout implements HasParentLayout<UserSectionLayout> { /* ... */ }

This would provide URLs like users/123/profile and users/123/other and the router should be able to register the routes quite easily since UserOtherCard is mapped to other and has a parent layout with users prefix and implements HasUrlParameter<Long>.

Plus, any component in the activation chain can get an instance of its parent layout and access any parameter in the hierarchy.

Note: having parent layouts with URL parameters is the only way I can guess to add navigation links on them when their child components are parameterized too.

@Legioth
Copy link
Member

Legioth commented Feb 20, 2018

It seems like both alternatives (HasParentLayout or HasParentRoute) could make routing quite straightforward, but I'm not sure about URL generation. What would the typesafe API look like for generating a URL like users/123/profile?

There could in theory be a monster like public <T, C HasParentLayout<? extends HasUrlParameter<T>>> String getUrl(Class<? extends C> navigationTarget, T parameter). This pattern would lead to a permutational explosion if we also want to support one parameter for the parent and one parameter for the child (departments/123/employees/456) and/or multiple parameters for the same class (HasUrlParameterPair).

One potential solution to this could be some kind of fluid builder that would allow defining one set of parameters per method call instead of doing everything through only one getUrl invocation. It could then be something like buildUrl(parentClass, param1, param2).withChild(childClass, param3).build();.

One additional observation is that it's seems redundant to define layout = UserSectionLayout.class in the @Route annotation since the same information is already in the type parameter. On the other hand, defining the parent in the annotation is more discoverable and feels slightly less verbose. Supporting both is of course also an option, even though there might also be some confusion if there are two quite similar ways of doing basically the same thing.

We might still also want to support placeholders in the route mapping strings, e.g. for cases when a view needs to access parameters from multiple parent layouts.

@heruan
Copy link
Member

heruan commented Feb 20, 2018

One potential solution to this could be some kind of fluid builder […]

Totally agree, a builder would be versatile and safe.

We might still also want to support placeholders in the route mapping strings, e.g. for cases when a view needs to access parameters from multiple parent layouts.

Not necessary if layouts provide getters, since you can descend in the hierarchy, e.g. for

@RoutePrefix("departments")
public class DeparmentsLayout implements HasUrlParameter<Long> {
    // ...
    public Long getDepartmentId() { /* ... */ }
}

@RoutePrefix("employees")
public class EmployeeLayout implements HasUrlParameter<Long>, HasParentLayout<DepartmentsLayout> {
    // ...
    public DepartmentsLayout getDepartmentsLayout() { ... }
}

@Route("profile")
public class EmployeeProfile implements HasParentLayout<EmployeeLayout> {

    @Override
    public void setParentLayout(EmployeeLayout parentLayout) {
        Long employeeId = parentLayout.getEmployeeId();
        DepartmentsLayout dl = parentLayout.getDepartmentsLayout();
        Long departmentId = dl.getDepartmentId();
    }

}

I'd avoid placeholders in route mappings, which in my opinion would open a Pandora's box of convoluted logic to provide the values to the user in a typesafe way (if possible at all).

@mvysny
Copy link
Member

mvysny commented Feb 20, 2018

Is it really that important to have URLs generated in a type-safe way? Are we willing to introduce all those builders and type safety and make users to use that, to avoid simpler but dynamic solution with placeholders? As if what happened to Scala was not scary enough.

@Legioth
Copy link
Member

Legioth commented Feb 20, 2018

Everyone has their own preference when it comes to potential tradeoffs with type safety, so I think we should consider supporting both approaches.

This would be in line with e.g. Binder where you can choose between typesafe lambdas for the property getter and setter, or alternatively giving the property name as a String and then let reflection take care of the rest.

manolo added a commit to vaadin/bakery-app-starter-flow-spring that referenced this issue Feb 20, 2018
Note that URL format has been changed from `storefront/123?edit=` to
`storefront/edit/123` which is more REST standard.

Jira: BFF-614

Related with:
Jira: BFF-354
vaadin/flow#2740
manolo added a commit to vaadin/bakery-app-starter-flow-spring that referenced this issue Feb 20, 2018
Note that URL format has been changed from `storefront/123?edit=` to
`storefront/edit/123` which is more REST standard.

Jira: BFF-614

Related with:
Jira: BFF-354
vaadin/flow#2740
alexberazouski pushed a commit to vaadin/bakery-app-starter-flow-spring that referenced this issue Feb 20, 2018
* Handle the edit verb in the URL for orders

Note that URL format has been changed from `storefront/123?edit=` to
`storefront/edit/123` which is more REST standard.

Jira: BFF-614

Related with:
Jira: BFF-354
vaadin/flow#2740

* Merge branch 'master' into fix/614-order-edit-url
@heruan
Copy link
Member

heruan commented Mar 1, 2018

A bit as an exercise (and much because we really need this to port our apps to Flow 😃) I've implemented a working-ish solution with router layouts: master...heruan:layout-url-parameters

Basically, when a RouterLayout implements HasUrlParameter, a pattern is appended to the route prefix of that layout and the resolver matches the pattern with the requested path, collects the parameters and sets them on the corresponding instance.

Then, any component in the chain implementing HasParentLayout (new interface) has the layout set, providing a way to get all the parameters in the layout chain.

Of course this is not finished yet (all router tests succeed, but a couple of other tests regarding route aliases for viewport and theme still fail), but I'm sharing this so if the feature doesn't get into 10 it might help others who need it from the start!

@heruan
Copy link
Member

heruan commented May 2, 2018

Any chance for this to be milestoned after 1.0 release?

@pleku pleku added this to the After 1.0 candidates milestone May 31, 2018
@heruan
Copy link
Member

heruan commented Sep 5, 2018

This is becoming more pressing as time passes; I'll try to give a different point of view than the URL-based already discussed here.

Right now Flow assumes that parameterized views are leaves. This is quite restricting, since the subject identified by the parameter might need multiple independent views and provide links for them (for example, a customer view might have a view for details, orders, sold items, contacts, etc.). In cases like this, the parameterized view is also a router layout, providing navigation on its sub-views and possibly other common components.

This could be worked around using multiple sibling parameterized view, with links on each of them to each other. But this workaround lacks a fundamental aspect: the "main" parameterized view cannot share its state with its siblings as it would be if it were a router layout with its children.

I really hope this issue will be considered soon!

@samie
Copy link
Member

samie commented Sep 11, 2018

There is now an implementation available as extension to Vaadin Flow by @fluorumlabs : https://vaadin.com/directory/component/url-parameter-mapping

@heruan
Copy link
Member

heruan commented Sep 11, 2018

Thank you @samie for the link and @fluorumlabs for the great plugin! The POV I described before was about having parameterized router layouts, so to provide navigation links with the parameter to the sub-views. Do you think I need to open a new issue for that? I guess no, since it would also include this.

@pleku
Copy link
Contributor

pleku commented Dec 18, 2018

What's the status of this? Is this still planned for 13?

@heruan this is on the maybe list at the moment and to be honest it looks unlikely it will make it.

@remal
Copy link

remal commented Dec 18, 2018

@pleku don't you think that readable hierarchical URLs are important? Is the task really so difficult to be implemented?

If there are some not obvious difficulties, please share. It can help with creating PR...

@F43nd1r
Copy link
Contributor

F43nd1r commented Dec 18, 2018

@remal I just checked the code, and I think the main problem is that this would require an almost full rewrite of RouteRegistry, as its current structure only supports search by full path string.

@heruan
Copy link
Member

heruan commented Dec 19, 2018

@remal I've given a shot at this too (see master...heruan:layout-url-parameters) and used that for a while successfully but I couldn't keep maintaining my fork so now is not up-to-date. Still, it could be a starting point if the team could give some feedback!

@heruan
Copy link
Member

heruan commented Oct 23, 2019

This is still blocking migration of some of our apps to Vaadin, any news about this? A version milestone maybe? 🙂

@bogdanudrescu bogdanudrescu self-assigned this Nov 15, 2019
@pleku pleku modified the milestones: Candidates, V14.X Nov 18, 2019
@pleku
Copy link
Contributor

pleku commented Nov 18, 2019

@heruan sorry not commiting to any milestones yet, but we have started to take a look "on the side" for making this happen in an upcoming minor release for Vaadin 14.

I can promise that after the feature is done, it will be available in the next minor release train for Vaadin 14, which should be released within no more than three months after the feature has been done 😎

@heruan
Copy link
Member

heruan commented Nov 18, 2019

Great news @pleku, thanks! Do you have design specs for this? I'm especially interested in the role of router layouts, and how parameters will be available on their instances.

@Legioth
Copy link
Member

Legioth commented Nov 27, 2019

We already have a quite smooth way for handling the common cases through the regular HasUrlParameter interface. What we need is some way of supporting all other cases. From that point of view, flexibility and conceptual simplicity would be more important factors than e.g. type safety or avoiding magic strings. Interop with HasUrlParameter is still relevant and one way of achieving that would be to use a design where HasUrlParameter can be defined as a helper API on top of a more flexible low-level approach.

Based on those constraints, I would suggest placeholders in the @Route value and a generic map-like abstraction for accessing typed placeholder values (e.g. parameters.getAsInt("id")). The map would be available in all navigation events so that parent layouts also have easy access to the parameter values. The HasUrlParameter value would be represented using a special key (e.g. _default) in the map.

When it comes to the placeholder syntax in the @Route value, I would favour the : based syntax that is already used in e.g. our client-side router even though the URI Template syntax is a "real" standard.

@pleku
Copy link
Contributor

pleku commented May 12, 2020

To be decided which Vaadin 14 minor this lands into. Probably 14.4 even though window is still open for 14.3.

@damian-burda
Copy link

To be decided which Vaadin 14 minor this lands into. Probably 14.4 even though window is still open for 14.3.

It would be great if it could land in 14.3 .

@pleku
Copy link
Contributor

pleku commented Sep 2, 2020

Also to clarify here, this is now being shipped in 17.0.0 today, so please test it there. We would want to include this to 14-series as soon as possible, but we also don't want to ship anything in the LTS that is not "battle-proven".

If it works out well, dropping out a comment here will work and asking "when is this coming to 14" to let us know we need to get this in.
EDIT: and just to clarify, unless there are severe issues discovered, 14.5 would be the current target as 14.4 is up next and it has been feature-freezed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.