MF-374 - Bring back CoAP adapter #413

dborovcanin · 2018-10-02T15:06:34Z

This pull request resolves #374.

codecov-io · 2018-10-02T15:12:55Z

Codecov Report

Merging #413 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #413   +/-   ##
=======================================
  Coverage   91.09%   91.09%           
=======================================
  Files          46       46           
  Lines        1706     1706           
=======================================
  Hits         1554     1554           
  Misses        103      103           
  Partials       49       49

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cacf57...30279ee. Read the comment docs.

drasko

Please add copyright headers. Otherwise looks good.

anovakovic01

@dusanb94 Good job! I left some small remarks for you to resolve, but generally, this looks great.

anovakovic01 · 2018-10-08T07:32:21Z

cmd/coap/main.go

+	envPort      string = "MF_COAP_ADAPTER_PORT"
+	envNatsURL   string = "MF_NATS_URL"
+	envThingsURL string = "MF_THINGS_URL"
+	envLogLevel  string = "MF_COAP_ADAPTER_LOG_LEVEL"


Remove redundant type declaration.

anovakovic01 · 2018-10-08T07:36:13Z

coap/README.md

+version: "2"
+services:
+  adapter:
+    image: mainflux/coap-adapter:[version]


Name of the image is mainflux/coap.

anovakovic01 · 2018-10-08T07:55:20Z

coap/adapter.go

+	svc.mu.Lock()
+	obs, ok := svc.subs[clientID]
+	if ok {
+		obs.Closed <- true


This is blocking operation, and you use it inside locked piece of code. Just be careful here not to make dead lock.

@dusanb94 You can't use defer ? Its much safer.

coap/api/logging.go

coap/api/transport.go

anovakovic01 · 2018-10-08T08:01:38Z

coap/api/transport.go

+func version(port string) {
+	b := bone.New()
+	b.GetFunc("/version", mainflux.Version("CoAP"))
+	http.ListenAndServe(port, b)


You should make two handlers: HTTP and CoAP, and handle this ListenAndServe part in main.

anovakovic01 · 2018-10-08T08:02:26Z

coap/nats/broker.go

+
+var _ mainflux.MessagePublisher = (*natsPublisher)(nil)
+
+const prefix = "channel"


First declare constants, then vars.

anovakovic01 · 2018-10-08T08:03:13Z

coap/api/transport.go

+type handler func(conn *net.UDPConn, addr *net.UDPAddr, msg *gocoap.Message) *gocoap.Message
+
+// NotFoundHandler handles erroneously formed requests.
+func NotFoundHandler(l *net.UDPConn, a *net.UDPAddr, m *gocoap.Message) *gocoap.Message {


As far as I can tell, there is no need for this to be public.

coap/adapter.go

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Uncomment error handling for authorization. Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Add CoAp adapter in Makefile services list and fix corresponding documentation. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Merge multipe `const` block int single and declare consts before vars. Un-export notFound handler since there is no need to export it. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

This separates CoAP and HTTP APIs. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

This PR is a part of CoAP adapter refactoring that will simplify adapter implementation. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Change CoAP message handling to simplify adapter implementation. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Update CoAP adapter to provide subset of necessary features from protocol specification. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

In case of the stopped ticker, its channel is NOT closed, so pinging might be left stuck waiting for the stopped ticker to send a notification. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Use more meaningful name for Handlers map. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Stop handler Ticker from ping goroutine rather than the cancel goroutine. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Fix potential leak of handlers providing check inside of put method. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Since MessageID satisfies observe option behaviour, use Message ID instead of local timestamp. Remove Thicker from handler and use it on transport layer. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Name `Observer` is used in protocol specification, so this naming makes code more self-documenting. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

coap/api/transport.go

drasko

LGTM

drasko · 2018-10-27T00:53:08Z

@dusanb94 this looks good. Please resolve remarks from @anovakovic01 and @manuio and let's merge this one.

Fix service name in startup log message. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

drasko

LGTM

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

anovakovic01 · 2018-10-29T10:57:05Z

cmd/coap/main.go

+	Port      string
+	NatsURL   string
+	ThingsURL string
+	LogLevel  string


These should be private.

anovakovic01 · 2018-10-29T10:59:46Z

coap/api/transport.go

+func observe(svc coap.Service, responses chan<- string) handler {
+	return func(conn *net.UDPConn, addr *net.UDPAddr, msg *gocoap.Message) *gocoap.Message {
+		var res *gocoap.Message
+		res = &gocoap.Message{


Why not using :=?

anovakovic01 · 2018-10-29T11:00:53Z

coap/api/util.go

+
+import (
+	"strings"
+)


import "strings"

anovakovic01 · 2018-10-29T11:02:00Z

coap/api/util.go

+	"strings"
+)
+
+const keyHeader = "key"


Maybe this should be called authorization like in other protocol adapters.

manuio

LGTM

Config fields from main.go should not be exported; minor style changes. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Use `authorization` value in URI-Query option instead of `key`. This mimics Authorization header in some other protocols (e.g. HTTP). Please note that this value can be replaced with simple `auth` to save space, due to constrained URI-Query option size. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

* Bring old CoAP code back Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Fix channel ID formatting due to type change Uncomment error handling for authorization. Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Update CoAP adapter docs Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Add copyright headers Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Remove redundant type declaration Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Add CoAP adapter to the list of services Add CoAp adapter in Makefile services list and fix corresponding documentation. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Refactor CoAP code Merge multipe `const` block int single and declare consts before vars. Un-export notFound handler since there is no need to export it. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Update http version endpoint This separates CoAP and HTTP APIs. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Refactor CoAP POST method handling This PR is a part of CoAP adapter refactoring that will simplify adapter implementation. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Refactor CoAP adapter Change CoAP message handling to simplify adapter implementation. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Add backoff timeout for server ping to client Update CoAP adapter to provide subset of necessary features from protocol specification. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Fix leaking locked goroutine In case of the stopped ticker, its channel is NOT closed, so pinging might be left stuck waiting for the stopped ticker to send a notification. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Format code Use more meaningful name for Handlers map. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Use and stop ticker from the same goroutine Stop handler Ticker from ping goroutine rather than the cancel goroutine. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Check if subscription already exists in put method Fix potential leak of handlers providing check inside of put method. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Use MessageID as Observe option Since MessageID satisfies observe option behaviour, use Message ID instead of local timestamp. Remove Thicker from handler and use it on transport layer. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Use name Observer insted of Handler Name `Observer` is used in protocol specification, so this naming makes code more self-documenting. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Add CoAP adapter to docker-compose.yml Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Add copyright headers Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Remove unused constants Fix service name in startup log message. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Add metrics endpoint Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Refactor code Config fields from main.go should not be exported; minor style changes. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com> * Update authorization URI-Query option Use `authorization` value in URI-Query option instead of `key`. This mimics Authorization header in some other protocols (e.g. HTTP). Please note that this value can be replaced with simple `auth` to save space, due to constrained URI-Query option size. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

dborovcanin added the work-in-progress label Oct 2, 2018

dborovcanin force-pushed the MF-374 branch from c0938a7 to 29607aa Compare October 2, 2018 15:07

dborovcanin force-pushed the MF-374 branch 2 times, most recently from 2cee6c4 to 6799a1d Compare October 3, 2018 09:08

dborovcanin removed the work-in-progress label Oct 4, 2018

drasko previously approved these changes Oct 7, 2018

View reviewed changes

anovakovic01 suggested changes Oct 8, 2018

View reviewed changes

dborovcanin dismissed drasko’s stale review via acb8ff2 October 17, 2018 10:10

nmarcetic requested a review from manuio October 17, 2018 10:24

manuio suggested changes Oct 17, 2018

View reviewed changes

coap/adapter.go Outdated Show resolved Hide resolved

dborovcanin added the work-in-progress label Oct 17, 2018

dborovcanin force-pushed the MF-374 branch 2 times, most recently from 6e1e3dd to 5124bb7 Compare October 24, 2018 23:55

dborovcanin removed the work-in-progress label Oct 25, 2018

dborovcanin added 15 commits October 26, 2018 10:46

Bring old CoAP code back

50fe512

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Fix channel ID formatting due to type change

6bb8e59

Uncomment error handling for authorization. Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Update CoAP adapter docs

44f98cc

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Add copyright headers

e872132

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Remove redundant type declaration

351a051

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Add CoAP adapter to the list of services

e248362

Add CoAp adapter in Makefile services list and fix corresponding documentation. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Refactor CoAP code

8d2dbed

Merge multipe `const` block int single and declare consts before vars. Un-export notFound handler since there is no need to export it. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Update http version endpoint

d1b9349

This separates CoAP and HTTP APIs. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Refactor CoAP POST method handling

56b46ca

This PR is a part of CoAP adapter refactoring that will simplify adapter implementation. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Refactor CoAP adapter

6d6abd2

Change CoAP message handling to simplify adapter implementation. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Add backoff timeout for server ping to client

e178038

Update CoAP adapter to provide subset of necessary features from protocol specification. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Fix leaking locked goroutine

14310d2

In case of the stopped ticker, its channel is NOT closed, so pinging might be left stuck waiting for the stopped ticker to send a notification. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Format code

47c339f

Use more meaningful name for Handlers map. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Use and stop ticker from the same goroutine

d93ee1b

Stop handler Ticker from ping goroutine rather than the cancel goroutine. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Check if subscription already exists in put method

597ff3f

Fix potential leak of handlers providing check inside of put method. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

dborovcanin added 3 commits October 26, 2018 10:46

Use MessageID as Observe option

b236533

Since MessageID satisfies observe option behaviour, use Message ID instead of local timestamp. Remove Thicker from handler and use it on transport layer. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Use name Observer insted of Handler

7b4631f

Name `Observer` is used in protocol specification, so this naming makes code more self-documenting. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

Add CoAP adapter to docker-compose.yml

4dee637

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

dborovcanin force-pushed the MF-374 branch from 4cf4dd4 to 4dee637 Compare October 26, 2018 08:48

Add copyright headers

34301af

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

manuio suggested changes Oct 26, 2018

View reviewed changes

coap/api/transport.go Outdated Show resolved Hide resolved

drasko previously approved these changes Oct 27, 2018

View reviewed changes

Remove unused constants

2c547f3

Fix service name in startup log message. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

dborovcanin dismissed drasko’s stale review via 2c547f3 October 27, 2018 17:25

drasko previously approved these changes Oct 28, 2018

View reviewed changes

Add metrics endpoint

e8d573a

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

dborovcanin dismissed drasko’s stale review via e8d573a October 29, 2018 10:31

anovakovic01 suggested changes Oct 29, 2018

View reviewed changes

manuio previously approved these changes Oct 29, 2018

View reviewed changes

dborovcanin dismissed manuio’s stale review via 6be0226 October 31, 2018 12:18

Refactor code

416482c

Config fields from main.go should not be exported; minor style changes. Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

dborovcanin force-pushed the MF-374 branch from 6be0226 to 416482c Compare October 31, 2018 12:21

manuio approved these changes Oct 31, 2018

View reviewed changes

anovakovic01 approved these changes Oct 31, 2018

View reviewed changes

anovakovic01 merged commit d6755e4 into absmach:master Oct 31, 2018

dborovcanin deleted the MF-374 branch January 31, 2019 10:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF-374 - Bring back CoAP adapter #413

MF-374 - Bring back CoAP adapter #413

dborovcanin commented Oct 2, 2018

codecov-io commented Oct 2, 2018 •

edited

Loading

drasko left a comment

anovakovic01 left a comment

anovakovic01 Oct 8, 2018

anovakovic01 Oct 8, 2018

anovakovic01 Oct 8, 2018

nmarcetic Oct 17, 2018

anovakovic01 Oct 8, 2018

anovakovic01 Oct 8, 2018

anovakovic01 Oct 8, 2018

drasko left a comment

drasko commented Oct 27, 2018

drasko left a comment

anovakovic01 Oct 29, 2018

anovakovic01 Oct 29, 2018

anovakovic01 Oct 29, 2018

anovakovic01 Oct 29, 2018 •

edited

Loading

manuio left a comment


		var _ mainflux.MessagePublisher = (*natsPublisher)(nil)

		const prefix = "channel"

MF-374 - Bring back CoAP adapter #413

MF-374 - Bring back CoAP adapter #413

Conversation

dborovcanin commented Oct 2, 2018

codecov-io commented Oct 2, 2018 • edited Loading

Codecov Report

drasko left a comment

Choose a reason for hiding this comment

anovakovic01 left a 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

drasko left a comment

Choose a reason for hiding this comment

drasko commented Oct 27, 2018

drasko left a 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

anovakovic01 Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

manuio left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 2, 2018 •

edited

Loading

anovakovic01 Oct 29, 2018 •

edited

Loading