fix(http): /api/v2/orgs/:id/owners|members to 404 #15504

dearyhud · 2019-10-18T18:42:39Z

Closes #

Describe your proposed changes here.

CHANGELOG.md updated with a link to the PR (not the Issue)
Well-formatted commit messages
Rebased/mergeable
Tests pass
http/swagger.yml updated (if modified Go structs or API)
Documentation updated or issue created (provide link to issue/pr)
Signed CLA (if not already signed)

121watts

Swagger looks good to me

asalem1 · 2019-10-18T19:14:59Z

http/org_service.go

@@ -73,6 +73,24 @@ const (
 	organizationsIDLabelsIDPath      = "/api/v2/orgs/:id/labels/:lid"
 )

+func (h *OrgHandler) handleGetOrgCheck(w http.ResponseWriter, r *http.Request) interface{} {


love the helper function!

Could this just return a organization pointer?

bthesorceror · 2019-10-18T21:19:41Z

http/org_service.go

-	if err := encodeResponse(ctx, w, http.StatusOK, newOrgResponse(b)); err != nil {
+	ctx := r.Context()
+
+	org := organization.(*influxdb.Organization)


See comment above, this would remove the need to cast.

bthesorceror · 2019-10-18T21:21:44Z

http/user_resource_mapping_service.go

@@ -63,11 +63,16 @@ type MemberBackend struct {

 	UserResourceMappingService influxdb.UserResourceMappingService
 	UserService                influxdb.UserService
+	Precheck                   func(w http.ResponseWriter, r *http.Request) interface{}


Could this function type have a defined return type instead of interface{}?

bthesorceror

There seems to be a few places that could have types instead of interface{}

jsteenb2 · 2019-10-21T16:29:36Z

http/middleware.go

@@ -151,3 +151,17 @@ func (b *bodyEchoer) Read(p []byte) (int, error) {
 func (b *bodyEchoer) Close() error {
 	return b.rc.Close()
 }
+
+type middleware func(http.HandlerFunc) http.HandlerFunc


can we export this type here, and refactor the LoggignMW to use it as well?

jsteenb2

small grips, but overall looks good

jsteenb2 · 2019-10-21T16:30:23Z

http/org_service.go

+}
+
+func checkOrganziationExistsWrapper(handler *OrgHandler) func(http.HandlerFunc) http.HandlerFunc {
+	fn := func(next http.HandlerFunc) http.HandlerFunc {


👏 looking good, very foxused func here, I dig it 👍

jsteenb2 · 2019-10-21T16:32:08Z

http/middleware.go

+
+type middleware func(http.HandlerFunc) http.HandlerFunc
+
+func handlerFuncWithMiddlewares(h http.HandlerFunc, m ...middleware) http.HandlerFunc {


can you rename this to something shorter maybe? applyMW? this is a mouthful for something you'll be chaining. Its quite common to see MW the type for middleware and how its referenced in a lot of go pkgs

jsteenb2 · 2019-10-21T16:34:26Z

http/org_service.go

+	return nil
+}
+
+func checkOrganziationExistsWrapper(handler *OrgHandler) func(http.HandlerFunc) http.HandlerFunc {


go on and take advantage of the type you just created func organziationExists(handler *OrgHandler) Middleware {

one other thing here is that you are passing the org handler to the func this is wrapping, as a value instead of using pointer semantics. you want to stick with one or the other to be consistent here. I'd recommend using pointer semantics here as that's what we are sharing across these functional boundaries. Even better yet, just drop the func above and put it in the wrapper bit and be donezo with teh wrapper stuff lol

This fix ensures that the endpoints: - /api/v2/orgs/:id/owners - /api/v2/orgs/:id/members 404 when the organization resource does not exist

ensures that the endpoints: * /api/v2/orgs/:id/owners * /api/v2/orgs/:id/members 404 when the organization resource does not exist

dearyhud requested review from a team as code owners October 18, 2019 19:10

121watts approved these changes Oct 18, 2019

View reviewed changes

asalem1 reviewed Oct 18, 2019

View reviewed changes

asalem1 approved these changes Oct 18, 2019

View reviewed changes

dearyhud force-pushed the fix/http-orgs-members-owners-404 branch from 205b6bb to 2029873 Compare October 18, 2019 20:28

imogenkinsman requested review from a team and removed request for a team October 18, 2019 21:13

ghost requested review from bthesorceror and removed request for a team October 18, 2019 21:15

bthesorceror reviewed Oct 18, 2019

View reviewed changes

bthesorceror suggested changes Oct 18, 2019

View reviewed changes

jsteenb2 reviewed Oct 21, 2019

View reviewed changes

jsteenb2 approved these changes Oct 21, 2019

View reviewed changes

dearyhud requested a review from bthesorceror October 21, 2019 19:14

dearyhud force-pushed the fix/http-orgs-members-owners-404 branch 2 times, most recently from a2f6bb5 to 5ec5915 Compare October 21, 2019 19:49

dearyhud changed the title ~~fix:(http) orgs resource endpoints to 404 when org does not exist~~ fix: (http) orgs resource endpoints to 404 when org does not exist Oct 21, 2019

dearyhud changed the title ~~fix: (http) orgs resource endpoints to 404 when org does not exist~~ fix:(http)/api/v2/orgs/:id/{owners|members} to 404 Oct 21, 2019

dearyhud force-pushed the fix/http-orgs-members-owners-404 branch from 5ec5915 to 264adb7 Compare October 21, 2019 20:25

dearyhud changed the title ~~fix:(http)/api/v2/orgs/:id/{owners|members} to 404~~ fix(http): /api/v2/orgs/:id/owners|members to 404 Oct 21, 2019

bthesorceror approved these changes Oct 21, 2019

View reviewed changes

fix(http): /api/v2/orgs/:id/owners|members to 404

822e366

This fix ensures that the endpoints: - /api/v2/orgs/:id/owners - /api/v2/orgs/:id/members 404 when the organization resource does not exist

dearyhud force-pushed the fix/http-orgs-members-owners-404 branch from 264adb7 to 822e366 Compare October 21, 2019 20:38

dearyhud merged commit c04249e into master Oct 21, 2019

dearyhud deleted the fix/http-orgs-members-owners-404 branch October 21, 2019 21:37

asalem1 pushed a commit that referenced this pull request Oct 22, 2019

fix(http): /api/v2/orgs/:id/owners|members to 404 (#15504)

62e749b

ensures that the endpoints: * /api/v2/orgs/:id/owners * /api/v2/orgs/:id/members 404 when the organization resource does not exist

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(http): /api/v2/orgs/:id/owners|members to 404 #15504

fix(http): /api/v2/orgs/:id/owners|members to 404 #15504

dearyhud commented Oct 18, 2019 •

edited

Loading

121watts left a comment

asalem1 Oct 18, 2019

bthesorceror Oct 18, 2019

bthesorceror Oct 18, 2019

bthesorceror Oct 18, 2019

bthesorceror left a comment

jsteenb2 Oct 21, 2019

jsteenb2 left a comment

jsteenb2 Oct 21, 2019

jsteenb2 Oct 21, 2019

jsteenb2 Oct 21, 2019 •

edited

Loading


		type middleware func(http.HandlerFunc) http.HandlerFunc

		func handlerFuncWithMiddlewares(h http.HandlerFunc, m ...middleware) http.HandlerFunc {

fix(http): /api/v2/orgs/:id/owners|members to 404 #15504

fix(http): /api/v2/orgs/:id/owners|members to 404 #15504

Conversation

dearyhud commented Oct 18, 2019 • edited Loading

121watts left a comment

Choose a reason for hiding this comment

asalem1 Oct 18, 2019

Choose a reason for hiding this comment

bthesorceror Oct 18, 2019

Choose a reason for hiding this comment

bthesorceror Oct 18, 2019

Choose a reason for hiding this comment

bthesorceror Oct 18, 2019

Choose a reason for hiding this comment

bthesorceror left a comment

Choose a reason for hiding this comment

jsteenb2 Oct 21, 2019

Choose a reason for hiding this comment

jsteenb2 left a comment

Choose a reason for hiding this comment

jsteenb2 Oct 21, 2019

Choose a reason for hiding this comment

jsteenb2 Oct 21, 2019

Choose a reason for hiding this comment

jsteenb2 Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

dearyhud commented Oct 18, 2019 •

edited

Loading

jsteenb2 Oct 21, 2019 •

edited

Loading