#683 Validate placeholder duplicates in path templates

[AlyHKafoury]

Fixes #683

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

[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 : https://github.com/ThreeMammals/Ocelot/blob/f4803c24bf9e9ca3929c78ca8eb23401e3c31c23/src/Ocelot/Configuration/Validator/FileConfigurationFluentValidator.cs#L36

and a test for it in here : https://github.com/ThreeMammals/Ocelot/blob/f4803c24bf9e9ca3929c78ca8eb23401e3c31c23/test/Ocelot.UnitTests/Configuration/Validation/FileConfigurationFluentValidatorTests.cs#L1476

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 :

https://github.com/ThreeMammals/Ocelot/blob/f4803c24bf9e9ca3929c78ca8eb23401e3c31c23/src/Ocelot/Configuration/Validator/FileConfigurationFluentValidator.cs#L36

and a test for it in here :

https://github.com/ThreeMammals/Ocelot/blob/f4803c24bf9e9ca3929c78ca8eb23401e3c31c23/test/Ocelot.UnitTests/Configuration/Validation/FileConfigurationFluentValidatorTests.cs#L1476

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]

@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

• [x] Fix integration tests. Done by https://github.com/ThreeMammals/Ocelot/pull/1927/commits/b4105c0145ed5d5a1b23f10a20d14b84d6bd9087
• [x] Release notes. Done by https://github.com/ThreeMammals/Ocelot/pull/1927/commits/11916b6672c6ee8330cab04e1545ed6dfeb5dcfe