Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No way to query product ingestion API since CustomRequestBuilder removed in V6 #2096

Open
Tomanow opened this issue Jul 25, 2024 · 11 comments · May be fixed by microsoft/kiota-java#1562
Open
Assignees
Labels
type:feature New experience request

Comments

@Tomanow
Copy link

Tomanow commented Jul 25, 2024

Is your feature request related to a problem? Please describe the problem.

We are unable to upgrade to v6 since we can no longer construct custom URLs for graph requests, and the SDK we use (the partner marketplace product ingestion API) is not supported by the client.

The BaseClient#customRequest() method and CustomRequestBuilder support was removed in v6 with no alternative provided.

Describe the solution you'd like.

Support for building custom graph request URIs and/or SDK support for the partner marketplace product ingestion APIs.

Additional context?

We (MongoDB) are a significant Microsoft Azure Partner with high profile integrations relying on the Java SDK.

@Tomanow Tomanow added status:waiting-for-triage An issue that is yet to be reviewed or assigned type:feature New experience request labels Jul 25, 2024
@Ndiritu Ndiritu self-assigned this Aug 12, 2024
@Ndiritu Ndiritu added status:needs-discussion An issue that requires more discussion internally before resolving and removed status:waiting-for-triage An issue that is yet to be reviewed or assigned labels Aug 20, 2024
@Ndiritu
Copy link
Contributor

Ndiritu commented Aug 20, 2024

@baywet @andrueastman what do you think about us adding a customRequest() method to the service lib's GraphServiceClient to cover this scenario?

public T customRequest(RequestInformation requestInformation, Class<T extends Parsable> responseType)

@andrueastman
Copy link
Member

Is this something that can be resolved with the generated withUrl methods?

public T customRequest(RequestInformation requestInformation, Class<T extends Parsable> responseType)

I believe this is functionally equivalent to the send methods in the request adapter.
https://github.com/microsoft/kiota-java/blob/6291fa271db0f3e5844832b6a2f42368d22006de/components/http/okHttp/src/main/java/com/microsoft/kiota/http/OkHttpRequestAdapter.java#L267

@baywet
Copy link
Member

baywet commented Aug 20, 2024

This API is "special" and doesn't follow the typical Graph conventions/infrastructure. They just happen to be on the same host. The base URI is https://graph.microsoft.com/rp/ (note the absence of version).
This is why nothing is generated for it, it's not part of the metadata, etc...
Which means the "with URL" pattern won't solve the partner issue here.
What would solve it is to be able to get an OkHttpClient with an auth middleware.
For that we'd need:

  1. a middleware that accepts an authentication provider, and gets the access token provide from it.
  2. a client factory that adds that middleware to the stack to make things easier
  3. a bit of documentation to demonstrate the usage for arbitrary requests. Where the guidance is to use the generated APIs first, then try with URL, if all else fails, this OkHttpClient. And also NOT to insert this middleware in the stack for the generated GraphClient since we already have the authentication provider here.

Assuming this is clear, does one of you want to work on a design here? We'd then generalize to other languages.

@Ndiritu
Copy link
Contributor

Ndiritu commented Aug 20, 2024

Sounds good @baywet. Was targeting re-using our (de)serialization but it would be added effort to make the customer's custom models into Parsable's yet v5 used GSON to deserialize to models.

Using an OkHttpClient with Graph auth middleware works better.

Could the Graph auth middleware still be present by default & be initialized with the same auth provider the RequestAdapter uses to retain allowed hosts & other config + check for an Authorization header before considering adding one?

I can take this on probably next week/sprint worst case.

@baywet
Copy link
Member

baywet commented Aug 20, 2024

Could the Graph auth middleware still be present by default & be initialized with the same auth provider the RequestAdapter uses to retain allowed hosts & other config + check for an Authorization header before considering adding one?

We could explore that approach but we need to be careful not to:

  • do auth twice
  • break CAE challenges / resolution

This would have the benefit of not asking customers to juggle between multiple okhttp clients.

@andrueastman
Copy link
Member

Just to be clear here, do we need the auth middleware/custom client as the token needed is different than the one that would be used for normal graph calls?

In my head, this should ideally work as the sendXXX methods from the adapter should handle the auth as well CAE challenges together with deserialization assuming the right method is called. (what we may need here is method that makes this all easier).

        // Create a request info object
        var requestInformation = new RequestInformation();
        requestInformation.httpMethod = HttpMethod.GET;
        requestInformation.urlTemplate = "https://graph.microsoft.com/rp/xxxxx";

        // Setup an error mapping
        HashMap<String, ParsableFactory<? extends Parsable>> errorMapping = new HashMap();
        errorMapping.put("XXX", ODataError::createFromDiscriminatorValue);

        // get the response as a stream
        var responseStream = graphClient.getRequestAdapter().sendPrimitiveCollection(requestInformation, errorMapping, Stream.class);
        // TODO deserialize the stream as desired.

        // OR get the parsable using a factory as User object
        var deserializedResponse = graphClient.getRequestAdapter().send(requestInformation, errorMapping, User::createFromDiscriminatorValue);

a middleware that accepts an authentication provider, and gets the access token provide from it.

A small challenge we may find with this is that the authentication providers act on RequestInformation instance from kiota abstractions while the middleware is handling transport specific objects(Request from okhttp) objects. Unless you mean hooking up on the AccessTokenProvider?

@Ndiritu
Copy link
Contributor

Ndiritu commented Aug 21, 2024

The authentication request is a standard Azure Identity request with the default Graph scopes.

My concerns with using the sendXXX methods is our SDK doesn't generate models for this API so customers have probably implemented their own. This would mean asking devs to implement Parsable & ParsableFactory.

With the okHttpClient the methods to call are more direct & (de)serialization in the control of the developer.

A small challenge we may find with this is that the authentication providers act on RequestInformation instance from kiota abstractions while the middleware is handling transport specific objects(Request from okhttp) objects. Unless you mean hooking up on the AccessTokenProvider?

Yeah, good point. We may have to add a method that converts from the native okHttp Request to RequestInformation

@baywet
Copy link
Member

baywet commented Aug 21, 2024

@andrueastman the request adapter interface (and the associated request information) was never designed to be used by humans, but rather to facilitate the work of the generated clients/reduce the amount of generated code that's required. (on top of providing a level of indirection to avoid tight coupling with any given http client).

The reason why we used this interface to implement tasks (batching, etc...) is mostly because we didn't want to have customers juggle with multiple clients, auth setup etc...

For arbitrary requests, customers will get a much better experience using the native client augmented with the middleware handlers. In fact we used to support that in the previous generation of SDKs. The reason why we didn't feel the need to port it to this newer generation was because the generated portion supports so much more of the API surface, so there was a much smaller need to implement it. That's until we ran in this "odd API".

@andrueastman
Copy link
Member

@andrueastman the request adapter interface (and the associated request information) was never designed to be used by humans, but rather to facilitate the work of the generated clients/reduce the amount of generated code that's required. (on top of providing a level of indirection to avoid tight coupling with any given http client).

Thanks for the insight here. This is the bit I was missing.

My concerns with using the sendXXX methods is our SDK doesn't generate models for this API so customers have probably implemented their own. This would mean asking devs to implement Parsable & ParsableFactory.

Not necessarily as they could use the methods returning a stream/string and then deserialize the values as they wish. However this is now moot. Let's go ahead with the earlier suggestion here.

@baywet
Copy link
Member

baywet commented Aug 27, 2024

related microsoft/kiota-dotnet#270

@Ndiritu Ndiritu added status:waiting-for-technical-design-work Needs technical design before any code work can start and removed status:waiting-for-technical-design-work Needs technical design before any code work can start status:needs-discussion An issue that requires more discussion internally before resolving labels Aug 29, 2024
@baywet
Copy link
Member

baywet commented Sep 5, 2024

authored microsoftgraph/msgraph-sdk-design#111 to help move this along

@Ndiritu Ndiritu linked a pull request Sep 11, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New experience request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants