MF-906 - Change single creation endpoints to use bulk service calls #927

nwneisen · 2019-10-31T16:56:05Z

What does this do?

Changes the single things and channels creation endpoints to use bulk service calls. All code for single creation is removed and tests updated to use bulk creation calls.

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

Resolves #906

List any changes that modify/break current functionality

Single creation endpoints behave the same from a user's perspective.

Have you included tests for your changes?

Tests were updated to use bulk creation methods.

Did you document any new/modified functionality?

NA

Notes

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

things/postgres/things_test.go

things/postgres/channels_test.go

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

nmarcetic

@nwneisen I would prefer to keep method naming simple and clean by removing Bulk word :) So I would prefer Create() vs BulkCreate() etc... even if method accept a slice of things/channels
Word bulk adds complexity and confusion IMHO, keeps things simple as they are and documenting interface (good comments) is perfect.
I am approving PR, good job really! This is just my personal feeling about naming, which you can accept or not :) It's your call.

drasko · 2019-11-01T16:45:20Z

@nwneisen you have a message from the SemaphoreCI:

ok  	github.com/mainflux/mainflux/things/api/things/http	3.091s	coverage: 95.3% of statements
?   	github.com/mainflux/mainflux/things/mocks	[no test files]
# github.com/mainflux/mainflux/things/postgres_test [github.com/mainflux/mainflux/things/postgres.test]
things/postgres/channels_test.go:624:22: thingRepo.SaveSave undefined (type things.ThingRepository has no field or method SaveSave)
FAIL	github.com/mainflux/mainflux/things/postgres [build failed]
FAIL```

;)

things/service.go

nwneisen · 2019-11-01T18:42:32Z

@nmarcetic I agree completely. When I do a search for remaining "bulk" word usages, I only see it still used with the single vs bulk endpoints. Let me know if there is somewhere I am missing.

I will say that I am still in favor of depreciating this single endpoint as well. If only one thing/channel is needed, then a user would simply send an array of size one. This would make it so the user only needs to understand one method to create a thing/channel.

drasko · 2019-11-01T18:46:36Z

I will say that I am still in favor of depreciating this single endpoint as well. If only one thing/channel is needed, then a user would simply send an array of size one.

@nwneisen exactly, this would be the best approach.

nwneisen · 2019-11-01T19:01:05Z

@manuio

@drasko Making sure everyone is on board as this was my previous suggestion.

It's also worth noting that with the exception of the blank payload, the bulk provisioning could be used to replace creating a single thing. This would be a breaking change but might be worth considering in the future to reduce maintenance. If sending a blank payload to create a thing is desired, then this could also be added to the bulk provisioning endpoint.

The single endpoint will create a thing/channel when is passed an empty payload. It also returns the created thing's/channel's ID in the location header.

drasko · 2019-11-01T19:10:53Z

The single endpoint will create a thing/channel when is passed an empty payload. It also returns the created thing's/channel's ID in the location header.

I am interested what would the others say, but personally this looks OK for me - passing an empty call means "create one thing without metadata", while passing a call with array of length 1 in a parameter might mean "create one thing with this meatadata".

I am however thinking that we might be interested in creating for example 100 things without metadata, and I do not know how we can do it with this API...

@nmarcetic @anovakovic01 @dusanb94 ^

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

codecov-io · 2019-11-01T19:35:55Z

Codecov Report

Merging #927 into master will decrease coverage by 0.89%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #927     +/-   ##
=========================================
- Coverage   83.85%   82.95%   -0.9%     
=========================================
  Files          75       75             
  Lines        5667     5298    -369     
=========================================
- Hits         4752     4395    -357     
+ Misses        655      643     -12     
  Partials      260      260

Impacted Files	Coverage Δ
things/redis/streams.go	`92.18% <100%> (-0.99%)`	⬇️
things/api/things/http/endpoint.go	`98.54% <100%> (ø)`	⬆️
things/service.go	`90.44% <100%> (-1.56%)`	⬇️
things/postgres/channels.go	`81.3% <100%> (-4.1%)`	⬇️
things/api/things/http/requests.go	`87.5% <100%> (ø)`	⬆️
things/api/things/http/transport.go	`93.77% <100%> (ø)`	⬆️
things/api/things/http/responses.go	`97.56% <100%> (+0.06%)`	⬆️
things/postgres/things.go	`79.62% <100%> (-7.31%)`	⬇️
... and 2 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 28a176a...6301379. Read the comment docs.

nwneisen · 2019-11-01T19:46:11Z

@drasko Using bulk create, an empty JSON payload ("" or "[]") will not create the thing/channel. A payload with an empty JSON object ("[{}]") will. To create 100 things without metadata, a JSON array with 100 empty objects could be used.

drasko · 2019-11-01T20:25:39Z

@nwneisen yep, I see.

It looks OK to me, I think there is no particular need to complicate the code and add other options - especially that most of the time interaction with the REST API will be via CLI or SDK (meaning that CLI and SDK must be changed as well in this PR, if the API changes).

Never the less, I'd like to hear the opinion of other maintainers.

things/things.go

anovakovic01 · 2019-11-04T10:38:35Z

@drasko I don't think that we need an endpoint for the creation of a single thing.

nmarcetic · 2019-11-04T12:36:08Z

@drasko I don't think that we need an endpoint for the creation of a single thing.

I agree with @anovakovic01
@nwneisen You can merge those two endpoints and keep just on which will accept a slice of things/channels. This will keep things simpler but will require changes in SDK, UI also, etc... its breaking change. Maybe is better idea to keep it like this, for now, 2 endpoints like you did, then in the next release merge it and make an announcement.

What do you think @nwneisen ?

dborovcanin · 2019-11-04T15:33:59Z

I agree with @nmarcetic. @nwneisen Please resolve remaining remark (variadic Save), so that we can approve and merge.

nwneisen · 2019-11-04T16:05:42Z

@nmarcetic I was thinking leave the single endpoint for now but look at adding a depreciation warning and/or announcement for when it would be completely removed.

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

nmarcetic · 2019-11-04T16:57:55Z

@nmarcetic I was thinking leave the single endpoint for now but look at adding a deprecation warning and/or announcement for when it would be completely removed.

@nwneisen c I was thinking to leave the single endpoint for now Now is not a single endpoint, there is 2 endpoints POST on /things and things/bulk
Let's leave it for now and as you said, make a deprecation warning and/or announcement and remove it in the next milestone.

nwneisen · 2019-11-04T17:00:43Z

@nmarcetic

I was thinking leave the single endpoint for now

I was thinking leave the single creation endpoint (/things) for now*

drasko · 2019-11-04T17:29:14Z

@nmarcetic @nwneisen can we open an issue for this merging of endpoints not to forget?

drasko

LGTM

nwneisen · 2019-11-04T17:41:43Z

@drasko @nmarcetic I will add the depreciation warning and place a warning in the docs before this PR gets merged

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

anovakovic01

LGTM!

drasko

LGTM

drasko · 2019-11-04T20:16:20Z

Thanks @nwneisen, again a great contribution!

I like PRs that remove code :).

…927) * Change single endpoints to use bulk creation Signed-off-by: Nick Neisen <nwneisen@gmail.com> * Remove single creation from thing's service Signed-off-by: Nick Neisen <nwneisen@gmail.com> * Remove single save from thing's postgres Signed-off-by: Nick Neisen <nwneisen@gmail.com> * Change BulkSave to Save Signed-off-by: Nick Neisen <nwneisen@gmail.com> * Change service calls to use variadic parameters Signed-off-by: Nick Neisen <nwneisen@gmail.com> * Change postgres things to use variadic functions Signed-off-by: Nick Neisen <nwneisen@gmail.com> * Add depreciation warnings Signed-off-by: Nick Neisen <nwneisen@gmail.com>

nwneisen added 3 commits October 31, 2019 10:06

Change single endpoints to use bulk creation

d84340d

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

Remove single creation from thing's service

108ac06

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

Remove single save from thing's postgres

91ff824

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

nwneisen requested a review from a team as a code owner October 31, 2019 16:56

Merge branch 'master' into MF-906

8112835

drasko requested changes Nov 1, 2019

View reviewed changes

things/postgres/things_test.go Outdated Show resolved Hide resolved

things/postgres/channels_test.go Outdated Show resolved Hide resolved

Change BulkSave to Save

7485f37

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

nmarcetic reviewed Nov 1, 2019

View reviewed changes

nmarcetic previously approved these changes Nov 1, 2019

View reviewed changes

anovakovic01 reviewed Nov 1, 2019

View reviewed changes

things/service.go Outdated Show resolved Hide resolved

Change service calls to use variadic parameters

9e34098

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

nwneisen dismissed nmarcetic’s stale review via 9e34098 November 1, 2019 19:28

anovakovic01 suggested changes Nov 4, 2019

View reviewed changes

things/things.go Outdated Show resolved Hide resolved

Change postgres things to use variadic functions

aedbbaf

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

nmarcetic previously approved these changes Nov 4, 2019

View reviewed changes

drasko previously approved these changes Nov 4, 2019

View reviewed changes

Add depreciation warnings

6301379

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

nwneisen dismissed stale reviews from drasko and nmarcetic via 6301379 November 4, 2019 17:50

nwneisen mentioned this pull request Nov 4, 2019

Merge single and bulk endpoints #934

Open

anovakovic01 approved these changes Nov 4, 2019

View reviewed changes

drasko approved these changes Nov 4, 2019

View reviewed changes

drasko merged commit 1a31ecd into absmach:master Nov 4, 2019

nwneisen deleted the MF-906 branch November 25, 2019 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF-906 - Change single creation endpoints to use bulk service calls #927

MF-906 - Change single creation endpoints to use bulk service calls #927

nwneisen commented Oct 31, 2019

nmarcetic left a comment •

edited

Loading

drasko commented Nov 1, 2019

nwneisen commented Nov 1, 2019

drasko commented Nov 1, 2019

nwneisen commented Nov 1, 2019

drasko commented Nov 1, 2019

codecov-io commented Nov 1, 2019 •

edited

Loading

nwneisen commented Nov 1, 2019 •

edited

Loading

drasko commented Nov 1, 2019

anovakovic01 commented Nov 4, 2019 •

edited

Loading

nmarcetic commented Nov 4, 2019

dborovcanin commented Nov 4, 2019

nwneisen commented Nov 4, 2019

nmarcetic commented Nov 4, 2019

nwneisen commented Nov 4, 2019 •

edited

Loading

drasko commented Nov 4, 2019 •

edited

Loading

drasko left a comment

nwneisen commented Nov 4, 2019 •

edited

Loading

anovakovic01 left a comment

drasko left a comment

drasko commented Nov 4, 2019

MF-906 - Change single creation endpoints to use bulk service calls #927

MF-906 - Change single creation endpoints to use bulk service calls #927

Conversation

nwneisen commented Oct 31, 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

nmarcetic left a comment • edited Loading

Choose a reason for hiding this comment

drasko commented Nov 1, 2019

nwneisen commented Nov 1, 2019

drasko commented Nov 1, 2019

nwneisen commented Nov 1, 2019

drasko commented Nov 1, 2019

codecov-io commented Nov 1, 2019 • edited Loading

Codecov Report

nwneisen commented Nov 1, 2019 • edited Loading

drasko commented Nov 1, 2019

anovakovic01 commented Nov 4, 2019 • edited Loading

nmarcetic commented Nov 4, 2019

dborovcanin commented Nov 4, 2019

nwneisen commented Nov 4, 2019

nmarcetic commented Nov 4, 2019

nwneisen commented Nov 4, 2019 • edited Loading

drasko commented Nov 4, 2019 • edited Loading

drasko left a comment

Choose a reason for hiding this comment

nwneisen commented Nov 4, 2019 • edited Loading

anovakovic01 left a comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

drasko commented Nov 4, 2019

nmarcetic left a comment •

edited

Loading

codecov-io commented Nov 1, 2019 •

edited

Loading

nwneisen commented Nov 1, 2019 •

edited

Loading

anovakovic01 commented Nov 4, 2019 •

edited

Loading

nwneisen commented Nov 4, 2019 •

edited

Loading

drasko commented Nov 4, 2019 •

edited

Loading

nwneisen commented Nov 4, 2019 •

edited

Loading