8000 MF-983 - Add HTTP query param to connections list endpoints to fetch disconnected Things or Channels by manuio · Pull Request #1217 · absmach/supermq · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

MF-983 - Add HTTP query param to connections list endpoints to fetch disconnected Things or Channels #1217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
Jul 26, 2020
Merged

MF-983 - Add HTTP query param to connections list endpoints to fetch disconnected Things or Channels #1217

merged 11 commits into from
Jul 26, 2020

Conversation

manuio
Copy link
Contributor
@manuio manuio commented Jul 10, 2020

Signed-off-by: Manuel Imperiale manuel.imperiale@gmail.com

Resolves #983

@manuio
MF-983 - Add HTTP query param to connections list endpoints to fetch …
2bd19ef 
…non
8000
 connected Things or Channels

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio manuio requested a review from a team as a code owner July 10, 2020 15:27
mteodor
mteodor suggested changes Jul 13, 2020
View reviewed changes
Copy link
Contributor
@mteodor mteodor left a comment

Choose a reason for hiding this comment

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

swagger needs update

@dborovcanin
Copy link
Collaborator

@manuio Please update PR, CI is failing and the branch is out of date.

@manuio manuio changed the title MF-983 - Add HTTP query param to connections list endpoints to fetch non connected Things or Channels MF-983 - Add HTTP query param to connections list endpoints to fetch disconnectedThings or Channels Jul 14, 2020
@manuio manuio changed the title MF-983 - Add HTTP query param to connections list endpoints to fetch disconnectedThings or Channels MF-983 - Add HTTP query param to connections list endpoints to fetch disconnected Things or Channels Jul 14, 2020
@codecov-commenter
Copy link
codecov-commenter commented Jul 14, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@b910244). Click here to learn what that means.
The diff coverage is 92.59%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1217   +/-   ##
=========================================
  Coverage          ?   76.38%           
=========================================
  Files             ?      106           
  Lines             ?     6963           
  Branches          ?        0           
=========================================
  Hits              ?     5319           
  Misses            ?     1259           
  Partials          ?      385
Impacted Files Coverage Δ
pkg/sdk/go/sdk.go 96.55% <ø> (ø)
things/api/things/http/requests.go 89.13% <ø> (ø)
things/api/things/http/transport.go 88.52% <75.00%> (ø)
things/postgres/channels.go 81.52% <97.05%> (ø)
things/postgres/things.go 80.80% <97.22%> (ø)
pkg/sdk/go/channels.go 58.67% <100.00%> (ø)
pkg/sdk/go/things.go 59.45% <100.00%> (ø)
things/api/things/http/endpoint.go 98.59% <100.00%> (ø)
things/redis/streams.go 92.30% <100.00%> (ø)
things/service.go 86.33% <100.00%> (ø)

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 b910244...992795a. Read the comment docs.

@manuio manuio closed this Jul 15, 2020
@manuio manuio reopened this Jul 15, 2020
@nmarcetic
Copy link
Collaborator

@manuio IMHO connected bool is much better than disconnected. If we have this query param, by default it should return all (booth conn/disconn) and if you pass query connected bool then filter and return filtered result.

@manuio
Copy link
Contributor Author
manuio commented Jul 16, 2020

@manuio IMHO connected bool is much better than disconnected. If we have this query param, by default it should return all (booth conn/disconn) and if you pass query connected bool then filter and return filtered result.

@nmarcetic do you mean to return connected by default? And disconnected if connected = false? If we return connected and disconnected we are returning all things :) I wanted a flag where the default value is false to simplify the code. Maybe we can find a better naming.

@nmarcetic
Copy link
Collaborator
nmarcetic commented Jul 16, 2020 •
edited
Loading

@manuio IMHO connected bool is much better than disconnected. If we have this query param, by default it should return all (booth conn/disconn) and if you pass query connected bool then filter and return filtered result.

@nmarcetic do you mean to return connected by default? And disconnected if connected = false? If we return connected and disconnected we are returning all things :) I wanted a flag where the default value is false to simplify the code. Maybe we can find a better naming.

@manuio Yes, return all things owned by user by default. then filter it with a query like &connected=true/false if you want. We have pagination. This would be the cleanest way.

@manuio
Copy link
Contributor Author
manuio commented Jul 16, 2020

@manuio IMHO connected bool is much better than disconnected. If we have this query param, by default it should return all (booth conn/disconn) and if you pass query connected bool then filter and return filtered result.

@nmarcetic do you mean to return connected by default? And disconnected if connected = false? If we return connected and disconnected we are returning all things :) I wanted a flag where the default value is false to simplify the code. Maybe we can find a better naming.

@manuio Yes, return all things owned by user by default. then filter it with a query like &connected=true/false if you want. We have pagination. This would be the cleanest way.

@nmarcetic Why would I specify a channel or thing ID (things/<thing_id>/channels or channels/<channel_id>/things) to fetch all existent things or channels? It's not really the same feature. I would keep it as it is but find a better naming. Maybe off instead of disconnected?

@nmarcetic
Copy link
Collaborator

things/<thing_id>/channels or channels/<channel_id>/things

@manuio Why would you specify ID (things/<thing_id>/channels or channels/<channel_id>/things) to fetch something that does not belong here :) like disconnected things/channels? I think something is definitely wrong with the design here.

Ok, keep it as is, I think connected bool is best (because of our internal terminology of connecting things to channel ).
We can discuss about the design later with all @mainflux/maintainers

dborovcanin
dborovcanin requested changes Jul 21, 2020
View reviewed changes
Copy link
Collaborator
@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

I agree with @nmarcetic that using connected param is better than disconnected.

things/api/things/http/transport.go Outdated
Comment on lines 26 to 30
offsetKey = "offset"
limitKey = "limit"
nameKey = "name"
metadataKey = "metadata"
disconKey = "disconnected"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer the old naming.

things/postgres/things.go Outdated
LIMIT :limit
OFFSET :offset;`
FROM things th
INNER JOIN connections co

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using conn instead of co is less ambiguous.

things/postgres/things.go Outdated
ORDER BY th.id
LIMIT :limit
OFFSET :offset;`
FROM things th
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use switch here, it's cleaner than using if.

mteodor
mteodor reviewed Jul 21, 2020
View reviewed changes
things/postgres/channels.go Outdated
LIMIT :limit
OFFSET :offset`
q := `SELECT id, name, metadata FROM channels ch
INNER JOIN connections co ON ch.id = co.channel_id
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesnt look properly aligned

manuio added 3 commits July 21, 2020 16:45
@manuio
Fix reviews
70007ec 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio
Merge branch 'master' of https://github.com/mainflux/mainflux into co…
cf0df4d 
…nnected
@manuio
Fix typos and add Swagger
74e0dd8 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
dborovcanin
dborovcanin requested changes Jul 21, 2020
View reviewed changes
things/mocks/channels.go Outdated
channels = append(channels, co)
}
}
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you avoid using else?

things/postgres/things.go Outdated
ORDER BY th.id
LIMIT :limit
OFFSET :offset;`
var q string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same remark for indentation.

manuio added 5 commits July 21, 2020 17:15
@manuio
Update SDK and CLI
5f5d446 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio
Fix typo
6f8f8bf 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio
Simplify queries
28863fa 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio
Fix reviews
c900403 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio
Fix tabulation
ec76600 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
mteodor
mteodor 9E88 previously approved these changes Jul 22, 2020
View reviewed changes
dborovcanin
dborovcanin requested changes Jul 22, 2020
View reviewed changes
things/postgres/channels.go Outdated
ORDER BY ch.id
LIMIT :limit
OFFSET :offset`
var q string
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use var q, qc string.

things/postgres/things.go Outdated
ORDER BY th.id
LIMIT :limit
OFFSET :offset;`
var q string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same remark for indentation.

@manuio
Fix reviews
992795a 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
drasko
drasko reviewed Jul 26, 2020
View reviewed changes
Copy link
Contributor
@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

@drasko drasko merged commit 9334568 into absmach:master Jul 26, 2020
manuio added a commit that referenced this pull request Oct 12, 2020
@manuio
MF-983 - Add HTTP query param to connections list endpoints to fetch …
b6ecced 
…disconnected Things or Channels (#1217)

* MF-983 - Add HTTP query param to connections list endpoints to fetch non connected Things or Channels

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Fix reviews

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Fix typos and add Swagger

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Update SDK and CLI

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Fix typo

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Simplify queries

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Fix reviews

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Fix tabulation

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Fix reviews

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Fix reviews

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio manuio deleted the connected branch February 23, 2022 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drasko drasko drasko left review comments

@mteodor mteodor mteodor approved these changes

@dborovcanin dborovcanin dborovcanin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fetch not connected Things or Channels
6 participants
@manuio @dborovcanin @codecov-commenter @nmarcetic @drasko @mteodor
0