microsoft / kiota

OpenAPI based HTTP Client code generator
https://aka.ms/kiota/docs
MIT License
2.89k stars 199 forks source link

provide a none key for serializers and deserializers so no registration is done in the client #3796

Closed andreaTP closed 8 months ago

andreaTP commented 10 months ago

Hi there 👋 ,

I'm finally taking the time to roll out my own versions of Http Client and Json serialization for Java. While connecting all of the dots toghether, I have the impression that the generated code is not completely portable in regards to serialization libraries.

This is the use case:

To my understanding, the end user would need to provide on the classpath both of the serialization libraries (the default on Gson and the alternative on Jackson), and, depending on the specific client he will use a different ser/deser library, which, I believe, is undesirable.

In an ideal world, the end user should be able to pull in the two generated SDKs and have on the classpath only one version of the serializers/deserializers libraries that he should be able to change.

Currently, the dependency is checked by the compiler, so, even if there is a way(through the RequestAdapter?) to actually use only one of the implementations they need to be available on the classpath to succeed the compilation step.

In this situation, the only possibility for the SDK author is to start publishing for the matrix of the possible ser/deser alternatives, again this is a viable workaround but it will cause friction and, possibly, fracturate the ecosystem.

I understand that the final solution for such situations(we all aim for) is to let any end user re-generate in their end project all of the needed clients, sadly, (I think) we agree that this is not viable as of today.

Proposal

I have just skimmed through, but this doesn't seems to be an "intrinsic" problem of the tool, but just a limitation of the current implementation. The ser/deser are registered only(at best of my understanding) in the BaseRequestBuilder generated class that gets instantiated passing as argument the RequestAdapter where ser/deser can be configured programmatically by the end user.

Doing a little bit of "inversion of control" should solve the situation, e.g. doing the ser/deser registration directly in the RequestAdapter instead of hardcoding it in the generated request builder.

Currently we have the generated API class constructor:

    public ApiClient(@jakarta.annotation.Nonnull final RequestAdapter requestAdapter) {
        super(requestAdapter, "{+baseurl}");
        this.pathParameters = new HashMap<>();
        ApiClientBuilder.registerDefaultSerializer(JsonSerializationWriterFactory.class);
        ...
        if (requestAdapter.getBaseUrl() == null || requestAdapter.getBaseUrl().isEmpty()) {
            requestAdapter.setBaseUrl("https://api.twilio.com");
        }
        pathParameters.put("baseurl", requestAdapter.getBaseUrl());
    }

that can, possibly become something like:

    public ApiClient(@jakarta.annotation.Nonnull final RequestAdapter requestAdapter) {
        super(requestAdapter, "{+baseurl}");
        this.pathParameters = new HashMap<>();
        requestAdapter.registerDefaultSerializers();
        if (requestAdapter.getBaseUrl() == null || requestAdapter.getBaseUrl().isEmpty()) {
            requestAdapter.setBaseUrl("https://api.twilio.com");
        }
        pathParameters.put("baseurl", requestAdapter.getBaseUrl());
    }

This moves the injection point to the RequestAdapter layer that ultimately becomes the single source of truth for ser/deser and is defined in the end user application. If I have not missed any invariant we should be able to completely remove --serializer and --deserializer from the CLI options, turning this in a breaking change. Thoughts?

baywet commented 10 months ago

Hi @andreaTP, New picture, summoning some kind of demon, nice 🤣 Jokes aside, thanks for starting the conversation. I'm not sure how would the request adapter be aware of which serializers/deserializers are available without using reflection and without requiring the user to manually provide them to the request adapter before hand with the design you're proposing?

andreaTP commented 10 months ago

New picture, summoning some kind of demon

yep, starting the conversation to, hopefully, get it right here 🙂

I'm not sure how would the request adapter be aware of which serializers/deserializers are available without using reflection and without requiring the user to manually provide them to the request adapter before hand with the design you're proposing?

note: I have only quickly gone through the Java implementation, there might be caveats in other languages I'm not aware of

I really think that this is easy to solve! The current default implementation of the RequestAdapter already stores the ser/deser references: https://github.com/microsoft/kiota-java/blob/0e77a02604f3ef83e5a12194e62dfd0ee17a9434/components/http/okHttp/src/main/java/com/microsoft/kiota/http/OkHttpRequestAdapter.java#L54-L55

I think that we have at least 2 options:

Side Note: in Java those problems are commonly solved using a SPI that will naturally fit in this picture, but is less portable across languages than plain constructors/methods invocations.

baywet commented 10 months ago

I think you may be missing some context on the decision to include the serializers/deserializers as part of the CLI arguments, in the generated code, and add them so a static registry instance so it can be discovered by the request adapter.

Before we did any of that the setup code for a client was the following:

var jpnf = new JsonParseNodeFactory();
// same for text/multipart/form/...
var pnRegistry = new ParseNodeFactoryRegistry();
pnRegistry.ContentTypeFactories.Add(jpnf.ContentType, jpnf);

// same block for the serialization writers

var requestAdapter = new HttpClientRequestAdapter(/*...*/, pnRegistry, swRegistry);
var client = new MyGeneratedClient(requestAdapter);
// finally I can make a call using my client

This was a LOT of boilerplate code. And the average consumer does not care about setting these things up. Some might, hence the possibility of doing so, but most don't.

We can't drive this through configuration (SPI approach) because we don't know where the client will run, and the configuration environment might differ a lot from app to app. Also this would require using some level of reflection, which we'd like to avoid when possible.

So the only alternative to keep it simple given the constraints outlined above, was to leverage a singleton factory.

This will work for:

The only population for whom it breaks down are people using a client built for them, but who are not happy with the defaults provided by the API provider. Like a customer of Microsoft Graph for example. They always have the option to generate their own client, which gives them the benefit of choosing their endpoints/operations.

andreaTP commented 10 months ago

Thanks a lot for the context Vincent it really helps.

The "workaround" of regenerating things obviously works, and I agree that regenerating everything in the end user land is the best solution for those situations for the long term.

Now understand your motivations, still, I still think that there are alternatives that are more convenient to eventually end up with a cohesive ecosystem. For example:

I think that there is great value in being able to ship a package that truly doesn't depend on the libraries below the abstractions interface.

baywet commented 10 months ago

would it help that objective if we had a special key for the cli arguments (like none) so the registration is not included in the client? This way people who want a versatile client at the cost of a bit more setup for the consumers, could do that.

andreaTP commented 10 months ago

Definitely, that would be a reasonable compromise! We would need to document well what to do in each situation but it works for me 👍

andreaTP commented 10 months ago

Adding more context: https://github.com/kiota-community/kiota-java-extra/issues/57

With a user-provided serialization/deserialization registration mechanism, we are going to be able to provide a better integration with existing frameworks and CDI mechanisms.

Please note that this feature becomes more compelling as the default registration mechanism is based on reflection and it's not going to work out of the box in a GraalVM native-image binary.

andreaTP commented 10 months ago

We have discussed the complete solution with @baywet and, if I'm not missing something, we agreed on rolling out a breaking change(when the time comes) aligning all of the languages to:

Ref: https://github.com/kiota-community/kiota-java-extra/pull/58#issuecomment-1843186952

baywet commented 10 months ago

Thanks for the extra context here. I think we'll need to create a separate issue whenever this one gets implemented.

andreaTP commented 10 months ago

I think we'll need to create a separate issue whenever this one gets implemented.

Agree but depending on the timelines, this one can be skipped in favor of the longer-term solution. The "workaround" described here leaves room for improvements anyhow...