Workbook API: missing PATCH method for updating workbook range

[MartinM85]

Hi,

metadata doesn't contain method for updating workbook range. Without this method, we can't update cells. It make Workbook API still unusable. Metadata contains only GET method.

https://learn.microsoft.com/en-us/graph/api/range-update?view=graph-rest-1.0&tabs=http

@baywet @sebastienlevert @andrueastman It seems to me to be the last major blocker related to Workbook API.

I can try to fix missing PATCH, but I don't know what's the root cause.

Maybe another blocker is still absence of composable functions like https://learn.microsoft.com/en-us/graph/api/range-delete?view=graph-rest-1.0

<!-- Set IsComposable to false for all functions with a ReturnType of graph.workbookRange. -->
    <!-- This is to prevent overgeneration of composable functions in the OpenAPI. -->
    <xsl:template match="edm:Schema[@Namespace='microsoft.graph']/edm:Function[edm:ReturnType[@Type='graph.workbookRange']]/@IsComposable">
        <xsl:attribute name="IsComposable">false</xsl:attribute>
    </xsl:template>

[baywet]

Thanks for creating this. For context this transform was added with #561 and is most likely the reason why the delete action operations are missing. We'd need to investigate the blast radius (in terms of expansion) if we removed that. @irvinesunday maybe there are other functions that are bound to workbook range, and are composable themselves, which lead to over expansion. And maybe pushing this transform "one level downwards" would help us enable those scenarios without blowing the expansion? What do you think?

As for the PATCH on the range function, I don't even think this means anything OData wise. Functions by nature do not describe a request body. It seems to me that the excel team decided to implement a PATCH operation support under that path even though it can't be represented with OData semantics.

[MartinM85]

Regarding to expanded paths. Do we need two ways to access the excel tables or names?

/drives/{drive-id}/items/{driveItem-id}/workbook/tables vs. /drives/{drive-id}/items/{driveItem-id}/workbook/worksheets/{workbookWorksheet-id}/tables

/drives/{drive-id}/items/{driveItem-id}/workbook/names vs. /drives/{drive-id}/items/{driveItem-id}/workbook/worksheets/{workbookWorksheet-id}/names

SDKs can generate code only for /drives/{drive-id}/items/{driveItem-id}/workbook/tables and /drives/{drive-id}/items/{driveItem-id}/workbook/names.

Or */microsoft.graph.columnsAfter() can be removed, because it's the same as */microsoft.graph.columnsAfter(count=n) where n=1 Same for */microsoft.graph.columnsBefore() and */microsoft.graph.columnsBefore(count=n) or */microsoft.graph.usedRange() vs. */microsoft.graph.usedRange(valuesOnly=bool).

https://learn.microsoft.com/en-us/graph/api/workbookrange-columnsafter?view=graph-rest-1.0

If the excel team could check newly generated paths in the diff mentioned in https://github.com/microsoftgraph/MSGraph-SDK-Code-Generator/pull/1141#issuecomment-1904988138, maybe they will find some paths which do not make sense, like:

/drives/{drive-id}/items/{driveItem-id}/workbook/tables/{workbookTable-id}/columns/{workbookTableColumn-id}/microsoft.graph.headerRowRange()/sort

Applying sort to headerRowRange doesn't make sense. Sort itself doesn't make sense (https://learn.microsoft.com/en-us/graph/api/resources/workbookrangesort?view=graph-rest-1.0), it should always be followed by apply (https://learn.microsoft.com/en-us/graph/api/rangesort-apply?view=graph-rest-1.0)

Regarding to the PATCH on the range function. Is there any workaround for this or it's something that requires changes in Workbook API?

[baywet]

Thanks for looking into it. This is a bit like airing our dirty laundry in public, but we've had multiple conversations with the Excel team about quality of the metadata in the past. Basically the API that's available in the graph is automatically generated from the Excel object model (in the product) to a large degree. And so is the CSDL for their APIs. Both the Excel team and us don't have such a granular control over things, and there doesn't seem to be any priority/urgency in tweaking the generation process.

What we can control however are:

• Whether functions are composable or not.
• Whether an entity set/collection property is the "ContainsTarget" (canonical path) or not. (that triggers the functions expansion)
• Additional annotations to say "yes, this is not supposed to be expanded, but please expand it anyway", or the opposite.

For the optional parameters, we need to provide both (with and without the parameters) since we don't want to force people to provide a value when they don't want/need to.

For the tables, I don't know that API very well, but I'm guessing that you can have tables outside of worksheets? Or Excel defaults to the main one when nothing is provided? If not, it's probably a metadata issue and we should kill those bindings to "save some space" as the APIs underneath most likely don't work anyway.

[MartinM85]

Tables are always inside a worksheet.

If you call POST /me/drive/items/{id}/workbook/tables/add, you need to specify a sheet name in the address property of the request body.

[baywet]

Since it's functionally equivalent to the other endpoint with worksheet, it'd be worth removing the binding from functions that return tables to workbooks then, so everyone uses the "canonical path" (through worksheet) to avoid over expansion.

[MartinM85]

Not sure why it was closed...

[MartinM85]

Please reopen

[irvinesunday]

I'm looking into this

[irvinesunday]

With regard to missing PATCH operations for workbook range...

The function range is a bound function

<Function Name="range" IsBound="true">
  <Parameter Name="bindparameter" Type="graph.workbookRangeView" />
  <ReturnType Type="graph.workbookRange" />
</Function>

According to the OData spec.: https://docs.oasis-open.org/odata/odata-openapi/v1.0/cn01/odata-openapi-v1.0-cn01.html#sec_InvokeBoundActionsandFunctionsonSing

We only generate get operations for bound functions. Do we want to start supporting patch operations for all bound functions?

cc: @darrelmiller

[irvinesunday]

@baywet I have tested out the composable functions "expansion" to only 1 level. I have used a CSDL which has this restriction removed:

<!-- Set IsComposable to false for all functions with a ReturnType of graph.workbookRange. -->
    <!-- This is to prevent overgeneration of composable functions in the OpenAPI. -->
    <xsl:template match="edm:Schema[@Namespace='microsoft.graph']/edm:Function[edm:ReturnType[@Type='graph.workbookRange']]/@IsComposable">
        <xsl:attribute name="IsComposable">false</xsl:attribute>
    </xsl:template>

Here's the diff. between the current paths and the new generated paths based on the above. The number of paths increase just slightly from 10048 to 11564

We are now able to generate paths like:

/drives/{drive-id}/items/{driveItem-id}/workbook/names/{workbookNamedItem-id}/microsoft.graph.range()/microsoft.graph.delete

@MartinM85 could you help confirm whether any other prior missing (and expected) paths are available from that diff file shared?

We however lose the below paths:

[MartinM85]

@irvinesunday Looks good, paths for all methods are generated.

I've noticed that workbook paths are generated also for File Storage Container (paths started at the line 8608) I'm not familiar with SharePoint Embedded, but if file storage container has relationship to drive and the drive itself can be accessed via/drives/{driveId} endpoint then no need to generate paths /storage/fileStorage/containers/{fileStorageContainer-id}/drive/items/{driveItem-id}/workbook. Similar to /sites/{siteId}/drive/* endpoints which are omitted from SDKs.

[baywet]

@irvinesunday thanks for looking into this. It'd be interesting to see whether the paths we're loosing here are actually supported by the service or not? Also the removal of the composable false in the XSLT here is a ~15% size increase. It's non-negligeable. It'd be interesting to look into the new paths that are added and see whether we're getting repetitions in the segments? My hunch is that a couple of functions (not range) are marked as composable when in facts they are not, leading to a lot of "wasteful repetition". If we could identify them as part of this patch it'd probably result in a much better experience.

[irvinesunday]

I have removed the containment for the drive navigation property of the entity fileStorageContainer and the resulting paths have significantly reduced (but marginal decrease, compared to the original number of paths). It seemed like we were having a lot of repetitive paths on the storage API:

Here's the new diff: https://www.diffchecker.com/U7BhT8uW/

[baywet]

As far as I understand the new file storage API, this is incorrect. When people deploy SharePoint embedded, they are effectively the equivalent of the Site/Web structure we have with other APIs. And the drive actually belongs to the container in the sense it can't be accessed any other way. Hence the containment.

[irvinesunday]

@irvinesunday thanks for looking into this. It'd be interesting to see whether the paths we're loosing here are actually supported by the service or not? Also the removal of the composable false in the XSLT here is a ~15% size increase. It's non-negligeable. It'd be interesting to look into the new paths that are added and see whether we're getting repetitions in the segments? My hunch is that a couple of functions (not range) are marked as composable when in facts they are not, leading to a lot of "wasteful repetition". If we could identify them as part of this patch it'd probably result in a much better experience.

We are getting some repeated segments, for example:

/drives/{drive-id}/items/{driveItem-id}/workbook/worksheets/{workbookWorksheet-id}/microsoft.graph.usedRange()/microsoft.graph.usedRange() and

/storage/fileStorage/containers/{fileStorageContainer-id}/drive/items/{driveItem-id}/workbook/worksheets/{workbookWorksheet-id}/microsoft.graph.usedRange()/microsoft.graph.usedRange()

I've only picked those 2 instances for now.

Also, @baywet what do you mean when you say the file storage API is incorrect? That it is not supported by the service? I can see its reference doc. here: https://learn.microsoft.com/en-us/graph/api/resources/filestoragecontainer?view=graph-rest-1.0

Also, with regard to making the drive navigation property non-contained, can't we get to the same resource using the drives API? Example is given here: https://learn.microsoft.com/en-us/graph/api/filestoragecontainer-get-drive?view=graph-rest-1.0&tabs=http#http-request

[baywet]

I can't think of a valid scenario where the same function would be composed to itself directly (not other function in between). So this is probably a fix to implement for yoko.

What I meant was that I believe /storage/fileStorage/containers/{fileStorageContainer-id}/drive/items path and similar ones are valid and should be part of the conversion result. For the drive that lives under the container (and drive items, etc...), there's no other path to access those.

[irvinesunday]

I can't think of a valid scenario where the same function would be composed to itself directly (not other function in between). So this is probably a fix to implement for yoko.

What I meant was that I believe /storage/fileStorage/containers/{fileStorageContainer-id}/drive/items path and similar ones are valid and should be part of the conversion result. For the drive that lives under the container (and drive items, etc...), there's no other path to access those.

Got it. Let me make the necessary changes to Yoko

[MartinM85]

The official doc mentions both endpoints

GET /storage/fileStorage/containers/{containerId}/drive
GET /drives/{driveId}

Even the official learning module, but it shows

GET https://graph.microsoft.com/v1.0/drives/{{ContainerID}}

I don't have time to setup SharePoint Embedded to confirm it.

[irvinesunday]

The official doc mentions both endpoints

GET /storage/fileStorage/containers/{containerId}/drive
GET /drives/{driveId}

Even the official learning module, but it shows

GET https://graph.microsoft.com/v1.0/drives/{{ContainerID}}

I don't have time to setup SharePoint Embedded to confirm it.

Thanks for the additional info here @MartinM85

Since we can get to the same Container resources using the drives endpoint, I think we can limit generating paths after /storage/fileStorage/containers/{fileStorageContainer-id}/drive/items/{driveItem-id} and eliminate the highlighted paths:

[irvinesunday]

The Yoko lib PR to resolve this is ready for review: https://github.com/microsoft/OpenAPI.NET.OData/pull/552

[baywet]

Thanks for investigating this further @irvinesunday . This begs the question: if the API surfaces are the same, why does the container entity have a drive property? @darrelmiller @BarryShehadeh do you have additional insights from the reviews here?

[baywet]

I just had a chat with Kevin Lam from ODSP. He confirmed that both path sets are valid on the service, but most people today are using the /drives set with SPE. This is because it was designed as a drop in replacement for OSDP and so ISVs don't have to update their code to support it. It'd be fine to patch the contains target to false and ask people to use the request builders with URLs they built themselves if they absolutely want to use that path over the canonical one.

[irvinesunday]

I just had a chat with Kevin Lam from ODSP. He confirmed that both path sets are valid on the service, but most people today are using the /drives set with SPE. This is because it was designed as a drop in replacement for OSDP and so ISVs don't have to update their code to support it. It'd be fine to patch the contains target to false and ask people to use the request builders with URLs they built themselves if they absolutely want to use that path over the canonical one.

Thanks for the follow-up @baywet

[microsoft-github-policy-service[bot]]

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.