Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose Response Headers on ArcgisError object for status != 200 #1181

Open
efbenson opened this issue Nov 8, 2024 · 4 comments
Open

Expose Response Headers on ArcgisError object for status != 200 #1181

efbenson opened this issue Nov 8, 2024 · 4 comments

Comments

@efbenson
Copy link

efbenson commented Nov 8, 2024

Describe the problem

We had a support request with AGOL and they need the response headers, there is no way to get them from this package without patching the module (which we are attempting) When a status code 500/503 (non 200) error is returned the response headers are not exposed via the error object.

Describe the proposed solution

We are seeing if this will work (in my simulations it appeared to). Tweaking this code here seemed to do it.

https://github.com/Esri/arcgis-rest-js/blob/main/packages/arcgis-rest-request/src/request.ts#L424

            const { status, statusText, headers } = response;
            const { message, details } = jsonError.error;
            const formattedMessage = `${message}. ${
              details ? details.join(" ") : ""
            }`.trim();

            throw new ArcGISRequestError(
              formattedMessage,
              `HTTP ${status} ${statusText}`,
              {body: jsonError, headers: [...headers]},
              url,
              options
            );
          })

Alternatives considered

No response

Additional Information

No response

@gavinr-maps
Copy link
Contributor

@efbenson thank you for logging this issue. This request seems reasonable to me. @patrickarlt @dbouwman do you have any thoughts on this request? Just to be clear, the proposed change is to change/add this code:

image
... at this location: https://github.com/Esri/arcgis-rest-js/blob/main/packages/arcgis-rest-request/src/request.ts#L424

@patrickarlt
Copy link
Contributor

This looks good do me if someone wants to do a PR.

@efbenson
Copy link
Author

Looking into this further there is a couple cases where they would have to be trapped and exposed. My original change and the condition under it:
https://github.com/Esri/arcgis-rest-js/blob/main/packages/arcgis-rest-request/src/request.ts#L420
image

That was not hard to patch, the hard part is when the wrapped response comes through on a 200 status code but has the error in the response payload:
https://github.com/Esri/arcgis-rest-js/blob/main/packages/arcgis-rest-request/src/request.ts#L458
image

For this case the response object is not available since it is in the previous promise scope and not passed through to this promise that only receives the response body. Probably the simplest way to clean that up would be to refactor it to use await and try catches to keep the response object in scope. But it's a good bit of work for me to just patch into the module in production and we are not up for the risk.

@gavinr-maps
Copy link
Contributor

Hi @efbenson, we think it might be easiest to create a variable up higher level in the scope (higher in the file) called something like originalResponseObject, and set that right before the switch/case block, so that then you can read from that variable within the .then(.. starting on line 476. Will that work for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants