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

async-nats: Support sample_freq values containing a % #1354

Merged
merged 5 commits into from
Dec 30, 2024

Conversation

bengsparks
Copy link
Contributor

@bengsparks bengsparks commented Dec 29, 2024

Closes #1353.

This PR updates async-nats's deserialization routine for sample_freq in consumers to account for a terminating '%', which is seen in the wild when managing NATS resources using Terraform / OpenTofu.

The changed code is partially tested by the same test added in #1300, but currently does not contain a test for the new case.
I instead tested it against the setup outlined in #1353.
I'd like to add such a case, but I'm not sure how to do so, as Rust's type system forbids me from creating a consumer with sample_freq set to something ending in %.

@Jarema
Copy link
Member

Jarema commented Dec 30, 2024

Hey!

Thanks for addressing the issue. The % was indeed the source of the problem.

The best way to test it send a custom request to JS API.

I wrote a quick and dirty example how to sent such request.
This should fail on main and work on your branch.

    #[tokio::test]
    async fn sample_frequency() {
        let client = async_nats::ConnectOptions::new()
            .connect("localhost:4222")
            .await
            .unwrap();

        let jetstream = async_nats::jetstream::new(client);

        jetstream
            .get_or_create_stream(stream::Config {
                name: "events".to_string(),
                subjects: vec!["events.>".to_string()],
                ..Default::default()
            })
            .await
            .unwrap();

        let consumer = json!({
            "stream_name": "events",
            "config": {
                "name": "consumer",
                "sample_freq": "10%",
            },
        });

        let info: Response<Info> = jetstream
            .request("CONSUMER.CREATE.events", &consumer)
            .await
            .unwrap();

        match info {
            Response::Ok(info) => {
                assert_eq!(info.config.sample_frequency, 10);
            }
            Response::Err { error } => panic!("expected ok response, got: {:?}", error),
        }
    }

Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

please add the mentioned test.

@bengsparks
Copy link
Contributor Author

bengsparks commented Dec 30, 2024

@Jarema
while adding the test, I've noticed an odd test here.
Both sections of the test use consumer::pull::Config, but the latter's name is "SampledPushConsumer".
This probably wasn't intentional? Should I add that change as part of this PR, or should I open a separate one for it?

In fact, I'd like to extend that test with your suggestion, as that test's purpose is to check the de/ser of sample_frequency

@Jarema
Copy link
Member

Jarema commented Dec 30, 2024

nice catch!

yes, it make sense to fix it, however, let's make it a separate PR.
I prefer not cluttering PR with many changes.

@bengsparks
Copy link
Contributor Author

Done in 5c5b36e.
I'll open the second PR after this one is merged

Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks for your contribution!

@Jarema Jarema merged commit db5ab8d into nats-io:main Dec 30, 2024
12 checks passed
@bengsparks bengsparks deleted the bug/consumer_on_stream branch January 2, 2025 12:38
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.

Context::get_consumer_from_stream fails to decode Pull Consumer created via OpenTofu Provider
2 participants