https://www.ory.sh/ logo
Join Slack
Powered by
Hello, does anyone have any knowledge on the lates...
# talk-kratos
s
Hello, does anyone have any knowledge on the latest comments from https://github.com/ory/kratos/issues/2380 - I am experiencing the same problem when updating admin metadata with the new PATCH endpoint from the latest release. Is patching admin metadata supported at all?
@bland-eye-99092 Hey Jonas I see that you implemented the PATCH identity endpoint - https://github.com/ory/kratos/pull/2471/commits/e746292e67a476e999d52a4048d2ccdde5e74c29 Can you confirm if patching admin metadata is expected to work?
b
Hey, IIRC it should be supported and this could be a bug. Could you create a new bug for this issue in the ory/kratos repository? Thanks.
s
Before I create the issue, can we just confirm we have the same expectation about the behaviour of this logic? If I have the following identity admin metadata:
Copy code
{}
And I am trying to add key 
key
with value 
value
, I assume I should be using the following request body for the PATCH endpoint:
Copy code
[{"op": "add", "value": "value", "path": "/metadata_admin/key"}]
Can you please confirm this is correct?
This is where I experience the first unexpected behaviour:
Copy code
{
  "error": {
    "code": 400,
    "status": "Bad Request",
    "reason": "add operation does not apply: doc is missing path: \"/metadata_admin/key\": missing value",
    "message": "The request was malformed or contained invalid parameters"
  }
}
I’ve tried an alternative patch
Copy code
[{"op": "replace", "value": {"key":"value"}, "path": "/metadata_admin"}]
Which was even worse cause it returned a 200 but the end result was actually 
metadata_admin
being set to null
b
That seems correct, which indicates there is a bug in there somewhere.
Which was even worse cause it returned a 200 but the end result was actually 
metadata_admin
being set to null
Yes, that is worse… And we should fix that.
s
Got it, thanks for confirming, will create the issue by end of today and send the link here
b
Thank you very much! 🙂
m
Hmm it does work for me, but I dont use the patch identity but rather supply the whole identity as json. So I think there is no problem with admin metadata in general, its more how you patch the identity in your case. My guess is something in the request is malformed, metadata AFAIK does not validate, so that is why you get the 200 back even though there is a malformed request. Please include some steps to reproduce this in your bug report, this will be crucial to find the solution @some-addition-86177
b
Yeah, the mechanisms are a bit different between the two endpoints and (I think) the way we apply JSON patches does not properly handle the admin metadata, if it’s not already present.
As for the 
null
issue, I am not sure, what causes that… But seems at least similar in nature.
s
@magnificent-energy-493 ofc will add steps to reproduce, but seems like we are talking about two different scenarios. Let me elaborate. From what I see there are two ways to update an identity: • the 
PUT /admin/identity/<id>
endpoint which expects a full update (all traits, all metadata, etc.) • the 
PATCH /admin/identity/<id>
endpoint which does partial updates (e.g. adding a single key to the metadata) The latter was introduced recently (in 0.11.0 if I am not mistaking). Previously I used the PUT endpoint simply because the PATCH didn’t exist yet. This was working fine (as you said) but it was cumbersome for partial updates because you need to do write-after-read and from what I saw there was no optimistic locking support to ensure there weren’t any writes in-between the read and write. That’s why I was very excited to see partial updates being added (i.e. the PATCH endpoint) and decided to switch to that. However, for some reason it doesn’t work as expected from a JSON Patch perspective for 
metadata_admin
(
metadata_public
seems to be fine). Will add all the test cases I’ve tried in the issue description
@bland-eye-99092 I don’t think it’s just the 
nil
case. I’ve seen the same problems even if other keys already exist. Will add the test case for that too
@bland-eye-99092 @magnificent-energy-493 Sorry for the delay - here is the issue https://github.com/ory/kratos/issues/3099
@bland-eye-99092 @magnificent-energy-493 FYI - seems like I am not the only one that experienced the same problem - https://github.com/ory/kratos/issues/2950 I closed my bug report in favour of the already existing bug report since they are related to the same bug
b
Ah, sorry. Didn’t have that on the radar. We will take a look soon. Thanks for your investigation and report, though. We highly appreciate it. 🙂 PRs are also welcome. 🙂
s
Got it, will be happy to help in my free time but also trying to save some time to assist with https://github.com/ory/kratos/issues/765 🙂 (which tbh is a functionality with higher priority for me)
3 Views
Open in Slack
PreviousNext
Powered by