Downstream route is not allowed to end on a forward slash

LueNC commented 4 months ago

Expected Behavior / New Feature

This is our route:

{
      "UpstreamHeaderTransform": {
        "Accept": "application/json"
      },
      "AuthenticationOptions": {
        "AuthenticationProviderKey": "Cookies",
        "AllowedScopes": []
      },
      "DelegatingHandlers": [
        "WebMailboxDelegatingHandler"
      ],
      "DownstreamPathTemplate": "/apis/v1/mailboxes/",
      "UpstreamPathTemplate": "/api/mailboxes",
      "DownstreamHeaderTransform": {
        "content-type": "application/json"
      },
      "DownstreamScheme": "https",
      "DownstreamHostAndPorts": [
        {
          "Host": "Censored",
          "Port": 443
        }
      ],
      "UpstreamHttpMethod": [
        "GET"
      ],
      "QoSOptions": {
        "ExceptionsAllowedBeforeBreaking": 10,
        "DurationOfBreak": 1000,
        "TimeoutValue":15000
      }
    }

I expect the UpstreamPath to be mapped to the DownstreamPath as per the template.

Actual Behavior / Motivation for New Feature

The last forward slash is removed, thus resulting in a 404 for our end as the last forward slash is expected. This used to work but for some reason this code was added to the DownstreamURLCreatorMiddleware

var dsPath = response.Data.Value; 
if (dsPath.EndsWith(Slash) && !upstreamPath.EndsWith(Slash)) 
{
    dsPath = dsPath.TrimEnd(Slash);
    response = new OkResponse<DownstreamPath>(new DownstreamPath(dsPath));
}

in this commit: #1911

Steps to Reproduce the Problem

1. 1. 1.

Specifications

Version: 23.2.2
Platform: Windows server 2012
Subsystem:

raman-m commented 4 months ago

Fix 1

      "DownstreamPathTemplate": "/apis/v1/mailboxes/",
      "UpstreamPathTemplate": "/api/mailboxes/",

Fix 2

      "DownstreamPathTemplate": "/apis/v1/mailboxes/?fake=bla-bla",
      "UpstreamPathTemplate": "/api/mailboxes/",

LueNC commented 4 months ago

@raman-m We should not need a workaround for something that has worked since Ocelot 15. Is there any reason that it is desirable to not allow an ending backslash?

raman-m commented 4 months ago

Docs

We should not need a workaround for something that has worked since Ocelot 15.

The Ocelot project evolves, my dear! If you need old behavior, don't upgrade the version! Stay on version 15.x!!!

Is there any reason that it is desirable to not allow an ending backslash?

Once again: Empty Placeholders It allows slashes if you define the following Catch All route:

      "DownstreamPathTemplate": "/apis/v1/mailboxes/{catchAll}",
      "UpstreamPathTemplate": "/api/mailboxes/{catchAll}",

Incoming URL path must be: /api/mailboxes/, note with slash! Then the downstrean URL path will be: /apis/v1/mailboxes/, note with slash!

Hope it helps!

raman-m commented 2 months ago

@sybren9 What's up?

sybren9 commented 2 months ago

@raman-m absolutely deplorable to introduce such a breaking change and offering no backwards compatibility.

And why is this not communicated at all on the release notes?

raman-m commented 2 months ago

Dear @sybren9,

Please be aware that Ocelot is open-source software, available at no cost, and as such, your judgment as a customer does not apply❗

such a breaking change

I believe that making requests with an invalid URL does not constitute a breaking change, as any HTTP tool can encounter an error. Additionally, I want to convey to Ocelot's enthusiasts that using invalid URLs to judge the validity of a configuration is not the correct approach.

The version 23 Empty Placeholders feature has been implemented and the corresponding documentation is now available.

offering no backwards compatibility.

The author highlighted a unique case where the absence of a trailing slash caused one of the many downstream services to fail, though the reason was not provided. It's important to understand that the Ocelot team cannot be held accountable for the instability of downstream services utilized by the community. Predicting how a new version will affect the behavior of downstream services is challenging and often not feasible. Community feedback is vital in monitoring backward compatibility. Rather than guaranteeing strict backward compatibility, we suggest a gradual migration of the solution through a review of the configuration, simply by updating the ocelot.json file.

And why is this not communicated at all on the release notes?

Have you read our 23.0.0 Release Notes at all? See Focus On → Middlewares → DownstreamUrlCreatorMiddleware with the note:

DownstreamUrlCreatorMiddleware: Fixed bug for ending/omitting slash in path templates aka Empty placeholders feature by @AlyHKafoury

This scenario presents a classic dilemma: on one hand, the community desires bug fixes and new behaviors, while on the other, the new feature impacts routing. As the product owner, I have the authority to determine the product's evolution. I believe that the new functionalities in #748 and #1911 will benefit the community due to the high level of interest, despite the potential effects on backward compatibility.

P.S.

Every major version update typically requires testing and possibly stabilization based on community feedback, which is a standard part of the natural Software Development Life Cycle (SDLC). Beyond the standard stabilization phase, we advise smooth migrations following valuable and reasonable discussions.

raman-m commented 2 months ago

@sybren9 While reviewing your contribution to our project, it turned out that your contribution was missing. 👉

No commits means no contributions
No issues means no help to improve the project
No pull requests means no possible contributions
No discussions means your low interest in our product

But you judge❗🤯

In my view, debating the backward compatibility of an OSS product is only justifiable if the user has made contributions; unfortunately, you have not.

ThreeMammals / Ocelot