MF-1264 - Add support for JSON readers #1295

dborovcanin · 2020-11-30T18:48:42Z

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

What does this do?

This pull request updates the Mainflux Readers interface to match recently added support for storing arbitrary JSON data.

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

Resolves #1264.

List any changes that modify/break current functionality

There are no breaking changes.

Have you included tests for your changes?

No. Existing tests are fixed, but new tests for reading data in a format other than SenML need to be added.

Did you document any new/modified functionality?

No. The existing documentation is sufficient at the moment.

codecov-io · 2020-11-30T18:54:30Z

Codecov Report

Merging #1295 (ac754e5) into master (47217cb) will decrease coverage by 1.15%.
The diff coverage is 44.55%.

@@            Coverage Diff             @@
##           master    #1295      +/-   ##
==========================================
- Coverage   60.97%   59.81%   -1.16%     
==========================================
  Files         110      113       +3     
  Lines        8581     8797     +216     
==========================================
+ Hits         5232     5262      +30     
- Misses       2904     3078     +174     
- Partials      445      457      +12

Impacted Files	Coverage Δ
readers/api/responses.go	`100.00% <ø> (ø)`
writers/cassandra/init.go	`66.66% <ø> (ø)`
writers/postgres/messages.go	`16.34% <13.58%> (-36.49%)`	⬇️
writers/cassandra/messages.go	`34.14% <21.87%> (-49.19%)`	⬇️
writers/influxdb/messages.go	`43.24% <30.76%> (-25.99%)`	⬇️
readers/cassandra/messages.go	`57.33% <34.88%> (-28.72%)`	⬇️
readers/postgres/messages.go	`58.82% <40.00%> (-31.01%)`	⬇️
writers/mongodb/messages.go	`43.47% <47.36%> (-26.90%)`	⬇️
readers/mongodb/messages.go	`69.56% <52.94%> (-15.35%)`	⬇️
writers/influxdb/tags.go	`55.55% <55.55%> (ø)`
... and 8 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 47217cb...ac754e5. Read the comment docs.

drasko

LGTM

manuio · 2020-12-29T09:22:05Z

readers/mongodb/messages.go

-		if err := cursor.Decode(&m); err != nil {
-			return readers.MessagesPage{}, errors.Wrap(errReadMessages, err)
+	messages := []interface{}{}
+	// var m interface{}


Remove the comment please

manuio · 2020-12-29T11:01:40Z

readers/mongodb/messages.go

-		var m message
-		if err := cursor.Decode(&m); err != nil {
-			return readers.MessagesPage{}, errors.Wrap(errReadMessages, err)
+	messages := []interface{}{}


Should we use var messages []interface{} here?

manuio · 2020-12-29T11:02:24Z

readers/influxdb/messages_test.go


 		assert.Equal(t, tc.page.Total, result.Total, fmt.Sprintf("%s: expected %d got %d", desc, tc.page.Total, result.Total))
 	}
 }
+
+func fromSenml(in []senml.Message) []interface{} {
+	ret := []interface{}{}


var ret []interface{} ?

mteodor · 2020-12-29T11:09:57Z

docker/addons/influxdb-writer/docker-compose.yml

@@ -54,7 +54,7 @@ services:
    networks:
      - docker_mainflux-base-net
    volumes:
-      - ./subjects.toml:/config/subjects.toml
+      - ./config.toml:/config.toml


is this path ok?

Yes, because the path is changed from /config to the root (/).

pkg/transformers/json/transformer.go

manuio · 2020-12-29T11:17:54Z

readers/cassandra/messages.go

@@ -114,3 +137,33 @@ func buildCountQuery(chanID string, names []string) string {

 	return fmt.Sprintf(cql, condCQL)
 }
+
+func parseFlat(flat interface{}) interface{} {


Looks like this function is duplicated for each reader. Should we maybe export it?

Yes, I'll probably put it in the transformers/json as a utility function.

Maybe transformers/pkg as shared package? Its oft to have multiple pkg and internal (not just in root).

I put it in pkg/tranformers/json. It's a utility function used by JSON transformer and readers, so I guess it's OK to leave it there.

mteodor · 2020-12-29T11:21:10Z

pkg/transformers/json/transformer.go

+	if len(subs) == 0 {
+		return nil, errors.Wrap(ErrTransform, errUnknownFormat)
+	}
+	format := subs[len(subs)-1]


what if len == 1?

It will take the first element of the subtopics slice.

mteodor · 2020-12-29T11:23:04Z

pkg/transformers/json/transformer.go

+
+func flatten(prefix string, m, m1 map[string]interface{}) (map[string]interface{}, error) {
+	for k, v := range m1 {
+		if k == "publisher" || k == "protocol" || k == "channel" || k == "subtopic" || strings.Contains(k, sep) {


should we make appropriate constants for this?

mteodor · 2020-12-29T11:38:51Z

pkg/transformers/transformer.go

@@ -3,7 +3,9 @@

 package transformers

-import "github.com/mainflux/mainflux/pkg/messaging"
+import (


single import

mteodor · 2020-12-29T11:50:59Z

readers/postgres/messages.go

    WHERE %s ORDER BY time DESC
-    LIMIT :limit OFFSET :offset;`, fmtCondition(chanID, query))
+    LIMIT :limit OFFSET :offset;`, format, fmtCondition(chanID, query))


idedntation doesnt seem right here

mteodor · 2020-12-29T11:56:52Z

readers/postgres/messages.go

+	switch format {
+	case defTable:
+		for rows.Next() {
+			msg := dbMessage{Message: senml.Message{Channel: chanID}}


are you sure this works? Channel doesnt get overwritten?

mteodor · 2020-12-29T11:57:35Z

writers/cassandra/init.go

@@ -3,9 +3,12 @@

 package cassandra

-import "github.com/gocql/gocql"
+import (


single import

mteodor · 2020-12-29T11:58:14Z

writers/cassandra/messages.go

-			name, unit, value, string_value, bool_value, data_value, sum,
-			time, update_time)
-			VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`
+            name, unit, value, string_value, bool_value, data_value, sum,


We're using spaces, it's probably just an issue with the preview.

nmarcetic · 2020-12-29T14:04:59Z

readers/influxdb/messages.go

 	q := influxdata.Query{
 		Command:  cmd,
 		Database: repo.database,
 	}

-	ret := []senml.Message{}
+	ret := []interface{}{}


Can we create custom type on domain level, like in transformers and use it like []MsgPaylod ? With probably better naming :)

pkg/transformers/json/example_test.go

drasko · 2020-12-29T18:55:41Z

readers/influxdb/messages.go

+const (
+	countCol       = "count_protocol"
+	format         = "format"
+	defMeasurement = "messages"


It is a bit unclear (at least from the naming itself) what defMeasurement represents, maybe leave a short comment here.

It's basically the measurement / table / collection for SenML messages. It's called messages for the sake of backward compatibility.

drasko · 2020-12-29T18:59:15Z

readers/mongodb/messages_test.go

 		assert.Nil(t, err, fmt.Sprintf("%s: expected no error got %s", desc, err))
 		assert.ElementsMatch(t, tc.page.Messages, result.Messages, fmt.Sprintf("%s: expected %v got %v", desc, tc.page.Messages, result.Messages))
 		assert.Equal(t, tc.page.Total, result.Total, fmt.Sprintf("%s: expected %v got %v", desc, tc.page.Total, result.Total))
 	}
 }
+
+func fromSenml(in []senml.Message) []interface{} {
+	ret := []interface{}{}


Maybe var ret []interface{} here as well? I am not bothered with this one neither, but if I remember well we aligned to a previous definition before. Please compare and align to the code in other Mainflux services.

drasko · 2020-12-29T19:00:25Z

readers/postgres/messages.go

 	"github.com/mainflux/mainflux/pkg/transformers/senml"
 	"github.com/mainflux/mainflux/readers"
 )

 const errInvalid = "invalid_text_representation"

+const (
+	format   = "format"
+	defTable = "messages"


Same here, maybe small comment for a code reader.

Although here is really looks like you are using def for default, and maybe comment is not necessary.

drasko · 2020-12-29T19:09:54Z

readers/postgres/messages_test.go

@@ -168,3 +168,11 @@ func TestMessageReadAll(t *testing.T) {
 		assert.Equal(t, tc.page.Total, result.Total, fmt.Sprintf("%s: expected %v got %v", desc, tc.page.Total, result.Total))
 	}
 }
+
+func fromSenml(in []senml.Message) []interface{} {
+	ret := []interface{}{}


Same here - variable definition.

pkg/transformers/json/transformer.go

drasko

LGTM

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

nmarcetic · 2020-12-30T12:49:52Z

pkg/transformers/json/message.go

+
+// Messages represents a list of JSON messages.
+type Messages struct {
+	Messages []Message


A bit eye-catchy Messages.Messages, maybe Messages.List or Messages.Data or something like that. Detail.

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

writers/postgres/messages.go

Change was done.

dborovcanin force-pushed the json branch 4 times, most recently from b3f9e77 to f330638 Compare December 28, 2020 17:42

dborovcanin changed the base branch from json to master December 28, 2020 17:52

dborovcanin changed the title ~~NOISSUE - Fix Readers to match Writers interface~~ MF-1264 - Add support for JSON readers Dec 28, 2020

dborovcanin marked this pull request as ready for review December 28, 2020 23:45

dborovcanin requested a review from a team as a code owner December 28, 2020 23:45

drasko previously approved these changes Dec 29, 2020

View reviewed changes

manuio suggested changes Dec 29, 2020

View reviewed changes

manuio reviewed Dec 29, 2020

View reviewed changes

manuio suggested changes Dec 29, 2020

View reviewed changes

mteodor reviewed Dec 29, 2020

View reviewed changes

manuio previously requested changes Dec 29, 2020

View reviewed changes

pkg/transformers/json/transformer.go Show resolved Hide resolved

manuio reviewed Dec 29, 2020

View reviewed changes

mteodor reviewed Dec 29, 2020

View reviewed changes

writers/cassandra/init.go Outdated

@@ -3,9 +3,12 @@

package cassandra

import "github.com/gocql/gocql"

import (

Copy link

Contributor

mteodor Dec 29, 2020

Choose a reason for hiding this comment

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

single import

mteodor reviewed Dec 29, 2020

View reviewed changes

dborovcanin dismissed drasko’s stale review via ea3cd7a December 29, 2020 13:20

nmarcetic reviewed Dec 29, 2020

View reviewed changes

drasko requested changes Dec 29, 2020

View reviewed changes

pkg/transformers/json/transformer.go Show resolved Hide resolved

drasko previously approved these changes Dec 29, 2020

View reviewed changes

dborovcanin dismissed drasko’s stale review via c38b151 December 30, 2020 10:07

dborovcanin force-pushed the json branch from c38b151 to 5a793a3 Compare December 30, 2020 10:17

dborovcanin force-pushed the json branch 2 times, most recently from f289d27 to beb45fc Compare December 30, 2020 11:44

manuio and others added 18 commits December 30, 2020 12:45

MF-1254 - Create universal JSON writer (absmach#1260)

06e149e

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

Add JSON support to Readers

bb39e84

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

Fix Influx Reader tests

14a4681

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

Fix messages format query

8dc4608

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

Fix Postgres reader

534c621

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

Fix Cassandra Readers and writers

3dc624d

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

Fix Mongo reader

4155ba7

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

Extract utility method to the JSON transformer

5eb1675

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

Fix Influx and Postgres count

dda2411

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

Update JSON transformer

284e1f7

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

Fix Influxdb Reader total count

12d168c

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

Refactor init.go for Cassandra writer

5cc20f1

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

Create a Payload type

42ca0f2

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

Add comments for defaults

051fcd5

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

Fix variable declarations

4082ad9

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

Replace interface{} with a new type

c8eade8

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

Don't set channel just to overwrite it later

6f77feb

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

Fix range search

beb45fc

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

nmarcetic reviewed Dec 30, 2020

View reviewed changes

Rename Messages field

ac754e5

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

nmarcetic reviewed Dec 30, 2020

View reviewed changes

writers/postgres/messages.go Show resolved Hide resolved

nmarcetic approved these changes Dec 30, 2020

View reviewed changes

mteodor approved these changes Dec 30, 2020

View reviewed changes

drasko merged commit e326494 into absmach:master Dec 30, 2020

dborovcanin deleted the json branch July 21, 2021 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF-1264 - Add support for JSON readers #1295

MF-1264 - Add support for JSON readers #1295

dborovcanin commented Nov 30, 2020 •

edited

Loading

codecov-io commented Nov 30, 2020 •

edited

Loading

drasko left a comment

manuio Dec 29, 2020

manuio Dec 29, 2020

manuio Dec 29, 2020

mteodor Dec 29, 2020

dborovcanin Dec 29, 2020

manuio Dec 29, 2020

dborovcanin Dec 29, 2020

nmarcetic Dec 29, 2020

dborovcanin Dec 29, 2020

mteodor Dec 29, 2020

dborovcanin Dec 29, 2020 •

edited

Loading

mteodor Dec 29, 2020

mteodor Dec 29, 2020

mteodor Dec 29, 2020

mteodor Dec 29, 2020

mteodor Dec 29, 2020

mteodor Dec 29, 2020

dborovcanin Dec 29, 2020

nmarcetic Dec 29, 2020

drasko Dec 29, 2020

dborovcanin Dec 29, 2020

drasko Dec 29, 2020

drasko Dec 29, 2020

drasko Dec 29, 2020

drasko Dec 29, 2020

drasko left a comment

nmarcetic Dec 30, 2020

MF-1264 - Add support for JSON readers #1295

MF-1264 - Add support for JSON readers #1295

Conversation

dborovcanin commented Nov 30, 2020 • edited Loading

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?

codecov-io commented Nov 30, 2020 • edited Loading

Codecov Report

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

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

dborovcanin Dec 29, 2020 • edited Loading

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

drasko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dborovcanin commented Nov 30, 2020 •

edited

Loading

codecov-io commented Nov 30, 2020 •

edited

Loading

dborovcanin Dec 29, 2020 •

edited

Loading