#683 Validate placeholder duplicates in path templates

[AlyHKafoury]

Fixes #683

• Ocelot crashes if there are same-named Catch-Alls e.g. foo/{bar}/{bar} #683

[AlyHKafoury]

@raman-m Draft for a better exception handling in upstreamcreator

[AlyHKafoury]

@raman-m if we agree on the first solution which is failing gracefully, then this PR should do exactly that

[raman-m]

@AlyHKafoury
What was/is the line of code which generates original ArgumentOutOfRangeException error?
Have you reproduced the bug?

I'm not sure that simple exception throwing is right... But, yes, technically we provide information about invalid route.
Also, @philproctor recommended on Jan 10, 2019 to update validators. So, having validation error + exception throwing by UpstreamTemplatePatternCreator will be one solid solution.

Let's discuss more with the team...
@RaynaldM @ggnaegi We need your attention here. My design approach is here, please read it.

[AlyHKafoury]

@raman-m I reproduced with the included unit test. the line that fails is this line

var indexOfNextForwardSlash = upstreamTemplate.IndexOf("/", indexOfPlaceholder, StringComparison.Ordinal);

the reason is indexOfPlaceholder is -1 because instead of matching the first place holder in the line it matches the last 1 causing a whole set of unexpected behaviors to happen. Even if we like fix this part and allow duplicate placer-holders this will cause alot of issues down the line

[raman-m]

Ok, got it! You've reproduced the bug by unit test. Please, postpone work on this. Or review related acceptance tests, if there is no something similar then write a new acceptance test to repro the bug too.

Need to discuss final design with the team.
Therefore, this PR remains a draft.

[AlyHKafoury]

@raman-m Other issues to work on ?

[raman-m]

AlyHKafoury:Fix-683
😃 Instead of free naming convention for feature branch, better to use original issue label which is
So, feature name could be bug-683 or bug/683.
Finally, it doesn't matter because branch name can have any name 🤣 even 683 and it is correct because I can find the issue in backlog by the number.

Conclusion: So presenting of issue number in branch name is must-have.

[raman-m]

Hi Aly!
I've marked the PR as ready for reviews, so we can continue development, because the author replied to us.
So, please read new requirements of linked issue #683 and let's get started active development once again.

[AlyHKafoury]

@raman-m since we are going to catch this early during validation should I remove exception handling at the upstreamceator stage ?

---

Update by Maintainer on Jan 26

Yes, please remove the exception. See my answer above ☝️

[AlyHKafoury]

@raman-m this is strange there is already a validator in here :

Ocelot/src/Ocelot/Configuration/Validator/FileConfigurationFluentValidator.cs

Line 36 in f4803c2

.Must((_, route) => IsPlaceholderNotDuplicatedIn(route.UpstreamPathTemplate))

and a test for it in here :

Ocelot/test/Ocelot.UnitTests/Configuration/Validation/FileConfigurationFluentValidatorTests.cs

Line 1476 in f4803c2

public void configuration_is_invalid_when_placeholder_is_used_twice_in_upstream_path_template()

The point is the current rule now checks for upstream only should I add also downstream check for this and a unit test for it or are we good with the only checking the upstream

[AlyHKafoury]

@raman-m this is strange there is already a validator in here :

Ocelot/src/Ocelot/Configuration/Validator/FileConfigurationFluentValidator.cs

Line 36 in f4803c2

.Must((_, route) => IsPlaceholderNotDuplicatedIn(route.UpstreamPathTemplate))

and a test for it in here :

Ocelot/test/Ocelot.UnitTests/Configuration/Validation/FileConfigurationFluentValidatorTests.cs

Line 1476 in f4803c2

public void configuration_is_invalid_when_placeholder_is_used_twice_in_upstream_path_template()

The point is the current rule now checks for upstream only should I add also downstream check for this and a unit test for it or are we good with the only checking the upstream

Yes the current problem is with downstream path only being duplicate, so I will add rule for that and add unit test for it

[raman-m]

Please, add user scenario of bug #683 into a test.

Also, I'd ask you to check the following...
Are there some tests which check that each upstream placeholder is presented in downstream with the same name?
If Yes, it is fine.
If No, we have to add this validation rule too.

[raman-m]

Please, work more and independently 😉
When you request code review, the build should be passed. Seems your code is not complete.

[raman-m]

@AlyHKafoury Please resolve the merge conflict!

[AlyHKafoury]

On it

[raman-m]

And merge develop to feature branch plz!
But it is better to rebase onto develop

[raman-m]

@AlyHKafoury Hi Aly!
How do you do?
What is the rest of the work here?

[raman-m]

@AlyHKafoury The feature branch has been rebased today ❗
Please delete local branch, fetch all, and checkout once again!

TODO: Tomorrow I will continue fixing of failed Acceptance tests...
A common failure is Unable to start Ocelot, errors are: xxx due to missing placeholder in a path template.

[raman-m]

Aly, where are you? Seems you live in Egypt... So, your time zone is +2 with 1-2 hours shift.
Ok, if you are not interested in this contribution then just ignore my comments.
I will deliver this bug fix by my own.

[raman-m]

TODO

• Fix integration tests. Done by b4105c0
• Release notes. Done by 11916b6

[raman-m]

Ready for delivery ✅