warden/groups: add a PUT method to overwrite members & some refactoring #768

zepatrik · 2018-02-07T09:06:46Z

There was no way to overwrite all group members meaning just sending the actual value, instead one had to keep track of changes or remove all members to add the new ones again.
Now a http PUT method allows to set the members array.

closes #745

There was no way to overwrite all group members meaning just sending the actual value, instead one had to keep track of changes or remove all members to add the new ones again. Now a http PUT method allows to set the members array.

zepatrik · 2018-02-07T09:07:42Z

not quite ready to merge, just for review if I am doing it right

zepatrik · 2018-02-07T09:09:26Z

warden/group/handler.go

-
-	if err := json.NewDecoder(r.Body).Decode(&g); err != nil {
-		h.H.WriteError(w, r, errors.WithStack(err))
+	if err := h.checkRequest(r, h.PrefixResource(GroupsResource), "create"); err != nil {


isn't it better to always first check the request?

Yes, maybe I was using g here for some reason which was why it was decoded first, but as it stands now the order can be reversed´the way you did it.

zepatrik · 2018-02-07T09:10:36Z

warden/group/handler.go

-	if err := h.Manager.RemoveGroupMembers(id, m.Members); err != nil {
+	w.WriteHeader(http.StatusNoContent)
+}
+


missing comment

zepatrik · 2018-02-07T09:13:02Z

warden/group/manager_sql.go

+	return nil
+}
+
+func (m *SQLManager) applyInTransaction(requests ...func(tx *sqlx.Tx) error) error {


check if rollbacks are correct

zepatrik · 2018-02-07T09:15:46Z

shouldn't this function (and the next one) be non capital because it is not checking the request at all, it is just a helper right?

aeneasr · 2018-02-07T09:47:45Z

shouldn't this function (and the next one) be non capital because it is not checking the request at all, it is just a helper right?

I'm not sure right now, I think they need to be upper case for swagger docs to work. I'd have to check, otherwise all of the handlers can be lower case.

aeneasr

Looks pretty solid so far!

aeneasr · 2018-02-07T09:48:25Z

warden/group/handler.go

+	w.WriteHeader(http.StatusNoContent)
+}
+
+func (h *Handler) OverwriteGroupMembers(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {


I would probably rename it to "UpdateGroup"

I think I'm going for UpdateGroupMembers because the http PUT url is GroupsHandlerPath + "/:id/members" so only one group is affected, not all of them

aeneasr · 2018-02-07T09:50:01Z

warden/group/handler.go

-
-	if err := json.NewDecoder(r.Body).Decode(&g); err != nil {
-		h.H.WriteError(w, r, errors.WithStack(err))
+	if err := h.checkRequest(r, h.PrefixResource(GroupsResource), "create"); err != nil {


Yes, maybe I was using g here for some reason which was why it was decoded first, but as it stands now the order can be reversed´the way you did it.

aeneasr · 2018-02-07T09:50:44Z

warden/group/manager_sql.go

+	return nil
+}
+
+func (m *SQLManager) applyInTransaction(requests ...func(tx *sqlx.Tx) error) error {


requests are really transactions, right?

A transaction is a sequence of requests I would rather say.

How about executors?

aeneasr · 2018-02-07T09:51:29Z

warden/group/manager_sql.go

+
+	for _, req := range requests {
+		if err := req(tx); err != nil {
+			tx.Rollback()


If rollback fails, the error should be returned instead

if err := tx.Rollback(); err != nil { return err }

aeneasr · 2018-02-07T09:51:54Z

warden/group/manager_sql.go

@@ -183,3 +187,33 @@ func (m *SQLManager) ListGroups(limit, offset int) ([]Group, error) {

 	return groups, nil
 }
+
+func (m *SQLManager) OverwriteGroupMembers(group string, members []string) error {
+	if err := m.applyInTransaction(m.deleteGroup(group), m.createGroup(group), m.addGroupMembers(group, members)); err != nil {


this can be simplified to return m.ApplyIn...

aeneasr · 2018-02-07T09:52:40Z

warden/group/manager_sql.go

+	return func(tx *sqlx.Tx) error {
+		for _, subject := range subjects {
+			if _, err := tx.Exec(m.DB.Rebind("DELETE FROM hydra_warden_group_member WHERE member=? AND group_id=?"), subject, group); err != nil {
+				if err := tx.Rollback(); err != nil {


You have duplicate rollbacks here, let applyInTransaction handle rollbacks instead!

aeneasr · 2018-02-07T09:52:58Z

warden/group/manager_sql.go

+	return func(tx *sqlx.Tx) error {
+		for _, subject := range subjects {
+			if _, err := tx.Exec(m.DB.Rebind("INSERT INTO hydra_warden_group_member (group_id, member) VALUES (?, ?)"), group, subject); err != nil {
+				if err := tx.Rollback(); err != nil {


Rollback is already covered by applyInTransaction

aeneasr · 2018-02-07T09:53:48Z

warden/group/manager_sql.go

+func (m *SQLManager) createGroup(group string) func(tx *sqlx.Tx) error {
+	return func(tx *sqlx.Tx) error {
+		if _, err := tx.Exec(m.DB.Rebind("INSERT INTO hydra_warden_group (id) VALUES (?)"), group); err != nil {
+			return err


Normal errors do not have stack traces in go, to add a stack trace do return errors.WithStack(err)

Oh now I got it, I thought that errors.WithStack(err) just has to be called once to get the whole stacktrace.

Exactly, and it also needs to be called at the location which is the deepest in the stack trace and there only once - otherwise you'll overwrite the stack trace. This is a bit tricky unfortunately. The rule is that we wrap all errors coming from other libraries but don't wrap errors coming from our own. That way we ensure that traces are always as deep as possible in the stack.

So this should be actually without a stacktrace?

aeneasr · 2018-02-07T09:53:55Z

warden/group/manager_sql.go

+func (m *SQLManager) deleteGroup(id string) func(tx *sqlx.Tx) error {
+	return func(tx *sqlx.Tx) error {
+		if _, err := tx.Exec(m.DB.Rebind("DELETE FROM hydra_warden_group WHERE id=?"), id); err != nil {
+			return err


Normal errors do not have stack traces in go, to add a stack trace do return errors.WithStack(err)

aeneasr · 2018-02-07T09:54:03Z

warden/group/manager_sql.go

+				if err := tx.Rollback(); err != nil {
+					return err
+				}
+				return err


Normal errors do not have stack traces in go, to add a stack trace do return errors.WithStack(err)

zepatrik · 2018-02-15T23:14:31Z

ready for review @arekkas

aeneasr

Totally forgot about this one, have some questions. I'll check out the SQL implementation in more detail because the changes are a bit complex.

aeneasr · 2018-03-16T10:29:39Z

warden/group/handler.go

+//       oauth2: hydra.warden.groups
+//
+//     Responses:
+//       204: emptyResponse


PUT requests typically don't return 204 responses but instead 200 + body. 204 is typically used for DELETE responses to aknowledge that the resource is gone

I may not have changed everything correctly when copying the comments

It's also in the response, see w.WriteHeader(http.StatusNoContent)

aeneasr · 2018-03-16T10:30:51Z

warden/group/manager_sql.go

+	return func(tx *sqlx.Tx) error {
+		_, err := tx.Exec(m.DB.Rebind("INSERT INTO hydra_warden_group (id) VALUES (?)"), group)
+
+		return errors.WithStack(err)


Not sure if this works for nil values? So if error is nil, does this return nil as well or not?

https://godoc.org/github.com/pkg/errors#WithStack
I looked it up it works as expected

Absolutely, it returns nil in that case: https://github.com/pkg/errors/blob/master/errors.go#L145

aeneasr · 2018-05-20T08:57:06Z

This is now tracked as ory/keto#10

* transfer UpdateRoleMembers from ory/hydra#768 to keto * fix tests by using right http method & correcting sql request * Change behavior to overwrite the whole role instead of just the members. + small sql migration fix

zepatrik commented Feb 7, 2018

View reviewed changes

aeneasr reviewed Feb 7, 2018

View reviewed changes

zepatrik added 3 commits February 12, 2018 18:31

unstaged

988e75f

Merge branch 'master' into add-replace-warden-groups-members

3bc3e02

Finish up PR, fixing error handling mostly

b69333d

aeneasr self-requested a review February 17, 2018 08:54

Merge branch 'master' into add-replace-warden-groups-members

50b2eb3

aeneasr reviewed Mar 16, 2018

View reviewed changes

aeneasr changed the base branch from master to 0.11 April 8, 2018 21:49

aeneasr changed the base branch from 0.11 to master April 8, 2018 21:50

aeneasr mentioned this pull request May 20, 2018

roles: add a PUT method to overwrite members ory/keto#10

Closed

aeneasr closed this May 20, 2018

zepatrik mentioned this pull request May 22, 2018

transfer UpdateRoleMembers ory/keto#14

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

warden/groups: add a PUT method to overwrite members & some refactoring #768

warden/groups: add a PUT method to overwrite members & some refactoring #768

zepatrik commented Feb 7, 2018 •

edited

Loading

zepatrik commented Feb 7, 2018

zepatrik Feb 7, 2018

aeneasr Feb 7, 2018

zepatrik Feb 7, 2018

zepatrik Feb 7, 2018

zepatrik commented Feb 7, 2018

aeneasr commented Feb 7, 2018

aeneasr left a comment

aeneasr Feb 7, 2018

zepatrik Feb 15, 2018

aeneasr Feb 7, 2018

aeneasr Feb 7, 2018

zepatrik Feb 7, 2018

aeneasr Feb 9, 2018

aeneasr Feb 7, 2018

aeneasr Feb 7, 2018

aeneasr Feb 7, 2018

aeneasr Feb 7, 2018

aeneasr Feb 7, 2018

zepatrik Feb 7, 2018

aeneasr Feb 9, 2018

zepatrik Feb 15, 2018

aeneasr Feb 7, 2018

aeneasr Feb 7, 2018

zepatrik commented Feb 15, 2018

aeneasr left a comment

aeneasr Mar 16, 2018

zepatrik Mar 16, 2018

aeneasr Mar 16, 2018

aeneasr Mar 16, 2018

zepatrik Mar 16, 2018

aeneasr Mar 16, 2018

aeneasr commented May 20, 2018

warden/groups: add a PUT method to overwrite members & some refactoring #768

warden/groups: add a PUT method to overwrite members & some refactoring #768

Conversation

zepatrik commented Feb 7, 2018 • edited Loading

zepatrik commented Feb 7, 2018

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

zepatrik commented Feb 7, 2018

aeneasr commented Feb 7, 2018

aeneasr 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

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

zepatrik commented Feb 15, 2018

aeneasr 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

aeneasr commented May 20, 2018

zepatrik commented Feb 7, 2018 •

edited

Loading