Skip to content
This repository has been archived by the owner on Nov 27, 2018. It is now read-only.

Fix behavior when optional parameters are defined within a route and not at the end #357

Closed
javiercn opened this issue Aug 25, 2016 · 7 comments
Assignees

Comments

@javiercn
Copy link
Member

#326

We create the incorrect tree. We should remove the optionality for route matching when we create the attribute route tree.

@javiercn
Copy link
Member Author

javiercn commented Aug 25, 2016

After doing some further investigation the E2E behavior is expected here. The optional parameter will be ignored during route matching. And for route generation it will only be considered as a default value. So for example, given the template

/a/b/{c=3}/d

It will only match routes like

/a/b/c/d
/a/b/e/d

and at link generation time, it will generate the route

/a/b/3/d

The reason for this to happen is that while TreeRouteBuilder creates the incorrect tree, TemplateRouteMatcher actually checks all the segments in the template and fails to match requests like /a/b when it gets to the d segment and determines that its not optional.

Given that the current behavior is what we expected the fix for this bug to be, we don't have to do anything here other than add the relevant tests to document the behavior here.

@javiercn
Copy link
Member Author

@rynowak @Eilon

@rynowak
Copy link
Member

rynowak commented Aug 26, 2016

I don't get what you're saying - are you saying that the behavior in 1.0.0 is correct or incorrect?

@Eilon
Copy link
Member

Eilon commented Aug 26, 2016

I think this bug just needs to be closed, no?

@javiercn
Copy link
Member Author

@rynowak still wants to fix the behavior on treeroutebuilder.

@Eilon Eilon added this to the 1.1.0 milestone Aug 28, 2016
@rynowak
Copy link
Member

rynowak commented Aug 29, 2016

We build the wrong data structure, and then rely on the implementation details of another part of the code to bail us out.

@Eilon
Copy link
Member

Eilon commented Aug 30, 2016

Well that sounds just dandy 😄 Indeed, let's clean that up... And we're sure we have enough test coverage for this now?

javiercn added a commit that referenced this issue Oct 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants