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

Correct handling of special MQTT headers in MQTT 5 #1758

Closed
dimabarbul opened this issue Sep 26, 2023 Discussed in #1757 · 5 comments · Fixed by #1764
Closed

Correct handling of special MQTT headers in MQTT 5 #1758

dimabarbul opened this issue Sep 26, 2023 Discussed in #1757 · 5 comments · Fixed by #1764
Assignees
Milestone

Comments

@dimabarbul
Copy link
Contributor

MQTT 5 has some special headers: mqtt.topic, mqtt.qos, mqtt.retain.

  1. When I send message and set, for example, mqtt.retain header (using headerMapping or payload mapper), it will be applied (i.e., the message will be retained) and added to MQTT user properties. These special headers should not be added to user properties.
  2. When I receive message using MQTT 5, the headers are set accordingly to message features, for example, mqtt.topic will contain topic name that the message was received on. But if the message has user property mqtt.topic, it will override special meaning of the header and contain whatever user property contains. User properties which named exactly as special MQTT headers should be ignored.
@dimabarbul
Copy link
Contributor Author

dimabarbul commented Sep 26, 2023

I believe, the same should apply to correlation-id, content-type and reply-to headers in consumed messages.

@dimabarbul
Copy link
Contributor Author

dimabarbul commented Sep 27, 2023

I've found comment about known MQTT headers in outgoing message stating that they weren't removed because it would break functionality for users who already depend on it:

    /*
     * Actually it would be correct to also include MqttHeader.MQTT_TOPIC,
     * MqttHeader.MQTT_QOS and MqttHeader.MQTT_RETAIN in the set of the known
     * MQTT header names because they are already dedicated properties in the
     * MQTT Publish.
     * However, as the named headers were included in user properties
     * up to the present, there might be users who rely on their presence.
     * Excluding the headers from user properties would then break
     * functionality.
     */

@thjaeckle, should feature toggle be introduced, e.g., ditto.connectivity.feature.mqtt-known-headers-in-user-properties-enabled?

@thjaeckle
Copy link
Member

@thjaeckle, should feature toggle be introduced, e.g., ditto.connectivity.feature.mqtt-known-headers-in-user-properties-enabled?

Oh, yes .. indeed - I vaguely remember discussing that some time ago.
Controlling it with a feature toggle would make sense, yes.

We have already some feature toggles defined here - so I would prefer to add the feature toggle in the same way.

// feature toggles
feature {
// enables/disables the merge things feature
merge-things-enabled = true
merge-things-enabled = ${?DITTO_DEVOPS_FEATURE_MERGE_THINGS_ENABLED}
// enables/disables the WoT (Web of Things) integration feature
wot-integration-enabled = true
wot-integration-enabled = ${?DITTO_DEVOPS_FEATURE_WOT_INTEGRATION_ENABLED}
// enables/disables the historical API access feature
historical-apis-enabled = true
historical-apis-enabled = ${?DITTO_DEVOPS_FEATURE_HISTORICAL_APIS_ENABLED}
}

Here is how feature toggles are evaluated currently:
https://github.com/eclipse-ditto/ditto/blob/master/base/model/src/main/java/org/eclipse/ditto/base/model/signals/FeatureToggle.java

@thjaeckle
Copy link
Member

@dimabarbul would you like to provide this for Ditto 3.4.0 (which we want to release in the next 1-2 weeks)?

@dimabarbul
Copy link
Contributor Author

Yes, I would love to.

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

Successfully merging a pull request may close this issue.

2 participants