MF-928 - Change CoAP lib #1233

dborovcanin · 2020-09-13T23:35:37Z

What does this do?

This pull request changes the lib used for CoAP protocol

Which issue(s) does this PR fix/relate to?

This pull request resolves #928 and partially #1144.

List any changes that modify/break current functionality

There are no such changes.

Have you included tests for your changes?

No, tests will be added in a separate pull request.

Did you document any new/modified functionality?

Yes.

Notes

The authorization option is changed to auth to be shorter.

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

codecov-commenter · 2020-09-14T16:56:08Z

Codecov Report

Merging #1233 into master will decrease coverage by 1.70%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1233      +/-   ##
==========================================
- Coverage   76.18%   74.48%   -1.71%     
==========================================
  Files         106      106              
  Lines        6983     6149     -834     
==========================================
- Hits         5320     4580     -740     
+ Misses       1278     1188      -90     
+ Partials      385      381       -4

Impacted Files	Coverage Δ
twins/redis/twins.go	`41.46% <0.00%> (-14.10%)`	⬇️
things/postgres/database.go	`59.09% <0.00%> (-9.88%)`	⬇️
things/redis/channels.go	`64.70% <0.00%> (-9.21%)`	⬇️
pkg/errors/errors.go	`67.64% <0.00%> (-5.53%)`	⬇️
authn/api/http/requests.go	`75.00% <0.00%> (-5.00%)`	⬇️
things/redis/things.go	`66.66% <0.00%> (-4.77%)`	⬇️
writers/cassandra/init.go	`66.66% <0.00%> (-4.77%)`	⬇️
writers/influxdb/messages.go	`69.23% <0.00%> (-4.11%)`	⬇️
pkg/sdk/go/version.go	`42.85% <0.00%> (-3.81%)`	⬇️
bootstrap/service.go	`74.34% <0.00%> (-3.78%)`	⬇️
... and 96 more

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 f18f2c1...c27082a. Read the comment docs.

drasko · 2020-09-15T00:13:22Z

cmd/coap/main.go

 	p := fmt.Sprintf(":%s", cfg.port)
 	l.Info(fmt.Sprintf("CoAP adapter service started, exposed port %s", cfg.port))
-	errs <- gocoap.ListenAndServe("udp", p, api.MakeCOAPHandler(svc, auth, l, respChan, cfg.pingPeriod))
+	r := api.MakeCoAPHandler(svc, l)
+	// errs <- gocoap.ListenAndServe("udp", p, api.MakeCOAPHandler(svc, auth, l, respChan, cfg.pingPeriod))


Remove dead code

drasko · 2020-09-15T00:15:27Z

coap/adapter.go

+		Token:  key,
+		ChanID: chanID,
+	}
+	_, err := svc.auth.CanAccessByKey(ctx, ar)


If var is not used, maybe you can embed error check.

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

cmd/coap/main.go

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

manuio

LGTM

mteodor · 2020-09-17T07:18:13Z

coap/adapter.go

+	delete(obs, token)
+	// If there are no observers left for the endpint, remove the map.
+	if len(obs) == 0 {
+		delete(svc.observers, endpoint)


do we need to remove this map? can it be reused here https://github.com/mainflux/mainflux/pull/1233/files#diff-a9745ffe405cfa25fb4f36761ce61eefR137

Yes, we can do it, but it's a potential memory leak. If we don't remove it and nobody actually adds the observer for the corresponding topic, we'll likely end up flooded with empty maps.

mteodor · 2020-09-17T07:24:57Z

coap/observer.go

+		if err := proto.Unmarshal(m.Data, &msg); err != nil {
+			return
+		}
+		if err := c.SendMessage(msg); err != nil {


nothing happens on error?

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

manuio · 2020-09-18T12:11:01Z

coap/adapter.go

+	_, err := svc.auth.CanAccessByKey(ctx, ar)
+	if err != nil {
+		return errors.Wrap(ErrUnauthorized, err)
+	}


You can inline this

manuio

LGTM

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

manuio · 2020-09-18T18:06:04Z

coap/client.go

+		c.logger.Error(fmt.Sprintf("Can't set content format: %s.", err))
+		return errors.Wrap(ErrOption, err)


Is this logged latter?

No, this is logged here. There is no convenient way to handle errors here since this is only a handler for the Nats subscription.

manuio · 2020-09-18T18:06:57Z

coap/client.go

+	err = c.client.WriteMessage(&m)
+	if err != nil {
+		c.logger.Error(fmt.Sprintf("Error sending message: %s.", err))
+	}
+	return err


You can inline this. And same question, is this logged later?

drasko

LGTM

manuio

LGTM

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

manuio

LGTM

drasko

LGTM

* Switch CoAP lib Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Revert removed adapter code Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * WIP CoAP refactor Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Add auth key Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Fix observers map Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Fix reading message body Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Fix subtopic parsing Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Fix error handling Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Fix multi-protocol communication Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Separate client from observer Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Remove unused config Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Remove TCP option Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Inline error check Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Add logging client errors Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Replace RWMutex since we're not using RLock Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Inline error handling Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Inline error handling Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>

dborovcanin force-pushed the MF-928 branch from d1b0aa2 to 80d32fe Compare September 14, 2020 16:51

dborovcanin marked this pull request as ready for review September 14, 2020 16:51

dborovcanin requested a review from a team as a code owner September 14, 2020 16:51

dborovcanin added 10 commits September 14, 2020 18:52

Switch CoAP lib

f86f607

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

Revert removed adapter code

0a98965

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

WIP CoAP refactor

85aded2

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

Add auth key

542c9bc

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

Fix observers map

56e8582

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

Fix reading message body

273cb14

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

Fix subtopic parsing

17d0caa

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

Fix error handling

bf58b7c

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

Fix multi-protocol communication

6f40513

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

Separate client from observer

80d32fe

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

drasko requested changes Sep 15, 2020

View reviewed changes

Remove unused config

fa92908

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

manuio suggested changes Sep 15, 2020

View reviewed changes

cmd/coap/main.go Outdated Show resolved Hide resolved

dborovcanin added 2 commits September 15, 2020 15:23

Remove TCP option

209bd21

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

Inline error check

dd4bfe9

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

manuio previously approved these changes Sep 16, 2020

View reviewed changes

mteodor reviewed Sep 17, 2020

View reviewed changes

dborovcanin dismissed manuio’s stale review via 7423ae3 September 18, 2020 11:45

dborovcanin added 2 commits September 18, 2020 13:46

Add logging client errors

7423ae3

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

Replace RWMutex since we're not using RLock

b81993b

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

manuio reviewed Sep 18, 2020

View reviewed changes

manuio previously approved these changes Sep 18, 2020

View reviewed changes

blokovi previously approved these changes Sep 18, 2020

View reviewed changes

dborovcanin dismissed stale reviews from blokovi and manuio via 7ca088b September 18, 2020 12:56

Inline error handling

7ca088b

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

manuio reviewed Sep 18, 2020

View reviewed changes

drasko previously approved these changes Sep 20, 2020

View reviewed changes

manuio previously approved these changes Sep 21, 2020

View reviewed changes

dborovcanin dismissed stale reviews from manuio and drasko via c27082a September 21, 2020 07:28

Inline error handling

c27082a

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

manuio approved these changes Sep 21, 2020

View reviewed changes

drasko approved these changes Sep 22, 2020

View reviewed changes

drasko merged commit f10e49e into absmach:master Sep 22, 2020

dborovcanin deleted the MF-928 branch June 13, 2022 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF-928 - Change CoAP lib #1233

MF-928 - Change CoAP lib #1233

dborovcanin commented Sep 13, 2020

codecov-commenter commented Sep 14, 2020 •

edited

Loading

drasko Sep 15, 2020

drasko Sep 15, 2020

manuio left a comment

mteodor Sep 17, 2020

dborovcanin Sep 18, 2020

mteodor Sep 17, 2020

manuio Sep 18, 2020

manuio left a comment

manuio Sep 18, 2020

dborovcanin Sep 21, 2020

manuio Sep 18, 2020 •

edited

Loading

drasko left a comment

manuio left a comment

manuio left a comment

drasko left a comment

		c.logger.Error(fmt.Sprintf("Can't set content format: %s.", err))
		return errors.Wrap(ErrOption, err)

MF-928 - Change CoAP lib #1233

MF-928 - Change CoAP lib #1233

Conversation

dborovcanin commented Sep 13, 2020

What does this do?

Which issue(s) does this PR fix/relate to?

List any changes that modify/break current functionality

Have you included tests for your changes?

Did you document any new/modified functionality?

Notes

codecov-commenter commented Sep 14, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio 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

manuio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

manuio left a comment

Choose a reason for hiding this comment

manuio left a comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 14, 2020 •

edited

Loading

manuio Sep 18, 2020 •

edited

Loading