Pnpjs: Not possible to access multiple sharepoints within same node app

[ntziolis]

Category

• [x] Enhancement
• [x] Bug
• [ ] Question
• [ ] Documentation gap/issue

Version

Please specify what version of the library you are using: [ 1.3.1 ]

Please specify what version(s) of SharePoint you are targeting: [ sharepoint online, same applies to ANY OnPrem sp any version ]

Expected / Desired Behavior / Question

• Have a single node app that can manage multiple share point environments (staging/production etc.)
• Or ability to create single Azure Function to trigger events in different share point environments (azure function is being called with baseUrl and credentials as params)

Observed Behavior

For a node app this means running setup on sp resets the configuration (baseUrl, credentials etc) across the entire app.

For azure functions this is even worst as this means that 2 separate Azure function calls WILL influence each other when running at the same time. This is HIGHLY counter intuitive and easily could cause havoc when wrong calls get executed against prod environments.

Steps to Reproduce

• create a node app
• perform a set of requests on 2 different share points in parallel

Suggestion

We wither need to be able to create an instance vs access to sp singleton. Or some ability to clone an sp object which "locks" its configuration.

[koltyakov]

Currently, parallel requests to different environments or with different credentials are only possible using child_process.

It's an architectural limitation, the sp.setup method is global. Also, one process can switch between environments dynamically. It can be easily done using await/async and sp.setup to a specific environment/creds before a call to the next environment.

Here is a sample of how it's possible switching between contexts.

[patrick-rodgers]

This isn't a bug, just not something the library was ever designed to do. I am not sure the scale of change required to implement something like this, but we can leave it open as something to investigate. Maybe something for 2.0 depending on the amount of churn involved. This isn't the first time it has come up and with more serverless work happening it is a scenario worth supporting more explicitly.

Edit: @ntziolis Can you share some sample code to give us a concrete idea of what you are doing, what is failing, and how you are setup in Azure? Want to be able to at least see the same issue as you are seeing.

[ntziolis]

Appreciate the rapid feedback.

Use case 1
Create monitoring / admin dashboard for share-point that tracks various share point environments. This means at the same time multiple uses could be accessing various share-point endpoints with different authorizations (down to different AADs entirely, so no shared user or app token can be created that can access all endpoints)

Use case 2
Azure functions that perform daily jobs or maintenance tasks on different sharepoints. Since the jobs are 100% the same for all sharepoint we would like to be able to have one deployment that simply gets called with different configurations to be used. However due to context sharing across azure function calls this is not reliably possible today. Instead we have to deploy separate azure function applications for each sharepoint which dramatically increases our admin overhead.

@koltyakov We actually tried this approach (calling setup prior to each call) but found that this does not work when auth is async. To pinpoint it in your example there is a race condition that might have line 23 executed multiple times prior to line 24 being executed. The auth function simply cannot be async if this approach is supposed to work in cases where "simultaneous" calls to different endpoints can happen.
This is not one of these academical race conditions, as we have actually seen this fail in production (Use case Azure Functions that trigger jobs see above).

Apart from that we are worried about the performance impact of calling setup prior to every call. This might work in the maintenance jobs but not in the dashboard application with many users accessing the site in parallel continuously.

I understand that there might not be a fix soon but Id be interested in where do you believe the best injection point would be as we are willing to fork the codebase to make this scenario work for us. any pointers are welcome.

[koltyakov]

@ntziolis,

Make sense, agreed. Have a suggestion though.

If it's headers based auth and environment with concurrent requests something like this can be used at least as a temporary solution:

import { Web } from '@pnp/sp';
import { getAuth } from 'node-sp-auth'; // not only SPO however limited 
                                        // to headers only authentications

const siteUrl: string = 'https://contoso.sharepoint.com/sites/site';
const creds = { /* ... */ };

(async () => {

    const { headers } = await getAuth(siteUrl, creds); 
    // uses caching while ttl of auth cookie/token
    // for AAD use corresponding auth methods and ADAL.js, etc.

    const web = new Web(siteUrl).configure({
        headers: { ...headers }
    }); // instance level settings

    return web.get(); // actual Function method(s)...

})()
  .catch(console.warn);

[ntziolis]

@koltyakov Thank you for that pointer, this was very helpful. It got us one step closer while opening up another issue, see below.

We did need to play around a little bit to get this to work in node due to Headers not being globally available. By making sure to call sp setup with a node compatible fetch client we could get this to work in principle:

  import { BearerTokenFetchClient } from "@pnp/nodejs";
  sp.setup({
    sp: {
      fetchClientFactory: () => {
        return new BearerTokenFetchClient("");
      },
    },
  });

Note:
We deliberately chose the token based client as this allows us to not provide any info like baseUrl (which is required with SPFetchClient) as we would rather have a request fail than execute against the wrong context.

This approach works fine for normal calls.

Sadly once we got this working we ran into the next issue which is that this approach does NOT work when batching. This is due to web.createBatch not passing through the headers that were supplied to web. Looking at the implementation of batch it doesn't seem to be possible to leverage a similar approach as with web as there is no configure method or similar.

Do you have any ideas on how to get `batch working in a similar fashion than web?

Ideas that we are contemplating:

• Create a custom fetch client as singleton that can hold multiple resolved credentials (meaning headers already)
• We would then call setup once per execution, which would add / update additional headers into the header cache
• Afterwards we would use a function to get the reference to sp instead of using the sp reference directly which also switches the header in the underlying client (synchronously)

Please let us know what you think as this starts to feel a little wrong :)

[ntziolis]

Just wanne provide an update:

• The route of using an singleton client with multiple auth context did not lead to a solution. SInce it had to be synchronous we would loose the ability to update the auth context if it expired.
• Instead we had to resolve to
  
  
  
  • create a derived class from SPBatch
  
  
  • that takes in the headers required
  
  
  • and copy & paste the executeImpl and change a single line to attach the passed in header
  
  
  • to keep things neat we have decided to override createBatch on the created webobject to leverage our custom SPBatch code
  
  

Resulting code can be found here:
https://gist.github.com/ntziolis/47ee46d5dae0d24d1accb2df38e8a058

Summary:

• The workaround for Web is perfectly acceptable for an advance use case like this
• The c&p approach for making Batch play ball however hurts as I know we will miss future bug fixes (lets be honest it will happen :) )
• We and likely others can live with sp as singleton if SPBatch case could be solved more elegantly without c&p code

Suggestions:

• Either Add a FetchOptions argument to SPBatch class ctor that gets applied to the header on execute
• Or add an setHeaders method
• Or add a TransformHeader (overridable and empty in base) method

Either approach would require only a minor code change with no side effects and change with no implications for non advanced users.

[ntziolis]

Do you need any feedback to move this forward? We have been using our overridden SPBatch in production for quite without any issues, also works with new version of the pnp libs. The code has been cleaned up, happy to share if you are interested.

[patrick-rodgers]

No other details needed - happy to have your code if you want to do a PR or share it. This has just been a lower priority issue though we've kept it open because we do want to have a look.

[patrick-rodgers]

Have found the time to add the ability to create isolated library instances as discussed in this issue. These changes are not yet merged, but a beta is available npm install @pnp/sp@beta for testing. Ask that you please give it a try and let us know what you think of our first pass at enabling this functionality, as well of course of any issues you encounter.

The initial docs are available in the PR and outline how to use the new capabilities.

Very interested in any feedback you have. This is a large change so we will be doing additional testing and may update any of the public methods in the beta as we work with it.

[ntziolis]

@patrick-rodgers great to hear and sry I didn't see the request for the cleaned up code until now. :(

[funlovin]

Great to hear that you are working on that topic.
I resolved it with child_process and lots of bootstrapping, were we need to switch the context within one child_process.
Thanks for new possibility....I will give it a try!

[dannyfoncke]

Thanks, eagerly awaiting the release

in the meanwhile, is there a commonjs version of the beta ?

[patrick-rodgers]

@dannyfoncke - yes, all the libraries should be available for each beta version.