pnp / pnpjs

Fluent JavaScript API for SharePoint and Microsoft Graph REST APIs
https://pnp.github.io/pnpjs/
Other
764 stars 304 forks source link

Graph batching seems to be broken #2807

Closed nbelyh closed 1 year ago

nbelyh commented 1 year ago

What version of PnPjs library you are using

3.x

Minor Version Number

3.17

Target environment

All

Additional environment details

I'm using PNPJS to call GRAPH API.

Question/Request

It seems that PNPJS makes request to https://graph.microsoft.com/$batch (without version). This URL seems to have stopped working. I'm receiving "Method not supported" (405)

The request should be probably made to https://graph.microsoft.com/v1.0/$batch (this works)

There is a specific code piece to remove the version, I'm wondering why? Here: https://github.com/pnp/pnpjs/blob/c807525236772b8483cc1cfee0c79ea071a28c27/packages/graph/batching.ts#L101

bcameron1231 commented 1 year ago

The endpoint comes from the base Graph Queryable object (if you're not supplying one). Seems like you may be missing something.

Can you share some code for us and how your initializing your Graphfi object?

nbelyh commented 1 year ago

@bcameron1231 Yes, I hope so. I am initializing it like this (reproducible example):

import { graphfi } from '@pnp/graph';
import '@pnp/graph/users';
import '@pnp/graph/batching';

const client= graphfi('https://graph.microsoft.com/v1.0').using(....);

const me1 = await client.me(); // <<--- fine
console.log(me1);

const [batch, execute] = client.batched();
batch.me().then(me2 => console.log(me2))
await execute(); /// <<<--- booom, "method not allowed"

It makes query to https://graph.microsoft.com/$batch instead of https://graph.microsoft.com/v1.0/$batch

bcameron1231 commented 1 year ago

So you don't need to pass in the Graph URL there. By default the graphFi object would point to v1.0.

If you want to specify the Graph Endpoint, you can do so by using behaviors. Endpoint, DefaultInit, GraphDefault behaviors allow you to do so.

I just tested without setting the url, with a batch, and it worked for me.

nbelyh commented 1 year ago

@bcameron1231 Hmm right If I remove the graph URL from graphfi(), the batched call starts working. But then the "normal" (non-batched) call breaks, i.e. client.me() with 'invalid url' exception 😅

The regex in the issue description source link specifically removes the '/v1.0' piece from the batching URL (thus breaking the call). Is there some reason for this?

patrick-rodgers commented 1 year ago

Is the issue really with .me? Everything should work by default without you needing to specify a url.

nbelyh commented 1 year ago

@patrick-rodgers For me the batching issue is not with ".me" (it's with planner batching actually), but it can be reproduced with ".me". Batching ".me", like in the example above, of course does not make much sense 😅
The things that I tried: me and me.memberOf give an error with empty URL. But I will re-check in a clean project.

bcameron1231 commented 1 year ago

What type of application is this? SPFx, Node, Browser? We'll take a look.

By invalid URL error, I suspect you are seeing this one? image

If you used DefaultInit() or GraphBrowser() (assuming this is non-spfx), it should work for now.

nbelyh commented 1 year ago

Yes, this one. Exact using is below, it's a browser with custom authentication (SSO):

Option 1 (results in batch call failing with "method not supported" because of missing '/v1.0' in the batch url):

     const graph = graphfi('https://graph.microsoft.com/v1.0');

      graph.using(
        DefaultHeaders(),
        DefaultInit(),
        BrowserFetchWithRetry(),
        DefaultParse(),
        MyCustomAuthBehavior()
   );

Option 2 (results in "Invalid URL"):

     const graph = graphfi();

      graph.using(
        DefaultHeaders(),
        DefaultInit(),
        BrowserFetchWithRetry(),
        DefaultParse(),
        MyCustomAuthBehavior()
   );

The MyCustomAuthBehavior just sets the authorization header to a known value i.e. "Authorization: Bearer mytoken", it is not related to the problem.

Another note - it works fine for SharePoint, i.e. no such problem with SPFI, only graph.

bcameron1231 commented 1 year ago

Would you mind sharing with me your CustomAuth Behavior? Feel free to blur out anything that may be private.

I can't reproduce the issue you have with option 2.

nbelyh commented 1 year ago

@bcameron1231 It looks like this:

export function MyCustomAuthBehavior(): (instance: Queryable) => Queryable {

  return (instance: Queryable) => {

    instance.on.auth.replace(async (url: URL, init: RequestInit) => {

      const accessToken = await AuthService.getAuthToken();  //<< this guy just returns a string
      init.headers = { ...init.headers, Authorization: `Bearer ${accessToken}` };

      return [url, init];
    });

    return instance;
  };
}

I can make a demo repository if still not reproducible

bcameron1231 commented 1 year ago

Yes, I still cannot reproduce. A repository would be helpful. Thank you!

nbelyh commented 1 year ago

Will do, most probably tomorrow.

bcameron1231 commented 1 year ago

Just checking in to see how things are going on your end?

bcameron1231 commented 1 year ago

Closing this due to inactivity. If you continue to have issues please open a new issue, link to this issue, and provide any additional details available. Thanks!

github-actions[bot] commented 1 year ago

This issue is locked for inactivity or age. If you have a related issue please open a new issue and reference this one. Closed issues are not tracked.