MF-397 - Introduce Thing Groups #1259
Conversation
things/groups.go
Outdated
| Description() string | ||
| Metadata() Metadata | ||
|
|
||
| SetID(id string) |
There was a problem hiding this comment.
Do we really need all these setters? Why not passing group struct pointer and changing the value directly.
This looks like some OO encapsulation, maybe it is needed but it Go it gives me "code smell".
There was a problem hiding this comment.
Idea here and in things/http.go is to extract the common code that can be used for other entities that we wish to group
so Group is an interface that would have its implementations in each entity package itself
Of course if this solution is ok, http.go and groups.go would be moved to appropriate package
There was a problem hiding this comment.
This is OK, but I do not like these setters. You can create an var struct without the setters, and then change internal struct directly through pointer receiver.
dborovcanin
changed the title
things/groups.go
Outdated
| "context" | ||
| ) | ||
|
|
||
| type Group interface { |
There was a problem hiding this comment.
I'm also not sure about this idea. Why can't we use Group struct here? This interface resembles struct anyways (explains fields, rather than behavior). It is a limitation compared to the interface, but it also provides better type safety and this looks sufficient for Mainflux needs anyways.
There was a problem hiding this comment.
Because Thing groups and User groups could differ in implementation and this interface should be common denominator.
Idea was to have at least some parts modular, that I put this into a package and reuse it in Users, Things and Channels groups, no need to rewrite again each endpoint for example.
This is something that was asked for by @drasko, at least one part. I can easily copy paste and refactor this into the usual place
There was a problem hiding this comment.
@mteodor this is OK, but remove getters and setters, use strucs and change the fields directly via pointer receiver.
There was a problem hiding this comment.
Group entity itself, however, is the same in both cases, right?
mteodor
changed the title
internal/groups/groups.go
Outdated
| // ListChildren retrieves groups that are children to group identified by parentID | ||
| ListChildren(ctx context.Context, token, parentID string, offset, limit uint64, m Metadata) (GroupPage, error) | ||
|
|
||
| // ListParents retrieves groups that are parent to group identified by childID. | ||
| ListParents(ctx context.Context, token, childID string, offset, limit uint64, m Metadata) (GroupPage, error) |
There was a problem hiding this comment.
While I understand that using parents and children terminology is natural, I'm afraid it's also ambiguous because parents imply that one Group has multiple parents, and children implies only direct descendants.
There was a problem hiding this comment.
Also, did you consider adding a feature to search genealogy only up to some depth, something like: give me only my children? Or: give me my only my children and children of my children. Same for parents. There are many possibilities for searching group members, but let's not get into that in this PR, this is just a remark for future reference.
There was a problem hiding this comment.
Regarding https://github.com/mainflux/mainflux/pull/1259/files#r511910630, do Service methods return direct ascendents and descendants or the entire tree? This should be better explained in the documentation comments so that the one who implements knows the desirable behavior of the Service interface.
internal/groups/groups.go
Outdated
| // ListChildren retrieves groups that are children to group identified by parentID | ||
| ListChildren(ctx context.Context, token, parentID string, offset, limit uint64, m Metadata) (GroupPage, error) | ||
|
|
||
| // ListParents retrieves groups that are parent to group identified by childID. | ||
| ListParents(ctx context.Context, token, childID string, offset, limit uint64, m Metadata) (GroupPage, error) |
There was a problem hiding this comment.
Regarding https://github.com/mainflux/mainflux/pull/1259/files#r511910630, do Service methods return direct ascendents and descendants or the entire tree? This should be better explained in the documentation comments so that the one who implements knows the desirable behavior of the Service interface.
internal/groups/api/transport.go
Outdated
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| return nil, errors.Wrap(groups.ErrFailedDecode, err) | ||
| } | ||
| req.ID = bone.GetValue(r, "groupID") |
Codecov Report
@@ Coverage Diff @@
## master #1259 +/- ##
==========================================
- Coverage 67.52% 64.38% -3.15%
==========================================
Files 108 109 +1
Lines 7316 7819 +503
==========================================
+ Hits 4940 5034 +94
- Misses 1936 2346 +410
+ Partials 440 439 -1
Continue to review full report at Codecov.
|
| return groupRes{}, errors.Wrap(groups.ErrMalformedEntity, err) | ||
| } | ||
|
|
||
| group := groups.Group{ |
There was a problem hiding this comment.
Here we use group and in the function before g. Please choose one.
| func DecodeGroupCreate(_ context.Context, r *http.Request) (interface{}, error) { | ||
| if !strings.Contains(r.Header.Get("Content-Type"), contentType) { | ||
| return nil, groups.ErrUnsupportedContentType | ||
| } |
| var req createGroupReq | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| return nil, errors.Wrap(groups.ErrFailedDecode, err) | ||
| } |
| if !strings.Contains(r.Header.Get("Content-Type"), contentType) { | ||
| return nil, groups.ErrUnsupportedContentType | ||
| } | ||
| var req updateGroupReq |
| if err := req.validate(); err != nil { | ||
| return groupPageRes{}, errors.Wrap(groups.ErrMalformedEntity, err) | ||
| } | ||
| page, err := svc.ListParents(ctx, req.token, req.groupID, req.level, req.metadata) |
There was a problem hiding this comment.
Please stay consistent in adding empty lines comparing to other function.
| child := toViewGroupRes(*ch) | ||
| view.Children = append(view.Children, &child) | ||
| } | ||
| return view |
There was a problem hiding this comment.
If empty line was added above, add one here as well
| if req.token == "" { | ||
| return groups.ErrUnauthorizedAccess | ||
| } | ||
|
|
There was a problem hiding this comment.
If function below do not have white lines, please suppers these as well.
There was a problem hiding this comment.
To be consistent I'd add it in the function where it's missing.
mteodor
force-pushed
the
things-group
branch
from
bb10e89
to
e05ba26
Compare
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
things/postgres/init.go
Outdated
| PRIMARY KEY (owner_id, path), | ||
| FOREIGN KEY (parent_id) REFERENCES thing_groups (id) ON DELETE CASCADE ON UPDATE CASCADE | ||
| )`, | ||
| `CREATE TABLE IF NOT EXISTS id ( |
There was a problem hiding this comment.
Table name id is less than ideal, consider renaming to members.
things/postgres/groups.go
Outdated
| q := fmt.Sprintf(`SELECT g.id, g.name, g.owner_id, g.parent_id, g.description, g.metadata, g.path, nlevel(g.path) as level FROM | ||
| (SELECT thing_groups.path FROM thing_groups where id = :parent_id) parent LEFT JOIN | ||
| (SELECT id, name, owner_id, parent_id, description,metadata, path FROM thing_groups) g | ||
| ON g.path @> parent.path AND nlevel(parent.path) - nlevel(g.path) >= 0 AND nlevel(parent.path) - nlevel(g.path) <= :level %s`, mq) |
There was a problem hiding this comment.
Is g.path @> parent.path AND nlevel(parent.path) - nlevel(g.path) >= 0 necessary, or we can use only g.path @> parent.path?
things/postgres/groups.go
Outdated
| mq = fmt.Sprintf("AND %s", mq) | ||
| } | ||
|
|
||
| q := fmt.Sprintf(`SELECT g.id, g.name, g.owner_id, g.parent_id, g.description, g.metadata, g.path, nlevel(g.path) as level FROM |
There was a problem hiding this comment.
Consider using simpler self join. You can try this query:
SELECT g.id, g.name, g.owner_id, g.parent_id, g.description, g.metadata, g.path, nlevel(g.path) as level
FROM thing_groups parent, thing_groups g
WHERE parent.id = :id AND g.path @> parent.path AND nlevel(parent.path) - nlevel(g.path) <= :level;It's a little simpler (and shorter) and possibly more performant.
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
|
|
||
| type groupRes struct { | ||
| id string | ||
| created bool |
|
|
||
| type groupRes struct { | ||
| id string | ||
| created bool |
There was a problem hiding this comment.
created is a time, not bool. Time of creation.
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
internal/groups/groups.go
Outdated
| Path string | ||
| Children []*Group | ||
| CreatedAt time.Time | ||
| UpdateAt time.Time |
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic mirko.teodorovic@gmail.com
Adding group of things