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.

Sign up for GitHub

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

Jump to bottom

Headers improvements #23

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Headers improvements #23

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4739e13
Headers improvements
Jan 24, 2022
aacdd8b
Headers improvements
Jan 25, 2022
16a0ee7
Poc Headers improvements:
Mar 30, 2022
79f4756
Poc Headers improvements
Apr 4, 2022
311a9d0
Poc Headers improvements
Apr 8, 2022
f5c76b0
Poc Headers improvements
Apr 18, 2022
3bf8be7
Format issues
Apr 18, 2022
72f7beb
PoC headers
Apr 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
187 changes: 125 additions & 62 deletions protocol/headers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,24 +13,43 @@ package protocol

import (
"encoding/json"
"fmt"
"strconv"
"time"
)

// Ditto-specific headers constants.
const (
HeaderCorrelationID = "correlation-id"
// ContentTypeDitto defines the Ditto JSON 'content-type' header value for Ditto Protocol messages.
ContentTypeDitto = "application/vnd.eclipse.ditto+json"

// HeaderCorrelationID represents 'correlation-id' header.
HeaderCorrelationID = "correlation-id"
// HeaderResponseRequired represents 'response-required' header.
HeaderResponseRequired = "response-required"
HeaderChannel = "ditto-channel"
HeaderDryRun = "ditto-dry-run"
HeaderOrigin = "origin"
HeaderOriginator = "ditto-originator"
HeaderETag = "ETag"
HeaderIfMatch = "If-Match"
HeaderIfNoneMatch = "If-None-Match"
HeaderReplyTarget = "ditto-reply-target"
HeaderReplyTo = "reply-to"
HeaderTimeout = "timeout"
HeaderSchemaVersion = "version"
HeaderContentType = "content-type"
// HeaderChannel represents 'ditto-channel' header.
HeaderChannel = "ditto-channel"
// HeaderDryRun represents 'ditto-dry-run' header.
HeaderDryRun = "ditto-dry-run"
// HeaderOrigin represents 'origin' header.
HeaderOrigin = "origin"
// HeaderOriginator represents 'ditto-originator' header.
HeaderOriginator = "ditto-originator"
// HeaderETag represents 'ETag' header.
HeaderETag = "ETag"
// HeaderIfMatch represents 'If-Match' header.
HeaderIfMatch = "If-Match"
// HeaderIfNoneMatch represents 'If-None-March' header.
HeaderIfNoneMatch = "If-None-Match"
// HeaderReplyTarget represents 'ditto-reply-target' header.
HeaderReplyTarget = "ditto-reply-target"
// HeaderReplyTo represents 'reply-to' header.
HeaderReplyTo = "reply-to"
// HeaderTimeout represents 'timeout' header.
HeaderTimeout = "timeout"
// HeaderSchemaVersion represents 'version' header.
HeaderSchemaVersion = "version"
// HeaderContentType represents 'content-type' header.
HeaderContentType = "content-type"
)

// Headers represents all Ditto-specific headers along with additional HTTP/etc. headers
Expand All @@ -42,114 +61,151 @@ type Headers struct {

// CorrelationID returns the 'correlation-id' header value or empty string if not set.
func (h *Headers) CorrelationID() string {
if h.Values[HeaderCorrelationID] == nil {
return ""
if value, ok := h.Values[HeaderCorrelationID]; ok && value != nil {
return value.(string)
}
return h.Values[HeaderCorrelationID].(string)
return ""
}

// Timeout returns the 'timeout' header value or empty string if not set.
func (h *Headers) Timeout() string {
if h.Values[HeaderTimeout] == nil {
return ""
// Timeout returns the 'timeout' header value or duration of 60 seconds if not set.
func (h *Headers) Timeout() time.Duration {
if value, ok := h.Values[HeaderTimeout]; ok {
if duration, err := parseTimeout(value.(string)); err == nil {
return duration
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we decide to change the API from string to time.Duration - the value in the Headers.Values map must be of type time.Duration as well - not a string one parsed every time. Otherwise, we introduce an inconsistency.
This of course, will requires some marshaling/unmarshaling customizations but it's important to keep the usage of the header the same regardless how the value was accessed - via the getter or via the Headers.Values map directly.

}
return 60 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the client supposed to handle default values of the headers? Or by spec, the cloud backend is supposed to interpret missing values to their defaults?
In general, the aim is always to send the smallest possible payload from the device - i.e. omit defaulting in this case.
Another topic is that if we add this default values handling here - it should be added everywhere for consistency. Otherwise - it's hard to use if it's randomly implemented.

Copy link

Choose a reason for hiding this comment

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

In this case the value is only returned/read and not set explicitly.

"In general, the aim is always to send the smallest possible payload from the device - i.e. omit defaulting in this case."

I'm totally agree with this statement, so it's good all getters/readers to return the default value no matter it's explicitly set or not (as it's done for the "response-required" value).
In order to make it consistent (and keep the payload compact), the setters/modifiers should remove the header if it is with its default value, i.e. no need to check a local variable before calling WithResponseRequired(required) - the Lib will do this for me.

For advanced users the Values map is exported so they can check any header existence or set it with its default value.

}

func parseTimeout(timeout string) (time.Duration, error) {
l := len(timeout)
if l > 0 {
t := time.Duration(-1)
switch timeout[l-1] {
case 'm':
if i, err := strconv.Atoi(timeout[:l-1]); err == nil {
t = time.Duration(i) * time.Minute
}
case 's':
if timeout[l-2] == 'm' {
if i, err := strconv.Atoi(timeout[:l-2]); err == nil {
t = time.Duration(i) * time.Millisecond
}
} else {
if i, err := strconv.Atoi(timeout[:l-1]); err == nil {
t = time.Duration(i) * time.Second
}
}
default:
if i, err := strconv.Atoi(timeout); err == nil {
t = time.Duration(i) * time.Second
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific reasoning behind implementing a custom duration string parsing instead of using the one from the Go SDK - i.e. time.ParseDuration?

if inTimeoutRange(t) {
return t, nil
}
}
return h.Values[HeaderTimeout].(string)
return -1, fmt.Errorf("invalid timeout '%s'", timeout)
}

func inTimeoutRange(timeout time.Duration) bool {
return timeout >= 0 && timeout < time.Hour
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are about to perform header values validation here - are we about to introduce them for all known headers limitations and expected values - e.g. requested-acks header value validation, etc.? Otherwise, it is not really consistent.


// IsResponseRequired returns the 'response-required' header value or empty string if not set.
// IsResponseRequired returns the 'response-required' header value or true if not set.
func (h *Headers) IsResponseRequired() bool {
if h.Values[HeaderResponseRequired] == nil {
return false
if value, ok := h.Values[HeaderResponseRequired]; ok && value != nil {
return value.(bool)
}
return h.Values[HeaderResponseRequired].(bool)
return true
}

// Channel returns the 'ditto-channel' header value or empty string if not set.
func (h *Headers) Channel() string {
if h.Values[HeaderChannel] == nil {
return ""
if value, ok := h.Values[HeaderChannel]; ok && value != nil {
return value.(string)
}
return h.Values[HeaderChannel].(string)
return ""
}

// IsDryRun returns the 'ditto-dry-run' header value or empty string if not set.
func (h *Headers) IsDryRun() bool {
if h.Values[HeaderDryRun] == nil {
return false
if value, ok := h.Values[HeaderDryRun]; ok && value != nil {
return value.(bool)
}
return h.Values[HeaderDryRun].(bool)
return false
}

// Origin returns the 'origin' header value or empty string if not set.
func (h *Headers) Origin() string {
if h.Values[HeaderOrigin] == nil {
return ""
if value, ok := h.Values[HeaderOrigin]; ok && value != nil {
return value.(string)
}
return h.Values[HeaderOrigin].(string)
return ""
}

// Originator returns the 'ditto-originator' header value or empty string if not set.
func (h *Headers) Originator() string {
if h.Values[HeaderOriginator] == nil {
return ""
if value, ok := h.Values[HeaderOriginator]; ok && value != nil {
return value.(string)
}
return h.Values[HeaderOriginator].(string)
return ""
}

// ETag returns the 'ETag' header value or empty string if not set.
func (h *Headers) ETag() string {
if h.Values[HeaderETag] == nil {
return ""
if value, ok := h.Values[HeaderETag]; ok && value != nil {
return value.(string)
}
return h.Values[HeaderETag].(string)
return ""
}

// IfMatch returns the 'If-Match' header value or empty string if not set.
func (h *Headers) IfMatch() string {
if h.Values[HeaderIfMatch] == nil {
return ""
if value, ok := h.Values[HeaderIfMatch]; ok && value != nil {
return value.(string)
}
return h.Values[HeaderIfMatch].(string)
return ""
}

// IfNoneMatch returns the 'If-None-Match' header value or empty string if not set.
func (h *Headers) IfNoneMatch() string {
if h.Values[HeaderIfNoneMatch] == nil {
return ""
if value, ok := h.Values[HeaderIfNoneMatch]; ok && value != nil {
return value.(string)
}
return h.Values[HeaderIfNoneMatch].(string)
return ""
}

// ReplyTarget returns the 'ditto-reply-target' header value or empty string if not set.
func (h *Headers) ReplyTarget() int64 {
if h.Values[HeaderReplyTarget] == nil {
return 0
if value, ok := h.Values[HeaderReplyTarget]; ok && value != nil {
return value.(int64)
}
return h.Values[HeaderReplyTarget].(int64)
return 0
}

// ReplyTo returns the 'reply-to' header value or empty string if not set.
func (h *Headers) ReplyTo() string {
if h.Values[HeaderReplyTo] == nil {
return ""
if value, ok := h.Values[HeaderReplyTo]; ok && value != nil {
return value.(string)
}
return h.Values[HeaderReplyTo].(string)
return ""
}

// Version returns the 'version' header value or empty string if not set.
func (h *Headers) Version() int64 {
if h.Values[HeaderSchemaVersion] == nil {
return 0
if value, ok := h.Values[HeaderSchemaVersion]; ok && value != nil {
return value.(int64)
}
return h.Values[HeaderSchemaVersion].(int64)
return 0
}

// ContentType returns the 'content-type' header value or empty string if not set.
func (h *Headers) ContentType() string {
if h.Values[HeaderContentType] == nil {
return ""
if value, ok := h.Values[HeaderContentType]; ok && value != nil {
return value.(string)
}
return h.Values[HeaderContentType].(string)
return ""
}

// Generic returns the value of the provided key header and if a header with such key is present.
Expand All @@ -164,10 +220,17 @@ func (h *Headers) MarshalJSON() ([]byte, error) {

// UnmarshalJSON unmarshels Headers.
func (h *Headers) UnmarshalJSON(data []byte) error {
var v map[string]interface{}
if err := json.Unmarshal(data, &v); err != nil {
var m map[string]interface{}
if err := json.Unmarshal(data, &m); err != nil {
return err
}
h.Values = v

if value, ok := m[HeaderTimeout]; ok {
if _, err := parseTimeout(value.(string)); err != nil {
return err
}
}

h.Values = m
return nil
}
41 changes: 39 additions & 2 deletions protocol/headers_opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@

package protocol

import (
"strconv"
"time"
)

// HeaderOpt represents a specific Headers option that can be applied to the Headers instance
// resulting in changing the value of a specific header of a set of headers.
type HeaderOpt func(headers *Headers) error
Expand Down Expand Up @@ -140,9 +145,41 @@ func WithIfNoneMatch(ifNoneMatch string) HeaderOpt {
}

// WithTimeout sets the 'timeout' header value.
func WithTimeout(timeout string) HeaderOpt {
//
// The provided value should be a non-negative duration in Millisecond, Second or Minute unit.
// The change results in timeout header string value containing the duration
// and the unit symbol (ms, s or m), e.g. '45s' or '250ms' or '1m'.
//
// The default value is '60s'.
// If a negative duration or duration of an hour or more is provided, the timeout header value
// is removed, i.e. the default one is used.
func WithTimeout(timeout time.Duration) HeaderOpt {
return func(headers *Headers) error {
headers.Values[HeaderTimeout] = timeout
if inTimeoutRange(timeout) {
var value string

if timeout > time.Second {
div := int64(timeout / time.Second)
rem := timeout % time.Second
if rem == 0 {
value = strconv.FormatInt(div, 10)
} else {
value = strconv.FormatInt(div+1, 10)
}
} else {
div := int64(timeout / time.Millisecond)
rem := timeout % time.Millisecond
if rem == 0 {
value = strconv.FormatInt(div, 10) + "ms"
} else {
value = strconv.FormatInt(div+1, 10) + "ms"
}
}

headers.Values[HeaderTimeout] = value
} else {
delete(headers.Values, HeaderTimeout)
}
return nil
}
}
Expand Down
45 changes: 40 additions & 5 deletions protocol/headers_opts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ package protocol
import (
"errors"
"testing"
"time"

"github.com/eclipse/ditto-clients-golang/internal"
)
Expand Down Expand Up @@ -270,12 +271,46 @@ func TestWithIfNoneMatch(t *testing.T) {
}

func TestWithTimeout(t *testing.T) {
t.Run("TestWithTimeout", func(t *testing.T) {
tmo := "10"
tests := map[string]struct {
arg time.Duration
want time.Duration
}{
"test_with_seconds": {
arg: 10 * time.Second,
want: 10 * time.Second,
},
"test_with_milliseconds": {
arg: 500 * time.Millisecond,
want: 500 * time.Millisecond,
},
"test_with_minute": {
arg: 1 * time.Minute,
want: time.Minute,
},
"test_with_zero": {
arg: 0,
want: 0 * time.Second,
},
"test_without_unit": {
arg: 5,
want: 1 * time.Millisecond,
},
"test_with_invalid_timeout": {
arg: -1,
want: 60 * time.Second,
},
"test_with_1_hour_timeout": {
arg: time.Hour,
want: 60 * time.Second,
},
}

got := NewHeaders(WithTimeout(tmo))
internal.AssertEqual(t, tmo, got.Timeout())
})
for testName, testCase := range tests {
t.Run(testName, func(t *testing.T) {
got := NewHeaders(WithTimeout(testCase.arg))
internal.AssertEqual(t, testCase.want, got.Timeout())
})
}
}

func TestWithSchemaVersion(t *testing.T) {
Expand Down
Loading