Headers improvements #23

antoniyatrifonova · 2022-01-24T14:51:13Z

timeout is changed to time.Duration
usage of 'comma ok' idiom
add constant for ditto specific content type

Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

- timeout is changed to time.Duration - usage of 'comma ok' idiom - add constant for ditto specific content type Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

antoniyatrifonova · 2022-01-24T14:52:58Z

@konstantina-gramatova please review this PR

- Update the doc according the refactoring - Convert timeout in time.Duration after unmarshaling JSON - Add more tests scenarios Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

konstantina-gramatova · 2022-01-26T11:37:15Z

protocol/headers.go

+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
+			}
+		}


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?

konstantina-gramatova · 2022-01-26T11:40:57Z

protocol/headers.go

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


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.

konstantina-gramatova · 2022-01-26T11:44:26Z

protocol/headers.go

+	}
+	return 60 * time.Second


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.

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.

konstantina-gramatova · 2022-01-26T11:48:00Z

protocol/headers.go

+func (h *Headers) Timeout() time.Duration {
+	if value, ok := h.Values[HeaderTimeout]; ok {
+		if duration, err := parseTimeout(value.(string)); err == nil {
+			return duration
+		}


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.

- change header type to map - provide the functionality for case-insensitive - change timeout getter and setter method to expect time.Duration type - improve the getter methods to return default value (if this value is presented) - improve the test scenarios Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

dimitar-dimitrow · 2022-04-06T07:38:08Z

protocol/headers.go

@@ -90,10 +94,10 @@ func (h Headers) CorrelationID() string {
 			return v.(string)
 		}
 	}
-	return ""
+	return uuid.New().String()


If initially missing, on every invocation of CorrelationID() a newly generated id would be returned.
Also if there are more than one headers differing only in capitalization, should we make sure that the one that is initially returned(the first met one) would be returned on every next invocation as well?

dimitar-dimitrow · 2022-04-06T07:38:40Z

protocol/headers.go

 // See https://www.eclipse.org/ditto/protocol-specification.html
 type Headers map[string]interface{}

 // CorrelationID returns the 'correlation-id' header value if it is presented.
-// If the header value is not presented, the CorrelationID returns empty string.
+// If the header value is not presented, the 'correlation-id' header value will be generated in UUID format.
 //
 // If there are two headers differing only in capitalization CorrelationID returns the first value.


If there are more than one headers differing only in capitalization CorrelationID returns the first met value.

dimitar-dimitrow · 2022-04-06T07:42:14Z

protocol/headers.go

+	if err := json.Unmarshal(data, &temp); err != nil {
+		return err
+	}
+	for k := range temp {


Is this cleaning needed? In the generic case the initial headers are empty.

dimitar-dimitrow · 2022-04-06T07:47:53Z

protocol/headers_opts.go

 func WithGeneric(headerID string, value interface{}) HeaderOpt {
 	return func(headers Headers) error {
+		for k := range headers {
+			if strings.EqualFold(k, headerID) {
+				delete(headers, k)


The user might want to add several different headers with keys only differing in the capitalization. In this case the user should directly use the exported map?

Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

dimitar-dimitrow · 2022-04-19T12:57:19Z

protocol/headers.go

+//
+// If there are more than one headers differing only in capitalization, the CorrelationID returns the first met value.
+// To use the provided key to get the value, access the map directly.
+func (h Headers) CorrelationID() (string, bool) {


Return only the correlation id, skip boolean flag for the unexpected type. Keep it consistent with other methods.

dimitar-dimitrow · 2022-04-19T13:05:26Z

protocol/headers.go

+func sortHeadersKey(h Headers) []string {
+	var keys []string
+	for k := range h {
+		keys = append(keys, k)


It would be more efficient to allocate the []string in advance as the size is known, than using append.

dimitar-dimitrow · 2022-04-19T13:07:10Z

protocol/headers.go

+}
+
+// With sets new Headers to the existing.
+func (h Headers) With(opts ...HeaderOpt) Headers {


dimitar-dimitrow · 2022-04-19T13:18:29Z

protocol/headers.go

+// ContentType returns the 'content-type' header value if it is presented.
+//
+// If the header value is not presented, the ContentType returns the empty string.
+// If the header value is not presented, the ContentType returns the empty string.


The line duplicated, copy paste error.

dimitar-dimitrow · 2022-04-19T13:24:15Z

protocol/headers.go

+// If the header value is presented, but the type is not a string or the value is not valid, the Timeout returns the default value.
+//
+// If there are more than one headers differing only in capitalization, the Timeout returns the first met value.
+// To use the provided key to get the value, access the map directly.


The API doc might be more optimized and punctual.

dimitar-dimitrow · 2022-04-19T13:24:41Z

protocol/headers_opts.go

+// If there aren't any headers for 'correlation-id', WithCorrelationID sets a new header with
+// key 'correlation-id' and the provided value.
+//
+// To use the provided key to set a new value, access the map directly.


The API doc might be more optimized and punctual.

Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

dimitar-dimitrow

LGTM

thjaeckle · 2022-04-20T07:49:05Z

Can/should this be merged? With most of the commit messages containing (only) a "PoC" message?

konstantina-gramatova · 2022-04-20T09:08:57Z

It's undergoing a last review currently - will leave a comment to confirm merge-readiness.

Headers improvements

4739e13

- timeout is changed to time.Duration - usage of 'comma ok' idiom - add constant for ditto specific content type Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

Headers improvements

aacdd8b

- Update the doc according the refactoring - Convert timeout in time.Duration after unmarshaling JSON - Add more tests scenarios Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

konstantina-gramatova suggested changes Jan 26, 2022

View reviewed changes

Trifonova.Antonia added 2 commits March 30, 2022 15:52

Poc Headers improvements

79f4756

Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

antoniyatrifonova force-pushed the headers-improvements branch from 06e361c to 79f4756 Compare April 4, 2022 14:59

dimitar-dimitrow reviewed Apr 6, 2022

View reviewed changes

Trifonova.Antonia added 3 commits April 8, 2022 09:18

Poc Headers improvements

311a9d0

Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

Poc Headers improvements

f5c76b0

Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

Format issues

3bf8be7

Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

dimitar-dimitrow reviewed Apr 19, 2022

View reviewed changes

PoC headers

72f7beb

Signed-off-by: Antonia Trifonova antonia.trifonova@bosch.io

dimitar-dimitrow approved these changes Apr 20, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Headers improvements #23

Headers improvements #23

antoniyatrifonova commented Jan 24, 2022

antoniyatrifonova commented Jan 24, 2022

konstantina-gramatova Jan 26, 2022

konstantina-gramatova Jan 26, 2022

konstantina-gramatova Jan 26, 2022

d-stefanova Jan 27, 2022

konstantina-gramatova Jan 26, 2022

dimitar-dimitrow Apr 6, 2022

dimitar-dimitrow Apr 6, 2022

dimitar-dimitrow Apr 6, 2022

dimitar-dimitrow Apr 6, 2022

dimitar-dimitrow Apr 19, 2022

dimitar-dimitrow Apr 19, 2022

dimitar-dimitrow Apr 19, 2022

dimitar-dimitrow Apr 19, 2022

dimitar-dimitrow Apr 19, 2022

dimitar-dimitrow Apr 19, 2022

dimitar-dimitrow left a comment

thjaeckle commented Apr 20, 2022

konstantina-gramatova commented Apr 20, 2022

Headers improvements #23

Are you sure you want to change the base?

Headers improvements #23

Conversation

antoniyatrifonova commented Jan 24, 2022

antoniyatrifonova commented Jan 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitar-dimitrow left a comment

Choose a reason for hiding this comment

thjaeckle commented Apr 20, 2022

konstantina-gramatova commented Apr 20, 2022