Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Fixes for strange routing behaviour with optionals. #2683

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PhilipSkinner
Copy link

@PhilipSkinner PhilipSkinner commented Jan 13, 2017

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the Nancy code style guidelines
  • I have provided test coverage for my change (where applicable)

Description

The routing engine incorrectly routes to longer urls in preference of the score, plus it falls back on routing to the last added matching routes instead of the first.

I've fixed the sorting comparison method and added some test coverage to show the particular use cases where I found issues.

The routing engine has been changed to order the matches by:

  • Score - greatest score wins.
  • Length - shortest match wins.
  • Indexed - first indexed wins.

Why? When I was testing some edge cases I found that the routing engine did "weird" things - it didn't do what I thought it would do. See the new tests to see what I mean.

@dnfclas
Copy link

dnfclas commented Jan 13, 2017

Hi @PhilipSkinner, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@dnfclas
Copy link

dnfclas commented Jan 13, 2017

@PhilipSkinner, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

@thecodejunkie
Copy link
Member

Can you rebase please? It will hopefully fix the Travis build error since we've pull in #2685

 The routing engine incorrectly routes to longer urls in preference
 of the score, plus it falls back on routing to the last added matching routes
 instead of the first.

 I've fixed the sorting comparison method and added some test coverage to
 show the particular use cases where I found issues.
@PhilipSkinner
Copy link
Author

Sorry for the delay, I've rebased and everything seems to be green now.

Looking forward to getting some feedback on the changes I'm proposing.

@thecodejunkie
Copy link
Member

plus it falls back on routing to the last added matching routes instead of the first.

You mean if we had

Route 1
Route 2

and both matched the request, it would pick Route 2 ?

This would be a breaking change, but the changes sound reasonable. Thoughts @NancyFx/owners @NancyFx/most-valued-minions ?

@PhilipSkinner for the sake of documenting the changes in this PR, can you give an example of the old and new behavior for the 3 (score, length, index) scenarios (update your description, thanks)

@khellang
Copy link
Member

TBH I'm struggling to see how this changed the routing based on the tests. Would it be possible to make the test module return the actual route string instead of a "friendly string"?

@thecodejunkie
Copy link
Member

@PhilipSkinner 👀

@thecodejunkie
Copy link
Member

@PhilipSkinner any updates on this? We'll soon be migrating away from project.json to the new csproj format and would like to close off a bunch of pull-requests before that happens. Thanks

@horsdal
Copy link
Member

horsdal commented Mar 8, 2017

Score taking precedence over length sounds reasonable.

@thecodejunkie
Copy link
Member

@PhilipSkinner one last ping mate. The migration to the new csproj format is happening soon and it would probably be easier for you to get this pull-requests in, before that, than having to migrate it yourself. 😄

@PhilipSkinner
Copy link
Author

Sorry, I didn't receive any alerts via email about these replies.

The system at the moment uses the last, rather than the first. I created a test to prove this before fixing it, the new test cases should ensure that it takes the first one rather than the last.

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

Successfully merging this pull request may close these issues.

6 participants