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

fix: force updateItemResource to use FormData #500

Merged
merged 1 commit into from Apr 2, 2019

Conversation

@jgravois
Copy link
Contributor

jgravois commented Mar 29, 2019

i hate to suggest we add this, but i have confirmed @ssylvia's report in #499

/updateResources is indeed misconfigured and currently only honors requests constructed with FormData (even when the parameters passed don't require it).

to make things worse, the endpoint returns { success: true } regardless.

arcgisRest.updateItemResource({
  id: "fe8",
  name: "logo.png",
  private: true,
  authentication
})
  .then(response)

until the issue is resolved (hopefully in June), my best idea is to inject an undocumented IParam that triggers the use of FormData regardless of the input parameters and then strip it out before the request is sent. to insert a little more logic inside request that ensures FormData will be used for calls to /updateResources` regardless of the input params.

requiresFormData(params) ||
    (url && new RegExp("/items/.+/updateResources").test(url));

with this change updateItemResource() will just work.

AFFECTS PACKAGES:
@esri/arcgis-rest-request

ISSUES CLOSED: #499

@jgravois jgravois requested a review from patrickarlt Mar 29, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #500 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #500   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          95     95           
  Lines        1214   1215    +1     
  Branches      225    226    +1     
=====================================
+ Hits         1214   1215    +1
Impacted Files Coverage Δ
...es/arcgis-rest-request/src/utils/process-params.ts 100% <ø> (ø) ⬆️
packages/arcgis-rest-request/src/request.ts 100% <100%> (ø) ⬆️
packages/arcgis-rest-items/src/update.ts 100% <100%> (ø) ⬆️
.../arcgis-rest-request/src/utils/encode-form-data.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d79fe0...24af241. Read the comment docs.

@jgravois jgravois changed the title fix: insert backdoor to force FormData request and ensure updateItemResource uses it internally fix: insert backdoor to force FormData for updateItemResource to use internally Mar 29, 2019
@jgravois jgravois requested a review from tomwayson Mar 31, 2019
Copy link
Member

tomwayson left a comment

Nice work on a quick fix.

I'd really like to solve this problem some other way than using an undocumented param.

If either of my suggestions are not viable, then I'd rather find some way to circumvent the call to requiresFormData() than check for a special param, even if that means add a documented option to IRequestOptions. I dunno. I feel torn between 2 bad choices: bloat the API, or have secret special snowflake params.

}/updateResources`;
const url = `${getPortalUrl(
requestOptions as IRequestOptions
)}/content/users/${owner}/items/${requestOptions.id}/updateResources`;

// mix in user supplied params
requestOptions.params = {
file: requestOptions.resource,

This comment has been minimized.

Copy link
@tomwayson

tomwayson Apr 1, 2019

Member

Is this an instance of a File? If so, shouldn't that cause requiresFormData() to return true even w/o passing useFormData: true?

}/updateResources`;
const url = `${getPortalUrl(
requestOptions as IRequestOptions
)}/content/users/${owner}/items/${requestOptions.id}/updateResources`;

// mix in user supplied params
requestOptions.params = {
file: requestOptions.resource,
fileName: requestOptions.name,
text: requestOptions.content,

This comment has been minimized.

Copy link
@tomwayson

tomwayson Apr 1, 2019

Member

If file isn't an instance of File or is optional, then maybe we can construct a new Blob out of content, which should cause requiresFormData() to return true, right?

@ssylvia

This comment has been minimized.

Copy link
Member

ssylvia commented Apr 1, 2019

@tomwayson Both file and params.text are now optional so unfortunately I don’t think your suggestions will work.

We asked for a change that was added in the march release that would allow us to update the access param (private option) without having to upload and replace the current resource. The only options that are passed will be: itemId, filename, and the private option.

The portal team confirmed that FormData is currently required but will update the method to be more flexible in the 7.2 (June) release.

We already tried passing a fake, empty file, as an extra param params.fakeFile to force FormData but apparently the REST API still uses this to update (and replace) the current resource.

@jgravois

This comment has been minimized.

Copy link
Contributor Author

jgravois commented Apr 1, 2019

We already tried passing a fake, empty file, as params.fakeFile to force FormData but apparently the REST API still uses this to update (and replace) the current resource.

i've confirmed this and it appears to tie our hands even further.

@patrickarlt do you have time to jump in and help us break the tie or perhaps offer an alternative suggestion?

in general, my instinct was to keep this undocumented to

  1. try and keep the public API as simple as possible
  2. not encourage developers into thinking they typically need to worry about request internals.

i'd have liked to suggest that @ssylvia just install from this branch until the bug is patched in June, but it doesn't appear you can point npm at a package in a Lerna monorepo via a git url.

lerna/lerna#1003 (comment)

@tomwayson

This comment has been minimized.

Copy link
Member

tomwayson commented Apr 1, 2019

no tie to break, I'm fine your solution @jgravois

@patrickarlt

This comment has been minimized.

Copy link
Contributor

patrickarlt commented Apr 1, 2019

@jgravois from an API design perspective this should probally go on request options as something like forceFormData and we can document it as a way to always force a request to use FormData.

@jgravois

This comment has been minimized.

Copy link
Contributor Author

jgravois commented Apr 1, 2019

@patrickarlt my first instinct was intentionally not to document the change because it doesn't correspond with anything documented in the Portal API. we're simply providing internal scaffolding to work around a (temporary) bug.

i'd even be willing to go so far as making the code comments more explicit to warn that the functionality is subject to change without notice.

on the flip side, if either of you think it'd be preferable to surface this and make the change 'official', couldn't we append it directly to IParams instead of IRequestOptions?

export interface IParams {
f?: ResponseFormats;
[key: string]: any;
}

becomes:

export interface IParams { 
  f?: ResponseFormats; 
  useFormData?: boolean;
  [key: string]: any; 
} 
@patrickarlt

This comment has been minimized.

Copy link
Contributor

patrickarlt commented Apr 1, 2019

@patrickarlt my first instinct was intentionally not to document the change because it doesn't correspond with anything documented in the Portal API. we're simply providing internal scaffolding to work around a (temporary) bug.

@jgravois I have no problem with doing this but what about the cases where someone wants to build something around request and needs to use this option to properly handle resources?

couldn't we append it directly to IParams instead of IRequestOptions?

I actually feel pretty strongly about this. IParams is ONLY for things that are documented params on the REST API. IRequestOptions is specially for controlling things about the request including weird overrides such as this. The behavior for forcing the use of FormData isn't about a param on the REST API it is about modifying the behavior of the request to get a specific behavior which sounds much more like something we would put on IRequestOptions like rawResponse.

@jgravois

This comment has been minimized.

Copy link
Contributor Author

jgravois commented Apr 1, 2019

what about the cases where someone wants to build something around request and needs to use this option to properly handle resources?

besides a misconfigured endpoint like the one @ssylvia discovered, i can't think of a reason a user would need to pass request parameters as FormData when the actual values don't require it.

Either way I agree with you that what i'm doing currently is basically just an abuse of IParams.

in light of the conversation, i came up with another idea.

i just pushed up another commit that substitutes a (simpler) RegExp test inside request to see if we're making a call to the endpoint that's currently misbehaving.

i feel better about this approach than my original suggestion, but i'm (still) open to feedback.

@jgravois jgravois requested a review from tomwayson Apr 2, 2019
@jgravois jgravois changed the title fix: insert backdoor to force FormData for updateItemResource to use internally fix: force updateItemResource to use FormData Apr 2, 2019
@@ -9,8 +9,14 @@ import { encodeQueryString } from "./encode-query-string";
* @param params An object to be encoded.
* @returns The complete [FormData](https://developer.mozilla.org/en-US/docs/Web/API/FormData) object.
*/
export function encodeFormData(params: any): FormData | string {
const useFormData = requiresFormData(params);
export function encodeFormData(params: any, url?: string): FormData | string {

This comment has been minimized.

Copy link
@tomwayson

tomwayson Apr 2, 2019

Member

maybe instead of taking a url as the second arg, this should take forceFormData?: boolean and then it would then: const useFormData = forceFormData || requiresFormData(params);.

That way we could have request() be the only place this temporary logic lives, like:

const forceFormData = new RegExp("/items/.+/updateResources").test(url);
// ...
fetchOptions.body = encodeFormData(params, forceFormData);
// ...
if (
        !requiresFormData(params) &&
        !forceFormData
      ) {}

This comment has been minimized.

Copy link
@jgravois

jgravois Apr 2, 2019

Author Contributor

great idea. incorporated (and rebased)

AFFECTS PACKAGES:
@esri/arcgis-rest-request
@esri/arcgis-rest-items

ISSUES CLOSED: #499
@jgravois jgravois force-pushed the force-form-data branch from a554be2 to 24af241 Apr 2, 2019
@jgravois jgravois merged commit 896725d into master Apr 2, 2019
3 checks passed
3 checks passed
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 9d79fe0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jgravois jgravois deleted the force-form-data branch Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.