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

B/fix file upload bugs #761

Merged
merged 9 commits into from
Oct 2, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export function attachmentFile() {
} else {
const fs = require("fs");
return fs.createReadStream(
"./packages/arcgis-rest-feature-service/test/mocks/foo.txt"
"./packages/arcgis-rest-feature-layer/test/mocks/foo.txt"
);
}
}
Expand Down
39 changes: 1 addition & 38 deletions packages/arcgis-rest-portal/src/items/add.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ import {
IUpdateItemResponse,
IItemResourceResponse,
determineOwner,
IManageItemRelationshipOptions,
IItemPartOptions
IManageItemRelationshipOptions
} from "./helpers";
import { updateItem, IUpdateItemOptions } from "./update";

Expand Down Expand Up @@ -137,39 +136,3 @@ export function addItemResource(
return request(url, requestOptions);
});
}

/**
* ```js
* import { addItemPart } from "@esri/arcgis-rest-portal";
* //
* addItemPart({
* id: "30e5fe3149c34df1ba922e6f5bbf808f",
* part: data,
* partNum: 1,
* authentication
* })
* .then(response)
* ```
* Inquire about status when publishing an item, adding an item in async mode, or adding with a multipart upload. See the [REST Documentation](https://developers.arcgis.com/rest/users-groups-and-items/add-item-part.htm) for more information.
*
* @param id - The Id of the item to get status for.
* @param requestOptions - Options for the request
* @returns A Promise to get the item status.
*/
export function addItemPart(
requestOptions?: IItemPartOptions
): Promise<IUpdateItemResponse> {
return determineOwner(requestOptions).then(owner => {
const url = `${getPortalUrl(requestOptions)}/content/users/${owner}/items/${
requestOptions.id
}/addPart`;

const options = appendCustomParams<IItemPartOptions>(
requestOptions,
["file", "partNum"],
{ params: { ...requestOptions.params } }
);

return request(url, options);
});
}
8 changes: 1 addition & 7 deletions packages/arcgis-rest-portal/src/items/create.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,9 @@ export function createFolder(
export function createItemInFolder(
requestOptions: ICreateItemOptions
): Promise<ICreateItemResponse> {
if (requestOptions.file && !requestOptions.multipart) {
return Promise.reject(
new Error("The request must be a multipart request for file uploading.")
);
}

if (requestOptions.multipart && !requestOptions.filename) {
return Promise.reject(
new Error("The file name is required for a multipart request.")
new Error("The filename is required for a multipart request.")
);
}

Expand Down
70 changes: 64 additions & 6 deletions packages/arcgis-rest-portal/src/items/upload.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,64 @@
/* Copyright (c) 2017-2019 Environmental Systems Research Institute, Inc.
* Apache-2.0 */

import { request } from "@esri/arcgis-rest-request";
import { request, appendCustomParams } from "@esri/arcgis-rest-request";
import { IItemAdd } from "@esri/arcgis-rest-types";

import { getPortalUrl } from "../util/get-portal-url";
import {
IUserItemOptions,
IUpdateItemResponse,
determineOwner
determineOwner,
IItemPartOptions,
serializeItem
} from "./helpers";

export interface ICommitItemOptions extends IUserItemOptions {
item: IItemAdd;
}

/**
* ```js
* import { addItemPart } from "@esri/arcgis-rest-portal";
* //
* addItemPart({
* id: "30e5fe3149c34df1ba922e6f5bbf808f",
* file: data,
* partNum: 1,
* authentication
* })
* .then(response)
* ```
* Add Item Part allows the caller to upload a file part when doing an add or update item operation in multipart mode. See the [REST Documentation](https://developers.arcgis.com/rest/users-groups-and-items/add-item-part.htm) for more information.
*
* @param requestOptions - Options for the request
* @returns A Promise to add the item part status.
*/
export function addItemPart(
requestOptions?: IItemPartOptions
): Promise<IUpdateItemResponse> {
const partNum = requestOptions.partNum;

if (!Number.isInteger(partNum) || partNum < 1 || partNum > 10000) {
return Promise.reject(new Error('The part number must be an integer between 1 to 10000, inclusive.'))
}

return determineOwner(requestOptions).then(owner => {
// AGO adds the "partNum" parameter in the query string, not in the body
const url = `${getPortalUrl(requestOptions)}/content/users/${owner}/items/${
requestOptions.id
}/addPart?partNum=${partNum}`;

const options = appendCustomParams<IItemPartOptions>(
requestOptions,
["file"],
{ params: { ...requestOptions.params } }
);

return request(url, options);
});
}

/**
* ```js
* import { commitItemUpload } from "@esri/arcgis-rest-portal";
Expand All @@ -22,19 +71,29 @@ import {
* ```
* Commit is called once all parts are uploaded during a multipart Add Item or Update Item operation. See the [REST Documentation](https://developers.arcgis.com/rest/users-groups-and-items/commit.htm) for more information.
*
* @param id - The Id of the item to commit upload for.
* @param requestOptions - Options for the request
* @returns A Promise to get the commit result.
*/
export function commitItemUpload(
requestOptions?: IUserItemOptions
requestOptions?: ICommitItemOptions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a breaking change, which is fine if it didn't actually work before.

if it did work before, then we should try to support the previous API and do something like requestOptions?: ICommitItemOptions | IUserItemOptions and then internally do something like const itemId = requestOptions.item ? requestOptions.item.id : requestOptions.id;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. This function didn't work before, but I agree with your approach to make it compatible with the previous code. Just in case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it didn't work it's ok to leave the breaking change as is

): Promise<IUpdateItemResponse> {
return determineOwner(requestOptions).then(owner => {
const url = `${getPortalUrl(requestOptions)}/content/users/${owner}/items/${
requestOptions.id
}/commit`;

return request(url, requestOptions);
const options = appendCustomParams<ICommitItemOptions>(
requestOptions,
[],
{
params: {
...requestOptions.params,
...serializeItem(requestOptions.item)
}
}
);

return request(url, options);
});
}

Expand All @@ -50,7 +109,6 @@ export function commitItemUpload(
* ```
* Cancels a multipart upload on an item. See the [REST Documentation](https://developers.arcgis.com/rest/users-groups-and-items/cancel.htm) for more information.
*
* @param id - The Id of the item to cancel upload for.
* @param requestOptions - Options for the request
* @returns A Promise to get the commit result.
*/
Expand Down
80 changes: 1 addition & 79 deletions packages/arcgis-rest-portal/test/items/add.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ import { attachmentFile } from "../../../arcgis-rest-feature-layer/test/attachme
import {
addItemData,
addItemResource,
addItemRelationship,
addItemPart
addItemRelationship
} from "../../src/items/add";

import { ItemSuccessResponse } from "../mocks/items/item";
Expand Down Expand Up @@ -320,82 +319,5 @@ describe("search", () => {
fail(e);
});
});

it("should add a binary part to an item", done => {
fetchMock.once("*", {
success: true
});

const file = attachmentFile();

addItemPart({
id: "3ef",
// File() is only available in the browser
file,
partNum: 1,
...MOCK_USER_REQOPTS
})
.then(() => {
expect(fetchMock.called()).toEqual(true);
const [url, options]: [string, RequestInit] = fetchMock.lastCall("*");
expect(url).toEqual(
"https://myorg.maps.arcgis.com/sharing/rest/content/users/casey/items/3ef/addPart"
);
expect(options.method).toBe("POST");
expect(options.body instanceof FormData).toBeTruthy();
const params = options.body as FormData;

if (params.get) {
expect(params.get("token")).toEqual("fake-token");
expect(params.get("f")).toEqual("json");
expect(params.get("file")).toEqual(file);
expect(params.get("partNum")).toEqual("1");
}

done();
})
.catch(e => {
fail(e);
});
});

it("should add a binary part to an item with the owner parameter", done => {
fetchMock.once("*", {
success: true
});

const file = attachmentFile();

addItemPart({
id: "3ef",
owner: "joe",
// File() is only available in the browser
file,
partNum: 1,
...MOCK_USER_REQOPTS
})
.then(() => {
expect(fetchMock.called()).toEqual(true);
const [url, options]: [string, RequestInit] = fetchMock.lastCall("*");
expect(url).toEqual(
"https://myorg.maps.arcgis.com/sharing/rest/content/users/joe/items/3ef/addPart"
);
expect(options.method).toBe("POST");
expect(options.body instanceof FormData).toBeTruthy();
const params = options.body as FormData;

if (params.get) {
expect(params.get("token")).toEqual("fake-token");
expect(params.get("f")).toEqual("json");
expect(params.get("file")).toEqual(file);
expect(params.get("partNum")).toEqual("1");
}

done();
})
.catch(e => {
fail(e);
});
});
}); // auth requests
});
23 changes: 0 additions & 23 deletions packages/arcgis-rest-portal/test/items/create.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -372,29 +372,6 @@ describe("search", () => {
});
});

it("should throw an error for a file upload request with multipart=false", done => {
fetchMock.once("*", ItemSuccessResponse);
const fakeItem = {
owner: "casey",
title: "my fake item",
type: "Web Mapping Application"
};
createItemInFolder({
item: fakeItem,
file: "some file",
// multipart is required to be true for file upload
multipart: false,
...MOCK_USER_REQOPTS
})
.then(() => {
fail();
})
.catch(() => {
expect(fetchMock.called()).toEqual(false);
done();
});
});

it("should throw an error for a multipart request with no file name", done => {
fetchMock.once("*", ItemSuccessResponse);
const fakeItem = {
Expand Down
Loading