warden/groups: add a PUT method to overwrite members & some refactoring by zepatrik · Pull Request #768 · ory/hydra

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

-
-	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

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


missing comment

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

+	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

+	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

-
-	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

+	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

+
+	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

 }
+
+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

+	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

+	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

+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

+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

+				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

+//       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

+	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

Conversation

zepatrik commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zepatrik commented Feb 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zepatrik commented Feb 7, 2018

Uh oh!

aeneasr commented Feb 7, 2018

Uh oh!

aeneasr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zepatrik commented Feb 15, 2018

Uh oh!

aeneasr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeneasr commented May 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zepatrik commented Feb 7, 2018 •

edited

Loading