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.

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

Support wildcards streams in request-response stream client #381

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

sfod
Copy link
Contributor

@sfod sfod commented Jan 30, 2025

Issue #, if available:

Request-response stream needs support subscriptions to topics like $aws/some-service/<EXECUTION_ID>/etc, where <EXECUTION_ID> changes for each message published by a service.

Description of changes:

Implement a module allowing to match topic filter with wildcards and a topic where PUBLISH message was received.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sfod sfod marked this pull request as ready for review February 3, 2025 22:42
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 91.04478% with 12 lines in your changes missing coverage. Please review.

Project coverage is 84.27%. Comparing base (1bf1224) to head (9208605).

Files with missing lines Patch % Lines
...quest-response/request_response_subscription_set.c 90.51% 11 Missing ⚠️
source/request-response/request_response_client.c 94.44% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #381      +/-   ##
==========================================
+ Coverage   84.22%   84.27%   +0.04%     
==========================================
  Files          24       25       +1     
  Lines       10398    10452      +54     
==========================================
+ Hits         8758     8808      +50     
- Misses       1640     1644       +4     

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

@@ -0,0 +1,138 @@
#ifndef AWS_MQTT_PRIVATE_REQUEST_RESPONSE_REQUEST_RESPONSE_SUBSCRIPTION_SET_H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to see the "request_response" prefix removed somehow.

/*
* Call specified callbacks for all stream and request operations with subscriptions matching a provided publish event.
*/
AWS_MQTT_API void aws_mqtt_request_response_client_subscriptions_match(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would rename this to something ala 'handle_incoming_publish' or similar

}

if (!aws_byte_cursor_eq_c_str(&subscription_topic_filter_segment, "+") &&
!aws_byte_cursor_eq_ignore_case(&topic_segment, &subscription_topic_filter_segment)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think case insensitive is right here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants