simpleidserver / SimpleIdServer

OpenID, OAuth 2.0, SCIM2.0, UMA2.0, FAPI, CIBA & OPENBANKING Framework for ASP.NET Core
https://simpleidserver.com/
Apache License 2.0
718 stars 94 forks source link

Should PatchOperationParameterConverter have an implementation for WriteJson method? #553

Closed omelianlevkovych closed 4 months ago

omelianlevkovych commented 1 year ago

We have PatchOperationParameterConverter which is used for example in PatchOperationParameter for Patch requests. There is no implementation for the WriteJson method in the PatchOperationParameterConverter. Does it make sense to call the default writer from base JsonConverter or at least throw NotImplementedException?

Because if we want to log the request we will always miss the operation data and it could be not obvious why this is actually happening.

Something similar to this:

public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
{
    writer.WriteValue(value.ToString());
}
simpleidserver commented 1 year ago

First thank you for reporting this issue :) It will be fixed for the next release !

KR,

SID

omelianlevkovych commented 1 year ago

I would like to contribute and create PR if it is possible.

simpleidserver commented 1 year ago

I would be pleased if you could contribute to the project :)

Instead of overriding the WriteJson method, you can make the following changes:

The same problem exists in the class RepresentationParameterConverter, and you can apply the same changes :)

Please feel free to create a pull request on the branch release/4.0.2.

simpleidserver commented 1 year ago

The issue should be fixed in the branch release/4.0.2 :) Changeset : https://github.com/simpleidserver/SimpleIdServer/commit/2ebe23e2d1071881f519920033e132dce1f280e6