owningSystemUrl in IServerInfo is not available in JS SDK's ServerInfo

arjanvanzutphen commented 1 year ago

Describe the bug

The property 'owningSystemUrl ' is not available in JS SDK's ServerInfo but is defined in IServerInfo.

Should owningSystemUrl be removed or does it serve another purpose?

Reproduction

Run this snippet from the website: https://developers.arcgis.com/arcgis-rest-js/api-reference/arcgis-rest-request/ArcGISIdentityManager#fromCredential

require(["esri/id"], (esriId) => {
  const credential = esriId.findCredential("https://www.arcgis.com/sharing/rest");
  const serverInfo = esriId.findServerInfo("https://www.arcgis.com/sharing/rest");

  const manager = ArcGISIdentityManager.fromCredential(credential, serverInfo);
});

TypeScript will notice that 'owningSystemUrl' is not available in ServerInfo

Logs

webpack compiled with 1 warning
ERROR in src/logic/GroupLogic.ts:16:68
TS2345: Argument of type 'ServerInfo' is not assignable to parameter of type 'IServerInfo'.
  Property 'owningSystemUrl' is missing in type 'ServerInfo' but required in type 'IServerInfo'.
    14 |   );
    15 |
  > 16 |   const manager = ArcGISIdentityManager.fromCredential(credential, serverInfo);
       |                                                                    ^^^^^^^^^^
    17 |   debugger;
    18 |   try {
    19 |     const resultGroupUsers = await getGroupUsers(groupId, {

System Info

"@esri/arcgis-rest-request": "4.2.0",

Additional Information

No response

gavinr-maps commented 1 year ago

Hi Arjan, thanks for logging this.

This snippet is actually old - using the JS API v3 syntx (for example, should be esri/identity/IdentityManager instead of esri/id). Could you please send a PR to update this snippet to the latest JS API? This can be found in https://github.com/Esri/arcgis-rest-js/tree/main/demos/jsapi-integration I think.

After that's updated, can you please provide an updated replication case.

gavinr-maps commented 6 months ago

Snippet (non-)issue

After further looking into this, I think that snippet is actually acceptable:

The snippet uses findCredential, which "Returns the Credential for the resource identified by the specified url"
The example in the REST JS sample uses getCredential, which "Returns a Credential object that can be used to access the secured resource identified by the input URL"

I think the Credential that is returned from findCredential or getCredential should both be able to be passed into the ArcGIS REST JS ArcGISIdentityManager.fromCredential function.

So I don't think the snippet is the issue.

Replication case

I was able to create a simple replication case for this issue in a StackBlitz: https://stackblitz.com/edit/vitejs-vite-ikrxa2?file=src%2Fmain.ts,src%2Fstyle.css,src%2Fmap.ts&terminal=dev

That ^ is the original issue (I think).

Given that, I do agree with @arjanvanzutphen that owningSystemUrl should be removed (or set to optional?) here. Note that this property does not seem to be included anymore: for example compare our fetch mock vs the actual response: https://sampleserver6.arcgisonline.com/arcgis/rest/info/?f=json

{
  "currentVersion": 10.91,
  "fullVersion": "10.9.1",
  "soapUrl": "https://sampleserver6.arcgisonline.com/arcgis/services",
  "secureSoapUrl": null,
  "authInfo": {
    "isTokenBasedSecurity": true,
    "tokenServicesUrl": "https://sampleserver6.arcgisonline.com/arcgis/tokens/",
    "shortLivedTokenValidity": 60
  }
}

gavinr-maps commented 6 months ago

Discussed with @patrickarlt, we propose to remove this line: https://github.com/Esri/arcgis-rest-js/blob/7d8220ad3d3a872fda36f02c21e4046a2b87b7ed/packages/arcgis-rest-request/src/ArcGISIdentityManager.ts#L97

Esri / arcgis-rest-js