jakartaee / jsonb-api

Jakarta JSON Binding
https://eclipse-ee4j.github.io/jsonb-api/
Other
78 stars 39 forks source link

Configure impl to interface mappings #65

Open jsonbrobot opened 7 years ago

jsonbrobot commented 7 years ago

I have a interface model like:

public interface Customer {

    String getName();
    Address getAddress();
}

public interface Address {

    String getStreet();
    String getCity();
}

For each interface there is a single implementation. Important to note that the model contains nested properties/types using (again) interfaces.

How can this be parsed?

JsonbConfig config = new JsonbConfig()
// TODO: what configuration to add to map interfaces to concrete implementations?
Jsonb jsonb = JsonbBuilder.create(config)
jsonb.fromJson(json, Customer.class);

Above gives exception: javax.json.bind.JsonbException: interface com.acme.Customer not instantiable

Note: I cannot change the interfaces nor the concrete implementations as these are provided.

jsonbrobot commented 6 years ago
jsonbrobot commented 6 years ago

@bravehorsie Commented There is already a pull request for this in the Yasson RI. https://github.com/eclipse/yasson/pull/64 @m0mus Can we move annotation and JsonbConfig in the above pull request to the spec for next release? Looks like a handy feauture.

jsonbrobot commented 6 years ago

@marceloverdijk Commented Can that solution be used without an @Annotation? In my case I cannot change the classes itself.

jsonbrobot commented 6 years ago

@bravehorsie Commented You can also add mapping with JsonbConfig see https://github.com/eclipse/yasson/pull/64/files#diff-42990ea8091951c02b2166a51a23f6a6 second test method. In your case it is also enough to call: Customer customer = jsonb.fromJson(json, CustomerImpl.class);

jsonbrobot commented 6 years ago

@marceloverdijk Commented Thanks! Looking forward to see this incorporated in the jsonb spec.

aguibert commented 5 years ago

I think this is a good feature and we should consider it for the next version of the spec.

Additionally, CDI already has the knowledge to know which impl classes map to which interfaces, so we could have the spec define that if CDI is present, it is used to discover a default impl->ifc mapping.

rmannibucau commented 5 years ago

Hi Johnzon has a property for that ([1])

Side note: cdi does not have that info for two reason:

So at the end you need to use an explicit mapping

[1] https://github.com/apache/geronimo-openapi/blob/master/geronimo-openapi-impl/src/main/java/org/apache/geronimo/microprofile/openapi/impl/loader/DefaultLoader.java#L46

aguibert commented 5 years ago

models are likely nlt cdi beans

Good point, usually the app directly creates model objects (or uses JSON-B to create them) as POJOs.

How about an explicit mapping through the API then? A combination of what you've done with system properties / JsonbConfig and what Roman has done in https://github.com/eclipse-ee4j/yasson/pull/64/files sounds good.

rmannibucau commented 5 years ago

Agree, both work. The @Impl option is weird in practise cause then it bounds the api to an impl which is never the case or you created an interface for nothing. If you take microprofike openapi, the api are not modifiable too so the property in jsonbconfig is the best option IMHO. In other words, if you can use @Impl, you can directly map the impl cause it is your own code and we just get back to the polymorphic issue which has another api to solve that.

aguibert commented 5 years ago

I agree that some sort of @Impl annotation on the field/method defeats the purpose of the application using an interface to begin with.

The main thing I'm after is type-safety with the config. Something like:

    private static final Jsonb jsonb = JsonbBuilder.create(new JsonbConfig()
                    .withInterfaceMapping(Vehicle.class, Truck.class);

But this is not quite as flexible as Roman's @Impl approach because we may want to map Vehicle.class -> Truck.class in one place, but Vehicle.class -> Van.class in another place. To solve this, we could allow for overloading withInterfaceMapping such as:

// Maps deserialization of an interface to a specific implementation for all JSON-B models, 
// unless specified at a more specific scope using withInterfaceMapping(Class, Class, Class)
public JsonbConfig withInterfaceMapping(Class<?> interface, Class<?> impl);

// Maps deserialization of an interface to a specific implementation for the given JSON-B model class
public JsonbConfig withInterfaceMapping(Class<?> interface, Class<?> impl, Class<?> jsonbClass);
rmannibucau commented 5 years ago

Woudlnt it be broken in array of object case? What about starting simple with a direct mapping, all others cases can be solved by deserializers right?

aguibert commented 5 years ago

Woudlnt it be broken in array of object case?

Not sure what you are referring to here -- can you clarify what you mean by this?

What about starting simple with a direct mapping, all others cases can be solved by deserializers right?

I see ifc->impl mapping as a cross-cutting concept to deserializers. For example, what if I have 20 objects that all reference the Vehicle interface, and I want to map them to the Truck impl class. With the deserializer approach, I would have to write 20 deserializers just to say Vehicle-->Truck 20 times over.

rmannibucau commented 5 years ago

@aguibert if you have an array of Foo and Foo being an interface, how do you map it in your last signature?

I would have to write 20 deserializers

No? You only need to map a deserializer (you write it once and reuse it in your model) if the default global mapping does not match and your field is nested which is likely rare. At least we pass microprofile-openapi with that option without custom deserializers for the interface mapping.

kazssym commented 4 years ago

I've implemented simple interface-to-implementation mapping for embedded interface fields using adapters. I wish it could be easier as those adapters do not really convert anything.