#941 Added SseDelegatingHandler

[vijay-karavadra]

Closes #941

• #941

Proposed Changes

• Added SseDelegatingHandler to handle SSE event stream requests

Discussions

• server-sent events

[coveralls]

coverage: 87.032% (-0.2%) from 87.279% when pulling caed517cff75ecbad7027a2d024e07ff86bec4ef on vijay-karavadra:develop into 7c583bf1e4d8f3626bbdb32f6d864bfd2d86f3bb on ThreeMammals:develop.

[coveralls]

coverage: 87.672% (-0.3%) from 87.958% when pulling 8b98a65a38bbe838d4f2ea4e5bf924c7aaf333b2 on vijay-karavadra:develop into c721273e2090008fe281a6f85bca5b6d0b34c7d7 on ThreeMammals:develop.

[vijay-karavadra]

@raman-m Please review.

[raman-m]

@vijay-karavadra commented on July 14, 2025

@raman-m Please review.

No problem! The code review has been completed!

[vijay-karavadra]

To be fair, this is a draft version of the feature. This handler has not yet been integrated into Ocelot's pipeline. We need to ensure the feature is functional by writing acceptance tests that make real HTTP requests to an Ocelot instance. Could you continue working on this pull request? Rest assured, we will guide and support you throughout the development process.

Sure @raman-m . I would love to work on it. Could you share some of the sample acceptance tests for reference?

[raman-m]

@vijay-karavadra commented on July 14, 2025

Could you share some of the sample acceptance tests for reference?

Dear Vijay,
Our acceptance testing project can be found here → Ocelot.AcceptanceTests.
As I mentioned earlier, there's nothing to test at the moment since the DelegatingHandler hasn't been integrated into Ocelot's pipeline.
Also, why the DelegatingHandler? Perhaps other solutions are available, and have you considered alternative designs?

1. The starting point should be configuration, as we are proposing support for a new protocol. However, I don't see any updates in the configuration yet.
2. I now notice a design challenge with the new SSE protocol, which problem was partially discussed in the https://github.com/ThreeMammals/Ocelot/issues/941#issuecomment-548814882.
3. Finally, will Ocelot's SSE route rely on existing http and https schemes to support SSE traffic, or will a dedicated pipeline be introduced to handle routes with a new sse protocol/scheme?

We need to prioritize discussing this with the team and community before developing any delegating handlers.

I’m marking this PR as a draft since it’s too early to implement the feature, as the idea and design haven’t been thoroughly discussed yet.

[vijay-karavadra]

@vijay-karavadra commented on July 14, 2025

Could you share some of the sample acceptance tests for reference?

Dear Vijay, Our acceptance testing project can be found here → Ocelot.AcceptanceTests. As I mentioned earlier, there's nothing to test at the moment since the DelegatingHandler hasn't been integrated into Ocelot's pipeline. Also, why the DelegatingHandler? Perhaps other solutions are available, and have you considered alternative designs?

1. The starting point should be configuration, as we are proposing support for a new protocol. However, I don't see any updates in the configuration yet.
2. I now notice a design challenge with the new SSE protocol, which problem was partially discussed in the Server-sent events protocol #941 (comment).
3. Finally, will Ocelot's SSE route rely on existing http and https schemes to support SSE traffic, or will a dedicated pipeline be introduced to handle routes with a new sse protocol/scheme?

We need to prioritize discussing this with the team and community before developing any delegating handlers.

I’m marking this PR as a draft since it’s too early to implement the feature, as the idea and design haven’t been thoroughly discussed yet.

Ok @raman-m . FYI I tested in the below manner and it worked for me. It works for the https/http. We just need to persist the connection during the communication, so I didn't need to introduce a new DownstreamScheme/protocol. However I understand if you decide to go with a new scheme then will do it accordingly.

[raman-m]

Sorry, I will return back to this PR after releasing version 24.1 in late August or at begging of September. The current release has many risks of delivery. So, be patient and work more on the acceptance tests. I believe you could search for delegating handlers in IDE and add tests in a testing class. Later we will find correct class. You can add even a new testing class. After adding tests mark this PR as ready for review. Good luck!

[vijay-karavadra]

Sorry, I will return back to this PR after releasing version 24.1 in late August or at begging of September. The current release has many risks of delivery. So, be patient and work more on the acceptance tests. I believe you could search for delegating handlers in IDE and add tests in a testing class. Later we will find correct class. You can add even a new testing class. After adding tests mark this PR as ready for review. Good luck!

@raman-m OK, let me add acceptance tests.

[raman-m]

@vijay-karavadra commented on July 22

Alright, make sure to write additional unit tests to cover the new lines since the latest Coverall build failed. It seems the reported -0.3% represents uncovered lines of the SseDelegatingHandler, with a coverage of only 28% ❗ Aim for 100% file coverage to ensure the Coveralls build turns green 🟢

I don't recommend using the develop branch of your forked repo as a feature branch because it will make syncing with upstream development difficult due to multiple merge commits. And you won’t be able to work on other features simultaneously because of reused main/head develop branch.

[vijay-karavadra]

@vijay-karavadra commented on July 22

Alright, make sure to write additional unit tests to cover the new lines since the latest Coverall build failed. It seems the reported -0.3% represents uncovered lines of the SseDelegatingHandler, with a coverage of only 28% ❗ Aim for 100% file coverage to ensure the Coveralls build turns green 🟢

I don't recommend using the develop branch of your forked repo as a feature branch because it will make syncing with upstream development difficult due to multiple merge commits. And you won’t be able to work on other features simultaneously because of reused main/head develop branch.

@raman-m Currently I don't have the bandwidth TBH. I'm ok if anyone wants to take it from here.

[raman-m]

What was your intention when opening this pull request? Have you had a chance to review our Development Process document?

[vijay-karavadra]

What was your intention when opening this pull request? Have you had a chance to review our Development Process document?

I just had some code I used to enable SSE in my local so I wanted the repository to benefit. Let me know if I did miss anything there??

[raman-m]

You missed the Development Process Making a successful contribution involves thorough testing and code review. Without testing, there can be no meaningful contribution.