Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix subscriptions via REST when using params #331

Merged
merged 2 commits into from
Dec 15, 2023
Merged

Fix subscriptions via REST when using params #331

merged 2 commits into from
Dec 15, 2023

Conversation

frankosterfeld
Copy link
Contributor

@frankosterfeld frankosterfeld commented Nov 24, 2023 •
edited
Loading

Unify subscription topic handling and fix it for REST (#328, #331)

Do not lose params when subscribing to a topic via REST.

Bump cpp-httplib to newest version, as it fixes an encoding error with query parameters encoded in query parameters (SubscriptionContext) when used with redirects.

To unify subscription handling (different client/servers were making slightly different assumptions), the following is now implemented:

  • Service names must always start with "/".
  • The topic is of the form "/some/service?param1=..&param2", where the service name is mandatory in the topic, and the params are optional.
  • When serialized to a ZMQ topic, i.e. the string-based PUB/SUB topic matching, the parameters are sorted, to make the subscription and notification topics match exactly (required by ZMQ, as ZMQ subscription topics are string-, not URI-based)
  • Valid REST URIs are e.g. "http://localhost:8080/service?a&b" or http://localhost:8080/service?a&b", i.e. the service is a mandatory part of the URI. mds:// and mdp:// URIs are analogous.
  • On the ZMQ message level, the "serviceName" in above examples would be "/service" and the "topic" would be "/service?a&b" (here param order is not enforced).

UPDATE: Rebased/merged #330 into this one:

Make sure that long-poll request handlers do not block forever when no corresponding event is received. Otherwise the clients will send one request after another once their request times out client-side, until the worker threads are exhausted and the server stops responding.

It would be great if we could also detect the client connection be dropped using Keep-Alive, but cpp-httplib doesn't seem to have API for that.

@codecov Codecov
Copy link

codecov bot commented Nov 24, 2023 •
edited
Loading

Codecov Report

Attention: 171 lines in your changes are missing coverage. Please review.

Comparison is base (99ae7d2) 55.84% compared to head (a03822f) 57.15%.

Files Patch % Lines
src/majordomo/include/majordomo/RestBackend.hpp 36.58% 18 Missing and 34 partials ⚠️
src/majordomo/include/majordomo/Broker.hpp 35.29% 3 Missing and 30 partials ⚠️
src/client/include/RestClientNative.hpp 15.15% 14 Missing and 14 partials ⚠️
src/core/include/Topic.hpp 70.00% 1 Missing and 20 partials ⚠️
src/client/include/Client.hpp 14.28% 2 Missing and 16 partials ⚠️
src/majordomo/include/majordomo/Worker.hpp 39.13% 3 Missing and 11 partials ⚠️
src/client/include/MockServer.hpp 40.00% 0 Missing and 3 partials ⚠️
src/zmq/include/zmq/ZmqUtils.hpp 0.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #331      +/-   ##
==========================================
+ Coverage   55.84%   57.15%   +1.30%     
==========================================
  Files          69       69              
  Lines        7291     7310      +19     
  Branches     2691     2703      +12     
==========================================
+ Hits         4072     4178     +106     
+ Misses       1484     1322     -162     
- Partials     1735     1810      +75

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@frankosterfeld frankosterfeld marked this pull request as ready for review November 29, 2023 15:49
@frankosterfeld frankosterfeld linked an issue Dec 15, 2023 that may be closed by this pull request
[1pt, 1.5pt] Rest: Subscription query parameters are not preserved in HTTP to ZMQ translation #328
Closed
frankosterfeld added a commit that referenced this pull request Dec 15, 2023
@frankosterfeld
Unify subscription topic handling and fix it for REST (#328, #331)
5f9a53e 
Do not lose params when subscribing to a topic via REST.

Bump cpp-httplib to newest version, as it fixes an encoding error
with query parameters encoded in query parameters (SubscriptionContext)
when used with redirects.

To unify subscription handling (different client/servers were making
slightly different assumptions), the following is now implemented:

 - Service names must always start with "/".
 - The topic is of the form "/some/service?param1=..&param2", where the
   service name is mandatory in the topic, and the params are optional.
 - When serialized to a ZMQ topic, i.e. the string-based PUB/SUB topic
   matching, the parameters are sorted, to make the subscription and
   notification topics match exactly (required by ZMQ, as ZMQ subscription
   topics are string-, not URI-based)
 - Valid REST URIs are e.g. "http://localhost:8080/service?a&b"
   or http://localhost:8080/service?a&b", i.e. the service is a
   mandatory part of the URI. mds:// and mdp:// URIs are analogous.
 - On the ZMQ message level, the "serviceName" in above examples would
   be "service" and the "topic" would be "/service?a&b" (here param
   order is not enforced).
@frankosterfeld
Unify subscription topic handling and fix it for REST (#328, #331)
19a2fcb 
Do not lose params when subscribing to a topic via REST.

Bump cpp-httplib to newest version, as it fixes an encoding error
with query parameters encoded in query parameters (SubscriptionContext)
when used with redirects.

To unify subscription handling (different client/servers were making
slightly different assumptions), the following is now implemented:

 - Service names must always start with "/".
 - The topic is of the form "/some/service?param1=..&param2", where the
   service name is mandatory in the topic, and the params are optional.
 - When serialized to a ZMQ topic, i.e. the string-based PUB/SUB topic
   matching, the parameters are sorted, to make the subscription and
   notification topics match exactly (required by ZMQ, as ZMQ subscription
   topics are string-, not URI-based)
 - Valid REST URIs are e.g. "http://localhost:8080/service?a&b"
   or http://localhost:8080/service?a&b", i.e. the service is a
   mandatory part of the URI. mds:// and mdp:// URIs are analogous.
 - On the ZMQ message level, the "serviceName" in above examples would
   be "/service" and the "topic" would be "/service?a&b" (here param
   order is not enforced).
@frankosterfeld
Do not block worker threads forever if no event is received (#329)
a03822f 
Make sure that long-poll request handlers do not block forever when no
corresponding event is received. Otherwise the clients will send one
request after another once their request times out client-side, until
the worker threads are exhausted and the server stops responding.

It would be great if we could also detect the client connection be
dropped using Keep-Alive, but cpp-httplib doesn't seem to have API for
that.
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Dec 15, 2023

Quality Gate Failed Quality Gate failed

Failed conditions

35.3% Coverage on New Code (required ≥ 80%)
17.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@wirew0rm wirew0rm merged commit 57f31a1 into main Dec 15, 2023
9 of 11 checks passed
@wirew0rm wirew0rm deleted the frank/rest-subscription-params branch December 15, 2023 12:17
wirew0rm pushed a commit that referenced this pull request Dec 15, 2023
@frankosterfeld @wirew0rm
Unify subscription topic handling and fix it for REST (#328, #331)
9704b65 
Do not lose params when subscribing to a topic via REST.

Bump cpp-httplib to newest version, as it fixes an encoding error
with query parameters encoded in query parameters (SubscriptionContext)
when used with redirects.

To unify subscription handling (different client/servers were making
slightly different assumptions), the following is now implemented:

 - Service names must always start with "/".
 - The topic is of the form "/some/service?param1=..&param2", where the
   service name is mandatory in the topic, and the params are optional.
 - When serialized to a ZMQ topic, i.e. the string-based PUB/SUB topic
   matching, the parameters are sorted, to make the subscription and
   notification topics match exactly (required by ZMQ, as ZMQ subscription
   topics are string-, not URI-based)
 - Valid REST URIs are e.g. "http://localhost:8080/service?a&b"
   or http://localhost:8080/service?a&b", i.e. the service is a
   mandatory part of the URI. mds:// and mdp:// URIs are analogous.
 - On the ZMQ message level, the "serviceName" in above examples would
   be "/service" and the "topic" would be "/service?a&b" (here param
   order is not enforced).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivan-cukic ivan-cukic ivan-cukic approved these changes

@wirew0rm wirew0rm wirew0rm approved these changes

@RalphSteinhagen RalphSteinhagen RalphSteinhagen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@frankosterfeld @ivan-cukic @wirew0rm @RalphSteinhagen