avaje / avaje-jsonb

java json binding library via source code generation
https://avaje.io/jsonb/
Apache License 2.0
63 stars 5 forks source link

Interfaces as MixinTargets (error: .... is abstract; cannot be instantiated) #109

Closed xabolcs closed 1 year ago

xabolcs commented 1 year ago

Hi!

In my use case I'd like to use Avaje Jsonb on an "API" dependency: it defines the structure of communicating between other projects like backends and UIs. The UI projects mostly use the interfaces as-is, but there are multiple backend projects and they provide their own implementations for those interfaces.

Because of being external I'm unable to use the @Json.Subtype annotation. And because of the target class is an interface, it dies if I use @Json.Mixin.

Are there any workaround? Or I'm doing some anti-pattern?

Thanks!

SentryMan commented 1 year ago

So to be clear, you're trying to use the subtype annotation on a model class provided by an external library?

SentryMan commented 1 year ago

can you perhaps give a general example of the interface and the subtypes you are trying to create?

xabolcs commented 1 year ago

can you perhaps give a general example

Sure.

xabolcs commented 1 year ago

Example project:

So to be clear, you're trying to use the subtype annotation on a model class provided by an external library?

I'd like to try to de/serialize those backends classes to JSONs through the interfaces defined in project api: I'd like to see only those attirbutes in the .json which are defined in the interface.

SentryMan commented 1 year ago

understood, I'll modify the import annotation to add subtype support.

xabolcs commented 1 year ago

Import? :thinking: That annotation is not just a shortcut?

SentryMan commented 1 year ago

wait a sec, have you tried adding a @Json annotation to the model classes in backend one? Not the interface?

xabolcs commented 1 year ago

Sure, but I don't want to annotate with @Ignore all my backend specific attribute. Just want to generate those which are specified in the interface.

For the record: if I annotate my backend class with @Json and put @Json.Ignore to every backend specific attribute, it will work as expected. But I don't want to put all those @Json.Ignore to my attributes! :upside_down_face:

SentryMan commented 1 year ago

do you really need the fields you need to serialize to be in the same class as the ones you don't want to be serialized? Why not separate into two? one for the backend stuff, and the other for serialization.

xabolcs commented 1 year ago

Good question! Strictly speaking, I don't need it.

But it would be nice to keep the effort minimal while converting the backend JSON data to the api structure.

SentryMan commented 1 year ago

So like how many backend fields are we talking about here, I'm afraid the solution for you might be to use @Ignore because I'm drawing a blank here on how to tell what interface methods to use. @rbygrave what do you think?

rbygrave commented 1 year ago

Well, the avaje-jsonb MixIn feature is designed for concrete types - I can't see how it would work for just interfaces per se. That is, it is designed where we have a second set of concrete types to use instead of the original (in order to support different json mapping annotations etc).

I'd say that this approach of using interfaces (to define the properties that should be included in the API) isn't something I've seen per se. My gut thought is that It almost implies that the API is exclusively for read-only use or there is other implementation code that is not seen (maybe generated).

I think the way forward would be to completely ignore the MixIn feature and actually show an example of what this looks like - interfaces, what implements the interfaces, what avaje-jsonb is expected to do / generate etc.

The UI projects mostly use the interfaces as-is,

And this is only reading data? No setters or constructors etc?

TLDR My REST API's are the data structures (ideally java records) + interfaces for the REST API (GET/POST, path parameters etc) ... and this is generated into openapi spec (which then say Typescript/Javascript/Anything else can generate their client code from).

For myself, I don't express data structures as java interfaces. If avaje-jsonb is to support that I'd need to see an example of how that works.

SentryMan commented 1 year ago

So he actually doesn't want mixin, he just doesn't want to place a ton of ignore annotations on his concrete types.

see this:

For the record: if I annotate my backend class with @Json and put @Json.Ignore to every backend specific attribute, it will work as expected. But I don't want to put all those @Json.Ignore to my attributes! 🙃

rbygrave commented 1 year ago

The example project doesn't have constructors or setters or builders etc. The API is currently just 2 "data structures" and leaves out the java interface that shows how these data structures are used, returned, POST/GET etc. So how do users send data to this API/endpoint etc (when the data structures don't have setters, constructors, builders defined etc).

I realise the example is just a skeleton but there isn't enough there for me to understand how it actually works today or how it could work with avaje-jsonb.

rbygrave commented 1 year ago

But I don't want to put all those @Json.Ignore to my attributes!

Yeah ok, and it would be really good to have 1 good example that shows us what that means in practice. To me, it sounds like an API data structure type is also used for "other things" relatively deeply in the backend.

If that is the case, my gut says I don't do that sort of thing and prefer to use composition or mapping instead BUT ... does that mean this is being done so that for example mapping doesn't need to be done?

Is there some sort of Net Win here because there is now no extra mapping that needs to be done from an internal backend type to a front-end API type, this Net Win only exists for types that are read-only in the API?.

@xabolcs Can you provide a realistic example of one of these types that would require a lot of @Ignore ?

Edit: @xabolcs Can you also confirm if these are Read-Only cases where these types are converted in theToJson direction only (and the FromJson direction is never used).

xabolcs commented 1 year ago

My motivation is to migrate my legacy project which uses Gson to avaje-jsonb. It's legacy because it's 10 years old, has Java 6 in it's mind and was never rewritten.

So instead of providing an artificial example I point you to the real world use case:

To me, it sounds like an API data structure type is also used for "other things" relatively deeply in the backend.

If that is the case, my gut says I don't do that sort of thing and prefer to use composition or mapping instead BUT ... does that mean this is being done so that for example mapping doesn't need to be done?

Your are right! The "data workflow" is that the backend(s) talks to (queries) a third party program through REST API (or even XMLRPC) transforms that data to the common interface and the UI reads and shows it. So I think it is read-only. That transformation is almost just a JsonView in the case of Transmission. :ok_hand:

TLDR My REST API's are the data structures (ideally java records) + interfaces for the REST API (GET/POST, path parameters etc) ... and this is generated into openapi spec (which then say Typescript/Javascript/Anything else can generate their client code from).

Sure, the best solution would be to point me the right direction

rob-bygrave commented 1 year ago

Let me see if I understand this possibility of using interfaces to define the properties to serialise to json (and my gut says that deserialise from json would also work and could be supported).

Lets say we have an interface with getters/accessor methods like:

public interface IPeer {
  String getAddress(); // or address()
  String getClientName(); // or clientName()
  boolean isActive(); // or active()

 // interface "defines" the json properties to support of: 
 //  address, clientName, active (and implies that it should ignore any other properties)
}

And we have an implementation like:

@Json(propertiesFromInterface = IPeer.class)
public class TransmissionPeer implements IPeer {

  boolean active;
  String address;
  String clientName;

  boolean iAmNotInTheInterface; // Ignore for json
  String iAmAlsoExtraProperty; // Ignore for json
  ...
}

For this avaje-jsonb could generate a json adapter for TransmissionPeer that:

Then we can do:

JsonType<IPeer> jsonPeer =  jsonb.type(IPeers.class);

// and perhaps also do ? ...
JsonType<TransmissionPeer> jsonPeer =  jsonb.type(TransmissionPeer.class);

To me a benefit to this type of approach [vs the approach I normally take myself] is that it skips any need for mapping code that maps/converts a "backend type" into a "api type".

Although I originally though of this as being a read-only type use case I don't see any reason why it can't support the From-Json direction (assuming that we only register 1 concrete implementation for the IPeer.class).

A slight alternative to what I have above is that avaje-jsonb could potentially generate 2 adapters - one for IPeer.class and the other for TransmissionPeer.class with the thought that the TransmissionPeer adapter could include all properties [but this might be unnecessary and probably isn't the point/goal of this feature, hmm].

rob-bygrave commented 1 year ago

Jackson has @JsonDeserialize(as = ... <concrete type> )

import com.fasterxml.jackson.databind.annotation.JsonDeserialize;

@JsonDeserialize(as = MyInterfaceImpl.class)
public interface MyInterface {

    String getA();

    String getB();
}

Which is very close to what we are talking about here. To me the way Jackson does this has the linked type on the wrong side though - as there is now an import that exists in the interface type (API) that references the implementation type that could well be in another module.

This links the interface to the implementation rather then the opposite direction which is to link the implementation to the interface.

xabolcs commented 1 year ago

Let me see if I understand this ...

You understand it correctly.

To me a benefit to this type of approach [vs the approach I normally take myself] is that it skips any need for mapping code that maps/converts a "backend type" into a "api type".

This has historical reasons: Transmission is the first and only supported backend in openseedbox-server, so first the communicating to Transmission RPC was established, then that JSON format was generalized to a openseedbox-common to send the data to the frontend (openseedbox, the UI). But I want to add other torrent backends, like rtorrent from jesec which supports JSON RPC.

So in the long run I think it would be inevitable to create independent JSON classes for torrent programs and do the mapping/converting manually. :roll_eyes: But as it's still in the same domain (i.e. torrents) it shouldn't be too different from the current common interfaces (IPeer.class, ITorrent.class, ....).

A slight alternative to what I have above is that avaje-jsonb could potentially generate 2 adapters - one for IPeer.class and the other for TransmissionPeer.class with the thought that the TransmissionPeer adapter could include all properties [but this might be unnecessary and probably isn't the point/goal of this feature, hmm].

It would be nice to generate both interfaces: TransmissionPeer.class for communicating to Transmission backend itself, and IPeer.class to providing the data to UI.

rob-bygrave commented 1 year ago

FYI: I'm onboard with adding support for this:

It would be nice to generate both interfaces

My first thought is to use @Json.Import for the interface code generation generation, adding an optional attribute like implementationType to specify the concrete type to be used in order to support From-Json:

// given: interface IPeer.class ...
@Json.Import(value=IPeer.class, implementationType=TransmissionPeer.class)

We can get the adapter generation for TransmissionPeer [concrete types] as we do currently via @Json or @Json.Import so there is nothing extra required for that.

So this feature would be:

xabolcs commented 1 year ago

So this feature would be ...

Looks promising! Thanks for accepting and picking up my feature request! :pray:

xabolcs commented 1 year ago

Half off-topic question coming ... Why do you talk about @Json.Import instead of just @Json?

Reading the home page, I think it's just a shortcut, to minimize the boilerplate, isn't it?

SentryMan commented 1 year ago

yeah, I'm not sure about using import for this, (though we should add the feature to import as well)

rob-bygrave commented 1 year ago

Why do you talk about @Json.Import instead of just @Json?

Putting @Json on an interface that ONLY ever supports the To-Json direction is fine and imo we should support this.

We could support @Json on the interface that also specifies the target implementation BUT we need people to be wary of doing this as the interface is now tied to the implementation (like Jackson does via their as() attribute).

That is, putting @Json(implementationType = TransmissionPeer.class) on the IPeer interface [so that additionally the From-Json direction is supported] means that interface is now tied to the implementation at compile time which will not work in the case where the implementation is in another module. The way to do this properly is via a @Json.Import.

Json.Import ... it's just a shortcut, to minimize the boilerplate, isn't it?

No, it is not a shortcut. It is primarily for the cases where we do not control the source code and are unable to put the @Json annotation on the type that we want to json serialise etc.

SentryMan commented 1 year ago

makes sense when you put it like that

rbygrave commented 1 year ago

https://github.com/avaje/avaje-jsonb/pull/140 - Adds support for the "To-Json on interface type with fromJson() throwing UnsupportedOperationException"

Still left to do is the "Support both To-Json & From-Json on interface type when a concrete implementation type is specified"

rbygrave commented 1 year ago

https://github.com/avaje/avaje-jsonb/pull/142 - Adds "Support both To-Json & From-Json on interface type when a concrete implementation type is specified"

This means we should have the cases covered required for this now in version 1.6. I'll close this as fixed in 1.6 and we can open new issues as needed.

xabolcs commented 1 year ago

Thanks! :pray: