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

feat(rumqttc): allow empty client id #791

Merged
merged 16 commits into from
Feb 10, 2024
Merged

Conversation

arunanshub
Copy link
Contributor

@arunanshub arunanshub commented Feb 2, 2024

Fixes #667

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • Formatted with cargo fmt
  • Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.

@arunanshub arunanshub force-pushed the feat/allow-zero-empty-client-id branch 3 times, most recently from 7d8218b to bee99c5 Compare February 2, 2024 06:32
…ient_id`

We now provide an additional function, `try_set_clean_session` that
returns error if `client_id` is a 0 byte string and `clean_session` is
not `true`.
@arunanshub arunanshub marked this pull request as ready for review February 2, 2024 10:10
Copy link
Member

@swanandx swanandx left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have added some review comments, plz have a look once :)

Other than that:

  • please do same changes in v5's MqttOptions!
  • can we add test to verify we will only panic if client id is empty && clean session is false

Rest LGTM! 🚀 Nice work!

rumqttc/CHANGELOG.md Outdated Show resolved Hide resolved
rumqttc/src/lib.rs Outdated Show resolved Hide resolved
rumqttc/src/lib.rs Outdated Show resolved Hide resolved
rumqttc/src/lib.rs Outdated Show resolved Hide resolved
rumqttc/src/lib.rs Show resolved Hide resolved
rumqttc/src/v5/mod.rs Outdated Show resolved Hide resolved
rumqttc/CHANGELOG.md Outdated Show resolved Hide resolved
rumqttc/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@swanandx swanandx left a comment

Choose a reason for hiding this comment

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

can we please add some tests to verify set_clean_session is working correctly?

thanks :)

rumqttc/src/v5/mod.rs Show resolved Hide resolved
rumqttc/src/v5/mod.rs Outdated Show resolved Hide resolved
rumqttc/CHANGELOG.md Show resolved Hide resolved
@swanandx
Copy link
Member

swanandx commented Feb 8, 2024

Hey @arunanshub , any updates!?

@arunanshub
Copy link
Contributor Author

arunanshub commented Feb 8, 2024 via email

@swanandx
Copy link
Member

swanandx commented Feb 8, 2024

it has been difficult to find where they are validating the "clean_start", "client_id" and save session options

as discussed already, they ( or other clients ) don't validate it!

This PR is blocked by adding tests to check if are using assert correctly and other changes suggested in review.

Can you please prioritise em so we can close the PR? Then you can continue your research and open follow up PR ( if required ) 😁

wdyt? your call, otherwise if you prefer, lmk when we are good to go for this PR 👍

@swanandx swanandx merged commit 0266b85 into main Feb 10, 2024
3 checks passed
@swanandx swanandx deleted the feat/allow-zero-empty-client-id branch February 10, 2024 12:23
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.

Client ID should be optional
2 participants