solveRoute throws error when used with rawResponse

[wsvoja]

Description

Hi,

we are using the function solveRoute in conjuction with the rawResponse=true.
We observed, that this throws an error and looking closer, we found that solveRoute calls cleanResponse in the then section after the request was sent.

https://github.com/Esri/arcgis-rest-js/blob/3.x/packages/arcgis-rest-routing/src/solveRoute.ts#L136

The json() method wasn't yet called on this response object, and therefore the property route doesn't exist in the response object.

It is accessed however in https://github.com/Esri/arcgis-rest-js/blob/3.x/packages/arcgis-rest-routing/src/solveRoute.ts#L161 which throws the error.

This behavior seems to be applicable for at least version 3.X.X and 4.X.X.

Reproduction

1. create an ISolveRouteOptions object
2. Add rawResponse: true during the creation
3. Call solveRoute
4. Add a .catch() block after the call
5. solveRoute will not resolve, it will enter the catch block

Logs

No response

System Info

@esri/arcgis-rest-routing: "^3.5.1"

Additional Information

No response

[wsvoja]

Just saw you were working on the issue, while I wrote this message. Great!

Hi, we saw, that not only solveRoute has this pattern of applying cleanResponse after the response was retrieved.
This at least applies to matrix routing originDestinationMatrix and bulkGeocode
In case of bulkGeocode the post processing is not a separate function. It is located in the then block directly.

This structure causes someone to receive a different json response, when using rawResponse and call json on it, than without the usage of rawResponse.

Suggestion: Monkey patch json when rawResponse is requested:

  // ...
  .then((response) => {
    if (options.rawResponse) {
      const origJsonMethod = response.json
      response.json = () => {
        return origJsonMethod.call(response).then(cleanResponse);
      }
      return response;
    }
    // ...

[gavinr]

Thanks for calling to attention the other places like originDestinationMatrix and bulkGeocode .... we'll have to do the same thing as #1046 in those, or think of a better way to handle these cases holistically.

[gavinr]

@wsvoja it seems like originDestinationMatrix and bulkGeocode already support rawResponse: true correctly:

1. I have added a unit test in my PR for originDestinationMatrix so we don't have a regression around this in the future: https://github.com/Esri/arcgis-rest-js/pull/1046/files#diff-a136821d09a439304ecd94b20436ee7911717a905d17a79fce5fba1c7cd003f1R796
2. bulkGeocode already has a unit test that verifies this:

   arcgis-rest-js/packages/arcgis-rest-geocoding/test/bulk.test.ts

   Lines 161 to 192 in 805844b

   	it("should support rawResponse", (done) => {
   	fetchMock.once("*", GeocodeAddresses);
   	const MOCK_AUTH = {
   	getToken() {
   	return Promise.resolve("token");
   	},
   	portal: "https://mapsdev.arcgis.com"
   	};
   	bulkGeocode({ addresses, authentication: MOCK_AUTH, rawResponse: true })
   	.then((response: any) => {
   	expect(fetchMock.called()).toEqual(true);
   	const [url, options]: [string, RequestInit] = fetchMock.lastCall("*");
   	expect(url).toEqual(
   	"https://geocode.arcgis.com/arcgis/rest/services/World/GeocodeServer/geocodeAddresses"
   	);
   	expect(options.method).toBe("POST");
   	expect(response.status).toBe(200);
   	expect(response.ok).toBe(true);
   	expect(response.body.Readable).not.toBe(null);
   	response.json().then((raw: any) => {
   	expect(raw).toEqual(GeocodeAddresses);
   	done();
   	});
   	// this used to work with isomorphic-fetch
   	// expect(response instanceof Response).toBe(true);
   	})
   	.catch((e) => {
   	fail(e);
   	});
   	});

If you have a replication case to show it not working, please provide. Otherwise we'll move forward with the PR for the fix for solveRoute (#1046). Thanks!

[wsvoja]

Hi,

thank you for working on that.
I see the improvement in solveRoute and it will certainly fix the error.

However, the fix doesn't allow me to call cleanResponse.

The line
https://github.com/Esri/arcgis-rest-js/blob/main/packages/arcgis-rest-routing/src/solveRoute.ts#L178
shows, that cleanResponse is not publicly exported. So using the rawResponse=true parameter returns the raw response and then calling json() afterwards results a different object than rawResponse=false and retrieving the response with an implicit cleanResponse call.

This is what I wanted to point out in my second comment and this problem also applies to originDestinationMatrix and bulkGeocode.
And therefore I suggest to make an implicit call of cleanResponse when calling json() on the raw response object.

There might be other alternatives to this approach, such as by adding a method cleanJson() which calls json() and immediately cleanReponse or make cleanResponse public (or somehow available otherwise, e.g. as method in the response object) and document respectively.

Thank you and best regards.

[gavinr]

Hi @wsvoja, thanks for the message. Could you please explain more about your use case of why you want to access the rawResponse and then call cleanResponse() yourself? Why not just let ArcGIS REST JS call cleanResponse()?

[wsvoja]

Hi Gavin, sorry for my late response.

In our use case, we want to access the HTTP Reponse Headers, especially the cache information.
However, not always.
So there are situations, when we retrieve a cleaned Json (no raw response) and the orignal Json (when we call json() on the raw response object).

Both Json objects are different, since the response object is altered at multiple places, e.g. coordinates are converted with arcgisToGeoJSON etc.

Therefore our application has to deal with two different kinds of responses, which could be aligned easily by calling cleanResponse.

I could even be, that the raw response Json might not fit the documented schema, https://esri.github.io/arcgis-rest-js/api/routing/ISolveRouteResponse/#routes says IFeatureSetWithGeoJson but GeoJson is added during cleanResponse (https://github.com/Esri/arcgis-rest-js/blob/main/packages/arcgis-rest-routing/src/solveRoute.ts#L160)

I hope I could explain the situation well enough.

Thank you and best regards