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

fix: convert session_present flag to boolean to follow the declared type #242

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SergeTupchiy
Copy link

No description provided.

@SergeTupchiy
Copy link
Author

NOTE: it will break a few tests in emqx.

@@ -2255,3 +2255,6 @@ redact_packet(#mqtt_packet{variable = #mqtt_packet_connect{} = Conn} = Packet) -
Packet#mqtt_packet{variable = Conn#mqtt_packet_connect{password = <<"******">>}};
redact_packet(Packet) ->
Packet.

flag_to_bool(0) -> false;
Copy link
Author

Choose a reason for hiding this comment

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

Alternatively, can be fixed in https://github.com/emqx/emqtt/blob/fix-session_present-type/src/emqtt_frame.erl#L218
similarly to how CONNECT packet flags are parsed to bools: https://github.com/emqx/emqtt/blob/fix-session_present-type/src/emqtt_frame.erl#L202

But it would require changing the record field, e.g:

#mqtt_packet_connack{session_present  = SessionPresent,

Instead of:

#mqtt_packet_connack{ack_flags   = AckFlags,

@qzhuyan
Copy link
Contributor

qzhuyan commented May 16, 2024

I think it is a good change but may break many places, worth it?
Do you have a good reason?

@SergeTupchiy
Copy link
Author

I think it is a good change but may break many places, worth it? Do you have a good reason?

No reason, besides improving consistency (other flags are converted and stored as boolean).
In emqx session_present flag is only used in a few test cases which would be easy to fix.

Instead of this fix, we may change type of session_present to integer: https://github.com/emqx/emqtt/blob/master/src/emqtt.erl#L252 ?

@thalesmg
Copy link
Contributor

I think it is a good change but may break many places, worth it? Do you have a good reason?

No reason, besides improving consistency (other flags are converted and stored as boolean). In emqx session_present flag is only used in a few test cases which would be easy to fix.

Instead of this fix, we may change type of session_present to integer: https://github.com/emqx/emqtt/blob/master/src/emqtt.erl#L252 ?

IMO, using a boolean is more semantic for this case. Maybe we could indicate this is a breaking change with a major version bump?

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.

3 participants