MF-596 - Add subtopic to RawMessage #642

beres · 2019-03-07T16:46:12Z

What does this do?

The patch add subtopic support to the following components

cli
http
mqtt
normalizer
all readers
sdk messages
all writers
ws

Not updated:

coap
lora

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

Put here Resolves #XXX to auto-close the issue that your PR fixes (if such)

List any changes that modify/break current functionality

Have you included tests for your changes?

Did you document any new/modified functionality?

Notes

chombium

Hi @beres, thanks for your contribution. The code looks good, but IMHO there are few things which need improvements:

As @drasko already mentioned on Gitter you should sign the commits.
There are few tests which are failing:

# github.com/mainflux/mainflux/readers/influxdb_test
readers/influxdb/messages_test.go:116:27: not enough arguments in call to reader.ReadAll
        have (string, uint64, uint64)
        want (string, uint64, uint64, map[string]string)
# github.com/mainflux/mainflux/readers/cassandra_test
readers/cassandra/messages_test.go:102:27: not enough arguments in call to reader.ReadAll
        have (string, uint64, uint64)
        want (string, uint64, uint64, map[string]string)
# github.com/mainflux/mainflux/readers/mongodb_test
readers/mongodb/messages_test.go:109:27: not enough arguments in call to reader.ReadAll
        have (string, uint64, uint64)
        want (string, uint64, uint64, map[string]string)

You can check if all the test are still passing with make test. Don't forget to start your docker daemon because the tests are using Dockertest.
You can also run each test separately from the command line with for i.e. GOCACHE=off go test -v -race readers/influxdb/messages_test.go

There are no tests about the parts regarding the subtopic which you've added
Addition of few sentences about the subtopics in the user guide would also be nice

Keep up the good work.

Best Regards,
Jovan

mteodor

overall looks ok

mqtt/mqtt.js

writers/cassandra/init.go

Modified and tested: - cli - http - mqtt - normalizer - all readers - sdk messages - all writers - ws Missing: - coap - lora Signed-off-by: ale <ale@metaverso.org>

- add some test on readers Signed-off-by: ale <ale@metaverso.org>

dborovcanin

Thanks for the contribution, @beres!

Please sign your commits, add the reference to an existing issue in PR description (#596), remove all TODOs from the code and check CI.

cli/message.go

http/api/transport.go

http/nats/publisher.go

readers/cassandra/messages.go

readers/influxdb/messages.go

http/api/transport.go

readers/cassandra/messages.go

- update http/transport regexp to match subtopic names with only \w- - update ws/transport regexp like http ones with also the wildcard * and > Signed-off-by: ale <ale@metaverso.org>

Signed-off-by: ale <ale@metaverso.org>

http/api/transport.go

mqtt/mqtt.js

readers/cassandra/messages_test.go

readers/influxdb/messages_test.go

readers/mocks/messages.go

readers/mongodb/messages_test.go

ws/api/transport.go

ws/nats/publisher.go

drasko · 2019-03-08T21:32:10Z

@beres I think you are getting closer. Please take into account @anovakovic01's remarks so we can continue with review and iterations.

@anovakovic01

- renamed getDestChannel to fmtSubject - update api/transport and ws/transport route to be more readable - fix mqtt syntax - renamed func andQuery to query as suggested by @anovakovic01 - have a nice we :) Signed-off-by: ale <ale@metaverso.org>

- fix regexp added missing allowed chars - and _ on coap/api/transport - fix subtopic clean suffix / if present on coap/api/transport - improve regexp on http and ws /api/transport, now does not accept url that do not strictly match - add some ws subtopic tests Signed-off-by: ale <ale@metaverso.org>

drasko · 2019-03-11T19:16:10Z

@beres we are in a hurry with this one - we would like to release 0.8 on Friday. Can you please take in the account the remarks and correct the PR so that we can test it and merge it?

beres · 2019-03-12T07:43:02Z

hey @drasko with last commit all the report of @anovakovic01 have been fixed, did i miss something?

ws/api/transport.go

Signed-off-by: ale <ale@metaverso.org>

codecov-io · 2019-03-13T13:41:52Z

Codecov Report

Merging #642 into master will decrease coverage by 0.73%.
The diff coverage is 78.74%.

@@            Coverage Diff            @@
##           master    #642      +/-   ##
=========================================
- Coverage   87.13%   86.4%   -0.74%     
=========================================
  Files          62      62              
  Lines        3887    3707     -180     
=========================================
- Hits         3387    3203     -184     
- Misses        344     346       +2     
- Partials      156     158       +2

Impacted Files	Coverage Δ
writers/cassandra/init.go	`60% <ø> (-18.58%)`	⬇️
readers/api/requests.go	`100% <ø> (ø)`	⬆️
readers/cassandra/messages.go	`91.66% <100%> (+2.43%)`	⬆️
writers/mongodb/messages.go	`100% <100%> (ø)`	⬆️
readers/mongodb/messages.go	`91.3% <100%> (-3.44%)`	⬇️
readers/api/endpoint.go	`100% <100%> (ø)`	⬆️
writers/influxdb/messages.go	`85.71% <100%> (-0.78%)`	⬇️
ws/adapter.go	`93.75% <100%> (ø)`	⬆️
writers/cassandra/messages.go	`100% <100%> (ø)`	⬆️
sdk/go/message.go	`38.59% <35.71%> (-10.02%)`	⬇️
... and 15 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 2f09701...d746557. Read the comment docs.

…ng.org/doc/go1.10#test Signed-off-by: ale <ale@metaverso.org>

drasko · 2019-03-13T18:38:52Z

Looks like CI is passing and we are getting close. @anovakovic01 @dusanb94 @nmarcetic @manuio @mteodor please review.

coap/api/transport.go

http/api/transport.go

anovakovic01 · 2019-03-14T08:54:03Z

http/api/transport.go

-	auth             mainflux.ThingsServiceClient
+	errMalformedData  = errors.New("malformed request data")
+	auth              mainflux.ThingsServiceClient
+	channelPartRegExp = regexp.MustCompile(`^/channels/([\w\-]+)/messages((/[^/]+)*)*(\?.*)?$`)


I think that this ((/[^/]+)*)* can be simplified with ((/[^/]+)*)?. Also, @drasko do we want to add support for query parameters in HTTP adapter? I'm not sure that we should allow in subtopic characters such as ., > and *, which are specific to NATS.

actually http does not use query parameters, i use the same regexp for coap/ws/http. If you do not want a specific regexp per adapter, it could be placed in a shared place

@beres If you want to do this, topics.go file in the project root is a good place for that.

@dusanb94 could also be the place for common subtopic function?

@beres If that function is shared and does not accept any adapter-specific arguments, then I'd say yes.

- minor changes Signed-off-by: ale <ale@metaverso.org>

Signed-off-by: ale <ale@metaverso.org>

anovakovic01

LGTM!

drasko

LGTM

drasko · 2019-03-14T21:06:01Z

@manuio @chombium we need your positive reviews to merge this PR

manuio

LGTM

drasko · 2019-03-15T17:44:54Z

@beres thanks a lot for this important contribution! Good work here.

@anovakovic01

* Commit for mainflux-596 Modified and tested: - cli - http - mqtt - normalizer - all readers - sdk messages - all writers - ws Missing: - coap - lora Signed-off-by: ale <ale@metaverso.org> * - fix subtopic name in, when starting with dot, http/ws/mqtt - add some test on readers Signed-off-by: ale <ale@metaverso.org> * - fix string concatenation - update http/transport regexp to match subtopic names with only \w- - update ws/transport regexp like http ones with also the wildcard * and > Signed-off-by: ale <ale@metaverso.org> * added subtopic support to coap adapter Signed-off-by: ale <ale@metaverso.org> * - update replace functions with replaceall when needed - renamed getDestChannel to fmtSubject - update api/transport and ws/transport route to be more readable - fix mqtt syntax - renamed func andQuery to query as suggested by @anovakovic01 - have a nice we :) Signed-off-by: ale <ale@metaverso.org> * - fix error declaration on ws/nat/publisher - fix regexp added missing allowed chars - and _ on coap/api/transport - fix subtopic clean suffix / if present on coap/api/transport - improve regexp on http and ws /api/transport, now does not accept url that do not strictly match - add some ws subtopic tests Signed-off-by: ale <ale@metaverso.org> * - enabled wildcard chars on coap/api/transport - allow use special chars on http and ws api/transport Signed-off-by: ale <ale@metaverso.org> * - use strings.Replace() insted ReplaceAll() Signed-off-by: ale <ale@metaverso.org> * - allow every chars on subtopics - fix replace error on mqtt Signed-off-by: ale <ale@metaverso.org> * fix cassandra test Signed-off-by: ale <ale@metaverso.org> * fix ws test with invalid subtopic Signed-off-by: ale <ale@metaverso.org> * fix invalid GOCACHE in go1.12, replaced by -count 1, see https://golang.org/doc/go1.10#test Signed-off-by: ale <ale@metaverso.org> * - improve regexp on http/ws api/transport - minor changes Signed-off-by: ale <ale@metaverso.org> * - add generic function parseSubtopic on ws/http adapters Signed-off-by: ale <ale@metaverso.org> * - add generic function fmtSubtopic on coap adapter Signed-off-by: ale <ale@metaverso.org>

@anovakovic01

* Commit for mainflux-596 Modified and tested: - cli - http - mqtt - normalizer - all readers - sdk messages - all writers - ws Missing: - coap - lora Signed-off-by: ale <ale@metaverso.org> * - fix subtopic name in, when starting with dot, http/ws/mqtt - add some test on readers Signed-off-by: ale <ale@metaverso.org> * - fix string concatenation - update http/transport regexp to match subtopic names with only \w- - update ws/transport regexp like http ones with also the wildcard * and > Signed-off-by: ale <ale@metaverso.org> * added subtopic support to coap adapter Signed-off-by: ale <ale@metaverso.org> * - update replace functions with replaceall when needed - renamed getDestChannel to fmtSubject - update api/transport and ws/transport route to be more readable - fix mqtt syntax - renamed func andQuery to query as suggested by @anovakovic01 - have a nice we :) Signed-off-by: ale <ale@metaverso.org> * - fix error declaration on ws/nat/publisher - fix regexp added missing allowed chars - and _ on coap/api/transport - fix subtopic clean suffix / if present on coap/api/transport - improve regexp on http and ws /api/transport, now does not accept url that do not strictly match - add some ws subtopic tests Signed-off-by: ale <ale@metaverso.org> * - enabled wildcard chars on coap/api/transport - allow use special chars on http and ws api/transport Signed-off-by: ale <ale@metaverso.org> * - use strings.Replace() insted ReplaceAll() Signed-off-by: ale <ale@metaverso.org> * - allow every chars on subtopics - fix replace error on mqtt Signed-off-by: ale <ale@metaverso.org> * fix cassandra test Signed-off-by: ale <ale@metaverso.org> * fix ws test with invalid subtopic Signed-off-by: ale <ale@metaverso.org> * fix invalid GOCACHE in go1.12, replaced by -count 1, see https://golang.org/doc/go1.10#test Signed-off-by: ale <ale@metaverso.org> * - improve regexp on http/ws api/transport - minor changes Signed-off-by: ale <ale@metaverso.org> * - add generic function parseSubtopic on ws/http adapters Signed-off-by: ale <ale@metaverso.org> * - add generic function fmtSubtopic on coap adapter Signed-off-by: ale <ale@metaverso.org>

drasko changed the title ~~MF-596~~ MF-596 - Add subtopic to RawMessage Mar 7, 2019

drasko requested review from manuio, nmarcetic, anovakovic01, dborovcanin, drasko and mteodor March 7, 2019 18:50

chombium suggested changes Mar 7, 2019

View reviewed changes

mteodor reviewed Mar 8, 2019

View reviewed changes

mqtt/mqtt.js Outdated Show resolved Hide resolved

mqtt/mqtt.js Show resolved Hide resolved

manuio suggested changes Mar 8, 2019

View reviewed changes

writers/cassandra/init.go Show resolved Hide resolved

beres force-pushed the mainflux-596 branch from f1ed11c to 672194d Compare March 8, 2019 09:11

beres added 2 commits March 8, 2019 10:11

Commit for mainflux-596

60e2a79

Modified and tested: - cli - http - mqtt - normalizer - all readers - sdk messages - all writers - ws Missing: - coap - lora Signed-off-by: ale <ale@metaverso.org>

- fix subtopic name in, when starting with dot, http/ws/mqtt

672194d

- add some test on readers Signed-off-by: ale <ale@metaverso.org>

dborovcanin requested changes Mar 8, 2019

View reviewed changes

drasko requested changes Mar 8, 2019

View reviewed changes

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

readers/cassandra/messages.go Outdated Show resolved Hide resolved

beres force-pushed the mainflux-596 branch from 2d3add3 to 7d4cd40 Compare March 8, 2019 13:58

- fix string concatenation

7d4cd40

- update http/transport regexp to match subtopic names with only \w- - update ws/transport regexp like http ones with also the wildcard * and > Signed-off-by: ale <ale@metaverso.org>

beres force-pushed the mainflux-596 branch from bbaa80a to b02c5fe Compare March 8, 2019 14:28

added subtopic support to coap adapter

b02c5fe

Signed-off-by: ale <ale@metaverso.org>

anovakovic01 suggested changes Mar 8, 2019

View reviewed changes

beres force-pushed the mainflux-596 branch from e5c9716 to dc506b3 Compare March 8, 2019 17:49

beres added 2 commits March 11, 2019 14:23

beres force-pushed the mainflux-596 branch from dc506b3 to 638efec Compare March 11, 2019 14:14

drasko requested changes Mar 12, 2019

View reviewed changes

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

This was referenced Mar 12, 2019

Add subtopic to RawMessage #596

Closed

NOISSUE: add subtopic support for nats messages #565

Closed

fix ws test with invalid subtopic

8f759c2

Signed-off-by: ale <ale@metaverso.org>

beres added 3 commits March 13, 2019 15:02

fix invalid GOCACHE in go1.12, replaced by -count 1, see https://gola…

75d07ab

…ng.org/doc/go1.10#test Signed-off-by: ale <ale@metaverso.org>

Merge master into mainflux-596

35b17b1

Merge branch 'master' into mainflux-596

0ccee00

dborovcanin previously approved these changes Mar 14, 2019

View reviewed changes

anovakovic01 suggested changes Mar 14, 2019

View reviewed changes

- improve regexp on http/ws api/transport

c4b97ff

- minor changes Signed-off-by: ale <ale@metaverso.org>

beres dismissed dborovcanin’s stale review via c4b97ff March 14, 2019 11:13

beres force-pushed the mainflux-596 branch from 33cf9aa to c4b97ff Compare March 14, 2019 11:13

- add generic function parseSubtopic on ws/http adapters

831b200

Signed-off-by: ale <ale@metaverso.org>

drasko assigned beres Mar 14, 2019

drasko added this to the 0.9.0 milestone Mar 14, 2019

nmarcetic modified the milestones: 0.9.0, 0.8.0 Mar 14, 2019

beres added 2 commits March 14, 2019 18:39

Merge branch 'master' into mainflux-596

775ff24

- add generic function fmtSubtopic on coap adapter

d746557

Signed-off-by: ale <ale@metaverso.org>

anovakovic01 approved these changes Mar 14, 2019

View reviewed changes

drasko approved these changes Mar 14, 2019

View reviewed changes

drasko requested review from chombium and manuio March 14, 2019 21:05

manuio approved these changes Mar 14, 2019

View reviewed changes

chombium approved these changes Mar 15, 2019

View reviewed changes

nmarcetic merged commit 61b2d6b into absmach:master Mar 15, 2019

beres deleted the mainflux-596 branch April 5, 2019 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF-596 - Add subtopic to RawMessage #642

MF-596 - Add subtopic to RawMessage #642

beres commented Mar 7, 2019

chombium left a comment

mteodor left a comment

dborovcanin left a comment

drasko commented Mar 8, 2019

drasko commented Mar 11, 2019

beres commented Mar 12, 2019

codecov-io commented Mar 13, 2019 •

edited

Loading

drasko commented Mar 13, 2019

anovakovic01 Mar 14, 2019

beres Mar 14, 2019

dborovcanin Mar 15, 2019

beres Mar 15, 2019

dborovcanin Mar 15, 2019

anovakovic01 left a comment

drasko left a comment

drasko commented Mar 14, 2019 •

edited

Loading

manuio left a comment

drasko commented Mar 15, 2019

MF-596 - Add subtopic to RawMessage #642

MF-596 - Add subtopic to RawMessage #642

Conversation

beres commented Mar 7, 2019

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

chombium left a comment

Choose a reason for hiding this comment

mteodor left a comment

Choose a reason for hiding this comment

dborovcanin left a comment

Choose a reason for hiding this comment

drasko commented Mar 8, 2019

drasko commented Mar 11, 2019

beres commented Mar 12, 2019

codecov-io commented Mar 13, 2019 • edited Loading

Codecov Report

drasko commented Mar 13, 2019

anovakovic01 Mar 14, 2019

Choose a reason for hiding this comment

beres Mar 14, 2019

Choose a reason for hiding this comment

dborovcanin Mar 15, 2019

Choose a reason for hiding this comment

beres Mar 15, 2019

Choose a reason for hiding this comment

dborovcanin Mar 15, 2019

Choose a reason for hiding this comment

anovakovic01 left a comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

drasko commented Mar 14, 2019 • edited Loading

manuio left a comment

Choose a reason for hiding this comment

drasko commented Mar 15, 2019

codecov-io commented Mar 13, 2019 •

edited

Loading

drasko commented Mar 14, 2019 •

edited

Loading