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

[ChrisSwinchatt]

Expected Behavior

An informative exception which tells you you can't reuse catch-all names within a path.

Actual Behavior

This exception, which doesn't tell you what is wrong and looks like a bug in Ocelot:

[15:05:22 FTL] Application startup exception
System.AggregateException: One or more errors occurred. (Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: startIndex) ---> System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: startIndex
   at System.String.IndexOf(String value, Int32 startIndex, Int32 count, StringComparison comparisonType)
   at Ocelot.Configuration.Creator.UpstreamTemplatePatternCreator.Create(IReRoute reRoute)
   at Ocelot.Configuration.Creator.ReRoutesCreator.SetUpDownstreamReRoute(FileReRoute fileReRoute, FileGlobalConfiguration globalConfiguration)
   at Ocelot.Configuration.Creator.ReRoutesCreator.<>c__DisplayClass15_0.<Create>b__0(FileReRoute reRoute)
   at System.Linq.Enumerable.SelectListIterator`2.ToList()
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at Ocelot.Configuration.Creator.FileInternalConfigurationCreator.Create(FileConfiguration fileConfiguration)
   at Ocelot.Middleware.OcelotMiddlewareExtensions.CreateConfiguration(IApplicationBuilder builder)
   at Ocelot.Middleware.OcelotMiddlewareExtensions.UseOcelot(IApplicationBuilder builder, OcelotPipelineConfiguration pipelineConfiguration)
   at Ocelot.Middleware.OcelotMiddlewareExtensions.UseOcelot(IApplicationBuilder builder)

Steps to Reproduce the Problem

1. Create an ocelot.json which contains the following re-route:

        {
            "DownstreamPathTemplate": "/bar/{everything}/{everything}",
            "DownstreamScheme": "http",
            "DownstreamHostAndPorts": [
                {
                    "Host": "foo",
                    "Port": 80
                }
            ],
            "UpstreamPathTemplate": "/foo/bar/{everything}/{everything}",
            "UpstreamHttpMethod": [
                "Get"
            ],
            "Priority": 0
        }

1. Tell Ocelot to load it
2. Get the above exception

Specifications

• Version: 12.0.1
• Platform: ASP.Net Core 2.1
• Subsystem:

[philproctor]

Should probably be fixed in the initial reroute validators.

[raman-m]

@AlyHKafoury Your next coding challenge? 😉 It is related to CatchAll and Placeholders.

[AlyHKafoury]

@raman-m please assign to me

[raman-m]

@AlyHKafoury You're assigned!

[AlyHKafoury]

@raman-m I working on the unit tests

[raman-m]

@AlyHKafoury Because you're working on very similar from #748 & #1911 then you need to create feature branch from AlyHKafoury:Fix-748, otherwise you'll get duplicates. So, the right thing is branching from AlyHKafoury:Fix-748 ❕

[AlyHKafoury]

@raman-m I will rebase to develop now and continue on this

[raman-m]

@AlyHKafoury wrote

Yes, Rebase feature branch onto develop please and continue working... Thanks!

[raman-m]

@ChrisSwinchatt commented on Nov 12, 2018

Hello, Mr. Swinchatt! Thanks for reporting this funny bug! 😄 How did you test Ocelot in this user scenario, and realize to define this invalid duplicated placeholders? You have a talent for QA! 😉

Yeah, the previous dev team and Tom missed it, and in my opinion, the valid template definition should have all placeholders with different names, and each upstream placeholder should be presented in downstream template too. Otherwise, route validator should

• generate Critical error, and Ocelot will not start
• ignore this route, write Error event to the log and start Ocelot without this route in router mappings.

Make sense? Which design suits your usage scenarios best? Do you use Ocelot in your current projects?

@AlyHKafoury FYI And let's start discussing...

[ChrisSwinchatt]

Hello @raman-m,

I'm no longer working with Ocelot but IIRC I was writing code to generate an Ocelot config by traversing Swagger documents and it had a bug that resulted in outputting duplicate placeholders. So you could say that my bug found your bug. To your question, I would probably go the route of crashing early, but I leave it to you.

Thanks.

[raman-m]

@ChrisSwinchatt Thanks for your reply!

I was writing code to generate an Ocelot config by traversing Swagger documents and...

Since you are a big fan of Swagger, here is some useful information for you :point_right:

• PR #1429. As a Ocelot core team, we want to add Swagger support this year. The author developed just amazing library, which will be incorporated to Ocelot soon, this year, not planned yet... but we will try to start the integration as soon as possible, giving it high priority.
• Here is MMLib.SwaggerForOcelot project by @Burgyn. This is perfect tool which will be very useful for you. :yum:

Do not you mind, if we will create separate discussion where we can start brainstorming an integration and Swagger support, and I will invite you also to the thread?

---

To your question, I would probably go the route of crashing early, but I leave it to you.

:ok: Today I realized that ignoring the route will not be good. To crash early we can generate early validation Critical error, and moreover, if a request comes and matched the route then additionally write Error event to the log for each request. Crashing entire app won't be good, by exception throwing having it as unhandled one. So, we can generate exception, but we have to catch it in Error Mapper and write events into the log.

@AlyHKafoury FYI. Take into account this new design & requirements, please! :point_up:

[AlyHKafoury]

@raman-m Continuing on it

[AlyHKafoury]

@raman-m So, we are gonna throw a Critical error in validation, and then start the application normally then throw an error when a route matching this route is called ? and we will not crash the application ?

[raman-m]

@AlyHKafoury , I thought you're on vacation. :) Welcome back! :wink:

Regarding validation... You need a small research of validation logic. Something related to uniqueness of placeholders. If no validator, try to find another validators of uniqueness of route properties. And seems we need to do the same. Also, figure out, is it possible to write into the log by validation logic. I hope, it is possible outside of validation function. As far as I know, if validation function returns false then Ocelot crashes and the app won't start at all. What I'm trying to say, that there is restriction here by validation design. Possibly we have to crash app early, because validation function will return false. In this case we have a record in the log anyway by already implemented design.

The 2nd requirement to write extra record makes sense only if Ocelot won't crash. Probably, we can skip the 2nd requirement.

[raman-m]

@ChrisSwinchatt commented on Jan 20

Hi Chris!

FYI: Bug fixed today!

But we used not "an informative exception" solution and chose another solution based on Ocelot configuration validator adding more validation rules for route templates. With this bug fix Ocelot won't start because of validation errors in the log. And user must correct invalid routes with bad templates.

This patch will be released soon...