FluidFramework: Parse correctly ODSP Error facets

Per ODSP discussion today, this code need to be adjusted to look at the most innerError structure, per this document:

https://microsoft-my.sharepoint-df.com/:fl:/p/hammondpang/ERHuWAarf8REt-Vke5WIUm8Bf-42VNb55XBSrjAbcJL-fw?e=DRD71L

Today’s code:

            const joinSessionPromise = this.joinSession(requestWebsocketTokenFromJoinSession).catch((e) => {
                let code: string | undefined;
                try {
                    code = e?.response ? JSON.parse(e?.response)?.error?.code : undefined;
                } catch (error) {
                    throw e;
                }
                switch (code) {
                    case "sessionForbiddenOnPreservedFiles":
                    case "sessionForbiddenOnModerationEnabledLibrary":
                    case "sessionForbiddenOnRequireCheckout":
                        // This document can only be opened in storage-only mode. DeltaManager will recognize this error
                        // and load without a delta stream connection.
                        this.policies.storageOnly = true;
                        throw new DeltaStreamConnectionForbiddenError(code);
                    default:
                        throw e;
                }
            });

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 15 (15 by maintainers)

Most upvoted comments

The inner most code will always be to base/root/original error, but there is no guarantee that any specific error will ever be that, so if you are interested in a specific error code, you must search the stack.

anthony-murphy on Jun 4, 2021

@marcmasmsft, can you please clarify? I think it was the most inner code that we should always use, and the rest points mostly to the code path taken in SPO and results in outer codes to be more and more generic (though clearly client code may want to use more generic code for bucketing).

If we want to do search, then I’d suggest centralized code (in createOdspNetworkError) to extract all codes and storing this array of codes in single property, such that users (like connectToDeltaStream) can do trivial search. Code would be a bit more ugly as instead of switch statement we would have cascading IF statements.

vladsud on Jun 4, 2021

I believe the advice was not to look at a specific point, like first, or last, but instead just search the stack fo the error we are concerned with, as otherwise changes in their inheritence tree can result in breaking us

anthony-murphy on Jun 3, 2021

It’s more like this:

let error= JSON.parse(e?.response).error;
while (error.innerError !== undefined) {
    error = error.innerError;
}
const code = error.code;

vladsud on Jun 3, 2021

I think there are couple steps related to this item:

First, it makes sense for central code (fetchHelper or likely createOdspNetworkError) to do any kind of common parsing and expose result to all callers in some structured way.
- I do not know if it’s worth it, but we can consider for OdspError type to have facetErrorCode property, such that code referenced above (from connectToDeltaStream) could cast exception to OdspError and continue to work with more strongly typed data (it’s not really as strong typing as we do not control what kind of exception is thrown, but at least developers can trace usage).
- Code in connectToDeltaStream() should be changed to use leverage that new facetErrorCode property instead of current flow (code = e?.response ? JSON.parse(e?.response)?.error?.code : undefined).
This “common parsing” should likely fetch the most inner error code, according to facet structure described in the document references at the start of the issue.
It would be also great to record inner code in telemetry (i.e. have it as unique field added to error object in createOdspNetworkError) in addition to us already reporting response (I think we report it), as it makes it easier to do pivoting.

As for facet structure, I think you got it right with assumption that you account for any depth. I.e. in your example it’s depth of 3, and usage in connectToDeltaStream() assumes depth of one, but it can be anything. Please glance at that doc to ensure our understanding of it is correct. You may force some interesting cases (like create a file and deny access to it to yourself) to see other examples of payloads.

vladsud on Jun 3, 2021

the above document has the format. i would create a new function that takes an error and a list of potential codes. the function would walk the error codes, and see if any of the potential codes exist

anthony-murphy on Jun 3, 2021

It’s more incomplete than incorrect. the data in #5679 is true for those errors right now, but they’ve provided more guidance on how these types of errors should be parse in general. this will ensure they don’t break us, and allow us to have code for general error handling.

anthony-murphy on May 20, 2021