feat: implement partial client updates #2411

mattbonnell · 2021-03-19T16:26:58Z

Related issue

#2385

Proposed changes

Adds a PATCH handler to the client admin API.
Patch implemented as JSON Patch as per Decide RESTful PATCH strategy meta#2 using https://github.com/evanphx/json-patch

Checklist

I have read the contributing guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further comments

mattbonnell · 2021-03-19T16:28:10Z

wow, git made such a strange diff :) probably easiest to just look at the client/handler.go file directly

mattbonnell · 2021-03-19T16:54:17Z

getting the following error when I try to regenerate the sdk:

❯ make sdk
GOBIN=/Users/mattbonnell/dev/go/github.com/ory/hydra/.bin/ go install  github.com/ory/cli
# github.com/ory/jsonschema/v3/httploader
../../../../../go/pkg/mod/github.com/ory/jsonschema/v3@v3.0.1/httploader/httploader.go:27:15: undefined: httpx.NewResilientClientLatencyToleranceMedium
# github.com/ory/cli/cmd/dev/newsletter
../../../../../go/pkg/mod/github.com/ory/cli@v0.0.35/cmd/dev/newsletter/pkg.go:86:12: undefined: httpx.NewResilientClientLatencyToleranceMedium
make: *** [.bin/cli] Error 2

aeneasr

This looks pretty much like what I imagined! We obviously need a few tests and so on :) But I think this is almost good as is

aeneasr · 2021-03-23T09:27:31Z

client/handler.go

 	"net/http"
 	"time"

+	jsonpatch "github.com/evanphx/json-patch"


Is this the best library for the job? What alternatives do we have?

The JSON Patch site gives two options for Go implementations, this and another library, https://github.com/mattbaird/jsonpatch. Between the two, this one has more stars (551 vs 89) and more contributors (41 vs 8). This one was also updated more recently (January vs last August). Additionally, this library also supports Merge Patch, which was indicated as a potential alternative in ory/meta#2.

For these reasons, I felt this was the better choice between the two.

Perfect, thank you!

aeneasr · 2021-03-23T09:29:03Z

client/handler.go

+	original, err := json.Marshal(c)
+	if err != nil {
+		h.r.Writer().WriteError(w, r, err)
+		return
+	}
+	modified, err := patch.Apply(original)
+	if err != nil {
+		h.r.Writer().WriteError(w, r, err)
+		return
+	}
+	if err := json.Unmarshal(modified, c); err != nil {
 		h.r.Writer().WriteError(w, r, err)
 		return
 	}


I think we could wrap this, including DecodePatch, in a helper function in x and add a few test there? Alternatively we could also add this to ory/x/decoderx

I think that would be a good idea! Will investigate this.

Awesome, thank you :)

done! defined a helper in x/json.go

aeneasr · 2021-03-24T14:27:46Z

getting the following error when I try to regenerate the sdk:

You can bum ory/cli to a newer version which should fix that!

mattbonnell · 2021-03-24T14:40:52Z

getting the following error when I try to regenerate the sdk:

You can bum ory/cli to a newer version which should fix that!

Hey, can you help me understand the make sdk rule? The first command is:
swagger generate spec -m -o ./spec/api.json -x internal/httpclient -x gopkg.in/square/go-jose.v2

Isn't internal/httpclient the swagger-generated client code? I think I'm missing something here, this seems circular. How can we go about adding definitions to the swagger spec in this case? Additionally, it seems the sdk rule doesn't consider changes made in the client package.

I had added the PatchRequest and PatchDocument definitions to api.json, but this would overwrite them and then I get some other errors. If I remove this first line, then the rule runs as expected and I end up getting the new generated code in internal/httpclient (check the files which this PR adds to internal/httpclient).

The PR is failing in CI currently at this sdk generation step.

aeneasr · 2021-03-24T15:23:52Z

I'll look into this now

aeneasr · 2021-03-24T15:29:47Z

make sdk works fine for me but fails with:

The swagger spec at "./spec/api.json" is invalid against swagger specification 2.0.
See errors below:
- "updateOAuth2Client" is defined 2 times

because

// swagger:route PATCH /clients/{id} admin updateOAuth2Client

and

// swagger:route PUT /clients/{id} admin updateOAuth2Client

share the operation id. So one should be update and the other maybe patchOAuth2Client?

mattbonnell · 2021-03-24T16:13:48Z

spec/api.json

+        "schemes": ["http", "https"],
+        "tags": ["admin"],
+        "summary": "Patch an OAuth 2.0 Client",
+        "operationId": "patchOAuth2Client",


@aeneasr following up from your last comment:

that's the thing though, it is patchOAuth2Client as I've defined it, but it seems when that swagger generate command is run, it replaces spec/api.json and in the replaced version it has two updateOAuth2Clients defined for some reason. If you eliminate that first generate command, everything works as expected.

ohhh, i see. i think i was doing things wrong. hang on.

ok. think i've fixed it now.

mattbonnell · 2021-03-24T17:47:57Z

Hm, generate step failing in pipeline with

#!/bin/bash -eo pipefail
swagger generate spec -m -o spec/api.json $(printf -- " -x %s" internal/httpclient -x gopkg.in/square/go-jose.v2)
expected argument for flag `-x, --exclude', but got option `-x'

aeneasr · 2021-03-29T08:40:58Z

LMK when good for another 👀

mattbonnell · 2021-03-29T12:00:52Z

LMK when good for another 👀

hey, yes ready, looks like all checks are passing

edit: pulled in latest changes from master and there seems to be a vulnerability in the version of protobuf being used, causing ci/circleci:test to fail

zepatrik · 2021-03-31T08:53:25Z

I opened #2434 to fix the nancy warning

aeneasr · 2021-04-01T18:32:54Z

client/handler.go

+		return
+	}
+
+	if err := x.ApplyJSONPatch(patchJSON, c); err != nil {


It shouldn't be allowed to update the ID of the client as that is immutable iirc!

Good catch, will fix

…hydra into feat/partial-client-updates

aeneasr

Awesome! This looks pretty much good to go, but I think we need another test for the handler which also includes the ID behavior test! Another option would be to add a „deny list“ to the JSON PATCH helper which returns an error if a specific path is updated (e.g. ID). This would make it more general purpose and easier to test :)

mattbonnell · 2021-04-04T16:24:09Z

Awesome! This looks pretty much good to go, but I think we need another test for the handler which also includes the ID behavior test! Another option would be to add a „deny list“ to the JSON PATCH helper which returns an error if a specific path is updated (e.g. ID). This would make it more general purpose and easier to test :)

done!

client/handler.go

…hydra into feat/partial-client-updates

aeneasr · 2021-04-05T12:54:19Z

Thank you! Appreciate your contributions as always :)

aeneasr reviewed Mar 23, 2021

View reviewed changes

mattbonnell force-pushed the feat/partial-client-updates branch 2 times, most recently from 91669b5 to 90081b5 Compare March 24, 2021 13:56

mattbonnell marked this pull request as ready for review March 24, 2021 14:01

mattbonnell force-pushed the feat/partial-client-updates branch from 4a5221c to e2f4cba Compare March 24, 2021 14:50

mattbonnell commented Mar 24, 2021

View reviewed changes

mattbonnell added 6 commits March 25, 2021 07:46

implement client PATCH handler

077d1cc

add patch to swagger spec

fc89781

define ApplyJSONPatch helper, add tests

8507b59

regenerate sdk

7228d3d

fix swagger config

774425b

add patch models

ee2b8c4

mattbonnell force-pushed the feat/partial-client-updates branch from 8494cf0 to ee2b8c4 Compare March 25, 2021 11:46

mattbonnell added 2 commits March 29, 2021 08:01

Merge branch 'master' into feat/partial-client-updates

5d1f399

Merge branch 'master' into feat/partial-client-updates

1a44da8

aeneasr self-requested a review April 1, 2021 18:31

aeneasr reviewed Apr 1, 2021

View reviewed changes

mattbonnell added 2 commits April 2, 2021 11:51

prevent changes to the inner id

7278fc4

Merge branch 'feat/partial-client-updates' of github.com:mattbonnell/…

c5bb80b

…hydra into feat/partial-client-updates

aeneasr reviewed Apr 4, 2021

View reviewed changes

add denyPaths to ApplyJSONPatch and test cases

df3f973

aeneasr added 3 commits April 5, 2021 10:37

Merge branch 'master' into feat/partial-client-updates

90ecc59

Merge branch 'master' into feat/partial-client-updates

7134988

chore: format

d2862c9

aeneasr reviewed Apr 5, 2021

View reviewed changes

client/handler.go Show resolved Hide resolved

mattbonnell added 4 commits April 5, 2021 08:10

add handler tests

8de4399

Merge branch 'master' into feat/partial-client-updates

9ddf0d6

fix patch tests

71d3f2f

Merge branch 'feat/partial-client-updates' of github.com:mattbonnell/…

6905b3c

…hydra into feat/partial-client-updates

aeneasr approved these changes Apr 5, 2021

View reviewed changes

aeneasr merged commit 540c89d into ory:master Apr 5, 2021

mattbonnell deleted the feat/partial-client-updates branch April 5, 2021 12:58

aeneasr mentioned this pull request Apr 27, 2021

Decide RESTful PATCH strategy ory/meta#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement partial client updates #2411

feat: implement partial client updates #2411

mattbonnell commented Mar 19, 2021 •

edited

Loading

mattbonnell commented Mar 19, 2021

mattbonnell commented Mar 19, 2021

aeneasr left a comment

aeneasr Mar 23, 2021

mattbonnell Mar 23, 2021 •

edited

Loading

aeneasr Mar 24, 2021

aeneasr Mar 23, 2021

mattbonnell Mar 23, 2021

aeneasr Mar 24, 2021

mattbonnell Mar 24, 2021

aeneasr commented Mar 24, 2021

mattbonnell commented Mar 24, 2021

aeneasr commented Mar 24, 2021

aeneasr commented Mar 24, 2021

mattbonnell Mar 24, 2021 •

edited

Loading

mattbonnell Mar 24, 2021

mattbonnell Mar 24, 2021

mattbonnell commented Mar 24, 2021

aeneasr commented Mar 29, 2021

mattbonnell commented Mar 29, 2021 •

edited

Loading

zepatrik commented Mar 31, 2021

aeneasr Apr 1, 2021

mattbonnell Apr 2, 2021

mattbonnell Apr 2, 2021

aeneasr left a comment

mattbonnell commented Apr 4, 2021

aeneasr commented Apr 5, 2021

feat: implement partial client updates #2411

feat: implement partial client updates #2411

Conversation

mattbonnell commented Mar 19, 2021 • edited Loading

Related issue

Proposed changes

Checklist

Further comments

mattbonnell commented Mar 19, 2021

mattbonnell commented Mar 19, 2021

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattbonnell Mar 23, 2021 • edited Loading

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 Mar 24, 2021

mattbonnell commented Mar 24, 2021

aeneasr commented Mar 24, 2021

aeneasr commented Mar 24, 2021

mattbonnell Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattbonnell commented Mar 24, 2021

aeneasr commented Mar 29, 2021

mattbonnell commented Mar 29, 2021 • edited Loading

zepatrik commented Mar 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

mattbonnell commented Apr 4, 2021

aeneasr commented Apr 5, 2021

mattbonnell commented Mar 19, 2021 •

edited

Loading

mattbonnell Mar 23, 2021 •

edited

Loading

mattbonnell Mar 24, 2021 •

edited

Loading

mattbonnell commented Mar 29, 2021 •

edited

Loading