#211 - Prototype work to support custom Claim Converters.

[radcortez]

This doesn't build properly in the CI because the SmallRye Converters project is still in SNAPSHOT.

[sberyozkin]

Hi @radcortez What is this pretty scary CDI code about :-) ? Custom converters should have no any CDI code.

[radcortez]

Hehe no, they don't.

This is just some try out to support injection of any type that has a converter registered. It is super rough. It would not look like this in a polished version, but I wanted to see if this worked.

Plugin in the new SmallRye Converters, we are able to support out of the box all the primitive types and wrappers, plus custom Converters.

There is a limitation that I've found. Jsonb doesn't support conversion from a JsonObject directly to a Pojo, so you alway require to convert back to String and then to the object. The feature was already added to Yasson (Jsonb reference implementation), but it is not in the spec yet.

[radcortez]

Let me know if this is something worth pursuing. I believe it is.

[sberyozkin]

@radcortez As I said earlier, the implementations already have JsonObject/Array. I.e right now all non String claims are JSON objects or arrays.

So I don't want to start converting this JSONObject to String first if I have to convert it to Address for example, and then back to JSONB, it does not really work IMHO.

I.e there should be a dedicated converter interface which accepts JsonObject.

[sberyozkin]

Example:

public class AddressConverter implements JsonConverter<Address> {
    public Address convert(JsonObject obj) {
       Address a = new Address();
       a.setStreet(obj.getString("street"));
       // etc
       return a; 
    }
}

This is all I'd expect from a custom Address converter :-)

[sberyozkin]

By the way I referred to JSONP earllier but I really meant javax.json.JsonObject. In some cases the converters accepting a JSON rep in the String would indeed use JSONP, but in case of smallrye-jwt it would be very suboptimal unfortunately

[radcortez]

Hi @sberyozkin

My idea for a Converter was more like this:

AddressConverter implements Converter<JsonObject, Address> {
   public Address convert(JsonObject obj) {
          return JsonbBuilder.create().fromJson(value, Address.class))
    }
}

And this will use Jsonb annotations to handle your mapping. Of course you will be free to perform your manual mapping if you don't want to use Jsonb. With custom converters it will be the user choice.

With a small change to the Converter API, we are able to support your use case. With Jsonb, there is currently a limitation where, you are not able to pass in JsonObject to convert to Pojo directly:
https://stackoverflow.com/questions/55677292/convert-jsonobject-to-pojo-efficiently-with-json-b-1-0-e-g-yasson-java-ee-8

But I guess we shouldn't be to worries about it, since custom converters are targeted to the end user and we can't really control how they choose to implement their Converter.

[sberyozkin]

Converter<From, To> would work too :-)

[radcortez]

We are having some discussion on how to support it, but we will get there :)

[radcortez]

@sberyozkin Due to the findings in #218, we are actually manually converting the original claims to the JsonObject counterparts in here:

smallrye-jwt/implementation/src/main/java/io/smallrye/jwt/JsonUtils.java

Line 14 in 6a9ed99

public class JsonUtils {

So, the discussion about converting from JsonObject to String and then to a Jsonb object doesn't really apply, I believe.

I think we need to rethink our conversion model. I'll follow up in #218.

[sberyozkin]

@radcortez I'm not sure what you mean. If I need an Address then a converter from JsonObject to Address gives it.

[radcortez]

Not exactly, because there is no standard way to convert a JsonObject to a standard type. If you use Jsonb you can only convert from JsonReader, which can take a String, Inputstream etc. but it doesn't take a JsonObject.

So if we want to keep with standard, a toString conversion needs to happen from JsonObject to pass it to Jsonb to convert it into Address.

Of course, you can always convert manually, but I'm talking about the standard conversion mechanisms.

[sberyozkin]

@radcortez We've discussed it before, there should be an option for the users take JsonObject and do a simple manual conversion to whatever type they need.

JSONB based implementation of JsonObject to any type converter can just do jsonObject.toString() if needed until a direct JsonObject to some type conversion works via JSONB.

[sberyozkin]

Or, when the conversion is required, we check the available converters. If the converter with the closest match for a given claim value is available then use it, otherwise, if String converter is available, then convert internally to String and use it

[sberyozkin]

No, my last comment does not make much sense. String is not a good representation for a typed conversion from JSON-formatted content to a typed object. It can be street=a,house=b or <address><street>a</streer></address>.
Really, we are complicating things. For any complex claim, JsonObject is the best, with the manual conversion - also the cheapest. Users who'd turn JsonObject to String and then use JSONP will pay a performance penalty

[radcortez]

Ok. No worries. I have been working on a prototype to support multidimensional conversion in a new project.

One thing that we may want to discuss / improve is that we are eagerly manually converting the Jose types to our Jsonp expected types. The claim might not even be used after converted. Maybe we should consider converting only if the claim is actually requested? Either for injection or direct access.

[sberyozkin]

@radcortez

One thing that we may want to discuss / improve is that we are eagerly manually converting the Jose types to our Jsonp expected types. The claim might not even be used after converted. Maybe we should consider converting only if the claim is actually requested

From the performance optimization point of view it likely makes sense. I guess in most cases the users may get a name or roles from the token at most. Please open a new issue.

Re this actual issue. The more I think about it the more I'm getting convinced that all what is needed is a single interface:

interface JsonConverter<T> {
    T convert(JsonObject json);
}

All the claims with the simple claim values are already taken care of. The only thing which has to be covered is the conversion of the complex claims and these are JSONObjects (eagerly or possibly lazily converted from Maps). There are no other use cases.
The only other variation is an array of the complex claims, and again something like Set<Address> will be covered with the above interface.
This is all what is needed IMHO. We ship this interface, update JWTAuthContextInfoProvider to accept the injections of this interface, and create a map keyed by T. This map goes via JWTAuthContextInfo to the default principal, and is used at the conversion point if a claim is JSONObject. I can't see what else is needed.
Thanks

[radcortez]

Yes, a Converter with source JsonObject should be enough.

I'm sorry for the extended discussion. I was mostly trying to gather information on how to implement this in a generic way in the Converters project, so we could reuse this across multiple implementations.

[sberyozkin]

Hey @radcortez No problems at all, just for the record, IMHO the ideas you propose are very interesting, and we have very good discussions IMHO. Lets do the lazy conversion next in another PR when time allows and then check if we can quickly start from this single interface here. As I said elsewhere such an interface can be easily dropped if proves too limiting, etc...
Thanks for the help

[sberyozkin]

Re the lazy conversion - may be we should have it optional, but we can discuss if a property is needed in the dedicated issue/PR...