From 8ea3c7820befe018eb01291680f61f8d51921084 Mon Sep 17 00:00:00 2001 From: Hendrik de Graaf Date: Thu, 8 Oct 2020 17:30:50 +0200 Subject: [PATCH 1/7] feat(rest-api-link): set correct content-type for endpoints --- .../contentTypeForResource.test.ts | 19 +++++++++++++++++++ .../RestAPILink/contentTypeForResource.ts | 15 +++++++++++++++ .../RestAPILink/queryToRequestOptions.ts | 5 +++-- 3 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 services/data/src/links/RestAPILink/contentTypeForResource.test.ts create mode 100644 services/data/src/links/RestAPILink/contentTypeForResource.ts diff --git a/services/data/src/links/RestAPILink/contentTypeForResource.test.ts b/services/data/src/links/RestAPILink/contentTypeForResource.test.ts new file mode 100644 index 00000000..9d7fb8aa --- /dev/null +++ b/services/data/src/links/RestAPILink/contentTypeForResource.test.ts @@ -0,0 +1,19 @@ +import { contentTypeForResource } from './contentTypeForResource' + +describe('contentTypeForResource', () => { + it('returns text/plain for a string match', () => { + expect(contentTypeForResource('messageConversations/feedback')).toMatch( + 'text/plain' + ) + }) + it('returns text/plain for a regex match', () => { + expect( + contentTypeForResource('messageConversations/g8mcplgksV2/priority') + ).toMatch('text/plain') + }) + it('returns application/json for non-matches', () => { + expect(contentTypeForResource('nothing/to/match')).toMatch( + 'application/json' + ) + }) +}) diff --git a/services/data/src/links/RestAPILink/contentTypeForResource.ts b/services/data/src/links/RestAPILink/contentTypeForResource.ts new file mode 100644 index 00000000..78fdcd47 --- /dev/null +++ b/services/data/src/links/RestAPILink/contentTypeForResource.ts @@ -0,0 +1,15 @@ +const RESOURCES_EXPECTING_TEXT_PLAIN_POSTS: Array = [ + 'messageConversations/feedback', + /^messageConversations\/[a-zA-Z0-9]{11}\/priority$/, +] + +export const contentTypeForResource = (resource: string) => { + const isResourceExpectingPlainText = RESOURCES_EXPECTING_TEXT_PLAIN_POSTS.some( + textPlainResource => + textPlainResource instanceof RegExp + ? textPlainResource.test(resource) + : textPlainResource === resource + ) + + return isResourceExpectingPlainText ? 'text/plain' : 'application/json' +} diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions.ts b/services/data/src/links/RestAPILink/queryToRequestOptions.ts index be8b6982..b329630e 100644 --- a/services/data/src/links/RestAPILink/queryToRequestOptions.ts +++ b/services/data/src/links/RestAPILink/queryToRequestOptions.ts @@ -1,4 +1,5 @@ import { ResolvedResourceQuery, FetchType } from '../../engine' +import { contentTypeForResource } from './contentTypeForResource' const getMethod = (type: FetchType): string => { switch (type) { @@ -17,14 +18,14 @@ const getMethod = (type: FetchType): string => { export const queryToRequestOptions = ( type: FetchType, - { data }: ResolvedResourceQuery, + { data, resource }: ResolvedResourceQuery, signal?: AbortSignal ): RequestInit => ({ method: getMethod(type), body: data ? JSON.stringify(data) : undefined, headers: data ? { - 'Content-Type': 'application/json', + 'Content-Type': contentTypeForResource(resource), } : undefined, signal, From 0f43acfe0c4d6e1fadcda3826d2f5856f73c11f1 Mon Sep 17 00:00:00 2001 From: Hendrik de Graaf Date: Wed, 21 Oct 2020 16:15:10 +0200 Subject: [PATCH 2/7] feat(rest-api-link): add support for text/plain and multipart/form-data --- .../contentTypeForResource.test.ts | 19 -- .../RestAPILink/contentTypeForResource.ts | 15 -- .../RestAPILink/queryToRequestHeaders.test.ts | 41 ++++ .../RestAPILink/queryToRequestHeaders.ts | 38 ++++ .../textPlainMatchers.test.ts | 203 ++++++++++++++++++ .../textPlainMatchers.ts | 112 ++++++++++ .../RestAPILink/queryToRequestOptions.ts | 12 +- 7 files changed, 398 insertions(+), 42 deletions(-) delete mode 100644 services/data/src/links/RestAPILink/contentTypeForResource.test.ts delete mode 100644 services/data/src/links/RestAPILink/contentTypeForResource.ts create mode 100644 services/data/src/links/RestAPILink/queryToRequestHeaders.test.ts create mode 100644 services/data/src/links/RestAPILink/queryToRequestHeaders.ts create mode 100644 services/data/src/links/RestAPILink/queryToRequestHeaders/textPlainMatchers.test.ts create mode 100644 services/data/src/links/RestAPILink/queryToRequestHeaders/textPlainMatchers.ts diff --git a/services/data/src/links/RestAPILink/contentTypeForResource.test.ts b/services/data/src/links/RestAPILink/contentTypeForResource.test.ts deleted file mode 100644 index 9d7fb8aa..00000000 --- a/services/data/src/links/RestAPILink/contentTypeForResource.test.ts +++ /dev/null @@ -1,19 +0,0 @@ -import { contentTypeForResource } from './contentTypeForResource' - -describe('contentTypeForResource', () => { - it('returns text/plain for a string match', () => { - expect(contentTypeForResource('messageConversations/feedback')).toMatch( - 'text/plain' - ) - }) - it('returns text/plain for a regex match', () => { - expect( - contentTypeForResource('messageConversations/g8mcplgksV2/priority') - ).toMatch('text/plain') - }) - it('returns application/json for non-matches', () => { - expect(contentTypeForResource('nothing/to/match')).toMatch( - 'application/json' - ) - }) -}) diff --git a/services/data/src/links/RestAPILink/contentTypeForResource.ts b/services/data/src/links/RestAPILink/contentTypeForResource.ts deleted file mode 100644 index 78fdcd47..00000000 --- a/services/data/src/links/RestAPILink/contentTypeForResource.ts +++ /dev/null @@ -1,15 +0,0 @@ -const RESOURCES_EXPECTING_TEXT_PLAIN_POSTS: Array = [ - 'messageConversations/feedback', - /^messageConversations\/[a-zA-Z0-9]{11}\/priority$/, -] - -export const contentTypeForResource = (resource: string) => { - const isResourceExpectingPlainText = RESOURCES_EXPECTING_TEXT_PLAIN_POSTS.some( - textPlainResource => - textPlainResource instanceof RegExp - ? textPlainResource.test(resource) - : textPlainResource === resource - ) - - return isResourceExpectingPlainText ? 'text/plain' : 'application/json' -} diff --git a/services/data/src/links/RestAPILink/queryToRequestHeaders.test.ts b/services/data/src/links/RestAPILink/queryToRequestHeaders.test.ts new file mode 100644 index 00000000..100a3a74 --- /dev/null +++ b/services/data/src/links/RestAPILink/queryToRequestHeaders.test.ts @@ -0,0 +1,41 @@ +import { + queryToRequestHeaders, + contentTypeForResource, +} from './queryToRequestHeaders' + +describe('queryToRequestHeaders', () => { + it('returns undefined if query.data is falsey', () => { + expect(queryToRequestHeaders('create', { resource: 'test' })).toEqual( + undefined + ) + }) + it('return a header object with Content-Type application/json if query.data is defined', () => { + expect( + queryToRequestHeaders('create', { resource: 'test', data: 'test' }) + ).toEqual({ 'Content-Type': 'application/json' }) + }) +}) + +describe('contentTypeForResource', () => { + it('returns "application/json" for a normal resource', () => { + expect( + contentTypeForResource('create', { resource: 'test', data: 'test' }) + ).toEqual('application/json') + }) + it('returns "multipart/form-data" if data contains a FormData instance', () => { + const data = new FormData() + data.append('testKey', 'testValue') + + expect( + contentTypeForResource('create', { resource: 'test', data }) + ).toEqual('multipart/form-data') + }) + it('returns "text/plain" for a specific resource', () => { + expect( + contentTypeForResource('create', { + resource: 'messageConversations/feedback', + data: 'test', + }) + ).toEqual('text/plain') + }) +}) diff --git a/services/data/src/links/RestAPILink/queryToRequestHeaders.ts b/services/data/src/links/RestAPILink/queryToRequestHeaders.ts new file mode 100644 index 00000000..3d8b29b5 --- /dev/null +++ b/services/data/src/links/RestAPILink/queryToRequestHeaders.ts @@ -0,0 +1,38 @@ +import { ResolvedResourceQuery, FetchType } from '../../engine' +import * as textPlainMatchers from './queryToRequestHeaders/textPlainMatchers' + +const resourceExpectsTextPlain = ( + type: FetchType, + query: ResolvedResourceQuery +) => + Object.values(textPlainMatchers).some(textPlainMatcher => + textPlainMatcher(type, query) + ) + +export const contentTypeForResource = ( + type: FetchType, + query: ResolvedResourceQuery +) => { + if (resourceExpectsTextPlain(type, query)) { + return 'text/plain' + } + + if (query.data instanceof FormData) { + return 'multipart/form-data' + } + + return 'application/json' +} + +export const queryToRequestHeaders = ( + type: FetchType, + query: ResolvedResourceQuery +) => { + if (!query.data) { + return undefined + } + + return { + 'Content-Type': contentTypeForResource(type, query), + } +} diff --git a/services/data/src/links/RestAPILink/queryToRequestHeaders/textPlainMatchers.test.ts b/services/data/src/links/RestAPILink/queryToRequestHeaders/textPlainMatchers.test.ts new file mode 100644 index 00000000..256a4833 --- /dev/null +++ b/services/data/src/links/RestAPILink/queryToRequestHeaders/textPlainMatchers.test.ts @@ -0,0 +1,203 @@ +import { + isReplyToMessageConversation, + isCreateFeedbackMessage, + isCreateOrUpdateInterpretation, + isCommentOnInterpretation, + isInterpretationCommentUpdate, + isAddOrUpdateSystemOrUserSetting, + addOrUpdateConfigurationProperty, + isMetadataPackageInstallation, +} from './textPlainMatchers' + +describe('isReplyToMessageConversation', () => { + it('retuns true for POST to `messageConversations/${id}`', () => { + expect( + isReplyToMessageConversation('create', { + resource: 'messageConversations/oXD88WWSQpR', + }) + ).toEqual(true) + }) + it('retuns false for a POST to a different resource', () => { + expect( + isReplyToMessageConversation('create', { + resource: 'test/oXD88WWSQpR', + }) + ).toEqual(false) + }) +}) + +describe('isCreateFeedbackMessage', () => { + it('returns true for a POST to "messageConversations/feedback"', () => { + expect( + isCreateFeedbackMessage('create', { + resource: 'messageConversations/feedback', + }) + ).toEqual(true) + }) + it('retuns false for a POST to a different resource', () => { + expect( + isCreateFeedbackMessage('create', { + resource: 'messageConversations/somethingelse', + }) + ).toEqual(false) + }) +}) + +describe('isCreateOrUpdateInterpretation', () => { + it('returns true for a POST to "interpretations/chart/${id}"', () => { + expect( + isCreateOrUpdateInterpretation('create', { + resource: 'interpretations/chart/oXD88WWSQpR', + }) + ).toEqual(true) + }) + it('returns true for a PUT to "interpretations/chart/${id}"', () => { + expect( + isCreateOrUpdateInterpretation('replace', { + resource: 'interpretations/chart/oXD88WWSQpR', + }) + ).toEqual(true) + }) + it('returns true for PUT with populated query.id', () => { + expect( + isCreateOrUpdateInterpretation('replace', { + resource: 'interpretations/chart', + id: 'oXD88WWSQpR', + }) + ).toEqual(true) + }) + it('retuns false for PATCH requests with a valid query', () => { + expect( + isCreateOrUpdateInterpretation('update', { + resource: 'interpretations/chart/oXD88WWSQpR', + }) + ).toEqual(false) + }) + it('returns false for a request to a different resource', () => { + expect( + isCreateOrUpdateInterpretation('create', { + resource: 'interpretations/dummy/oXD88WWSQpR', + }) + ).toEqual(false) + }) +}) + +describe('isCommentOnInterpretation', () => { + it('retuns true for POST to `interpretations/${id}/comments`', () => { + expect( + isCommentOnInterpretation('create', { + resource: 'interpretations/oXD88WWSQpR/comments', + }) + ).toEqual(true) + }) + it('retuns false for a POST to a different resource', () => { + expect( + isCommentOnInterpretation('create', { + resource: 'test/oXD88WWSQpR/comments', + }) + ).toEqual(false) + }) +}) + +describe('isInterpretationCommentUpdate', () => { + it('returns true for a PUT to `interpretations/${interpretationId}/comments/${commentId}`', () => { + expect( + isInterpretationCommentUpdate('replace', { + resource: 'interpretations/oXD88WWSQpR/comments/oXD88WWSQpR', + }) + ).toEqual(true) + }) + it('returns true for PUT with populated query.id', () => { + expect( + isInterpretationCommentUpdate('replace', { + resource: 'interpretations', + id: 'oXD88WWSQpR/comments/oXD88WWSQpR', + }) + ).toEqual(true) + expect( + isInterpretationCommentUpdate('replace', { + resource: 'interpretations/oXD88WWSQpR/comments', + id: 'oXD88WWSQpR', + }) + ).toEqual(true) + }) + it('retuns false for PATCH requests with a valid query', () => { + expect( + isInterpretationCommentUpdate('update', { + resource: 'interpretations/oXD88WWSQpR/comments/oXD88WWSQpR', + }) + ).toEqual(false) + }) + it('returns false for a request to a different resource', () => { + expect( + isInterpretationCommentUpdate('create', { + resource: 'interpretations/oXD88WWSQpR/dummy/oXD88WWSQpR', + }) + ).toEqual(false) + }) +}) + +describe('isAddOrUpdateSystemOrUserSetting', () => { + it('retuns true for POST to `systemSettings/${settingKey}`', () => { + expect( + isAddOrUpdateSystemOrUserSetting('create', { + resource: 'systemSettings/keyWhatever', + }) + ).toEqual(true) + }) + it('retuns true for POST to `userSettings/${settingKey}`', () => { + expect( + isAddOrUpdateSystemOrUserSetting('create', { + resource: 'userSettings/keyWhatever', + }) + ).toEqual(true) + }) + it('retuns false for a POST to a different resource', () => { + expect( + isAddOrUpdateSystemOrUserSetting('create', { + resource: 'test/keyWhatever', + }) + ).toEqual(false) + }) +}) + +describe('addOrUpdateConfigurationProperty', () => { + it('retuns true for POST to `configuration/${property}`', () => { + expect( + addOrUpdateConfigurationProperty('create', { + resource: 'configuration/whatever', + }) + ).toEqual(true) + }) + it('retuns false for POST to `configuration/corsWhitelist`, which needs "application/json"', () => { + expect( + addOrUpdateConfigurationProperty('create', { + resource: 'configuration/corsWhitelist', + }) + ).toEqual(false) + }) + it('retuns false for a POST to a different resource', () => { + expect( + addOrUpdateConfigurationProperty('create', { + resource: 'test/whatever', + }) + ).toEqual(false) + }) +}) + +describe('isMetadataPackageInstallation', () => { + it('returns true for a POST to "synchronization/metadataPull"', () => { + expect( + isMetadataPackageInstallation('create', { + resource: 'synchronization/metadataPull', + }) + ).toEqual(true) + }) + it('retuns false for a POST to a different resource', () => { + expect( + isMetadataPackageInstallation('create', { + resource: 'synchronization/somethingelse', + }) + ).toEqual(false) + }) +}) diff --git a/services/data/src/links/RestAPILink/queryToRequestHeaders/textPlainMatchers.ts b/services/data/src/links/RestAPILink/queryToRequestHeaders/textPlainMatchers.ts new file mode 100644 index 00000000..56e92018 --- /dev/null +++ b/services/data/src/links/RestAPILink/queryToRequestHeaders/textPlainMatchers.ts @@ -0,0 +1,112 @@ +import { ResolvedResourceQuery, FetchType } from '../../../engine' + +/* + * Requests that expect a "text/plain" Content-Type have been collected by scanning + * the developer documentation: + * https://docs.dhis2.org/master/en/developer/html/dhis2_developer_manual_full.html + * + * Note that currently it is not allowed to include an id property on a "create" + * mutation object. This means that currently the `id` will always be included in + * the resource property (string). If we decide to allow the `id` property for + * "create" mutation-objects, we will have to include additional checks below. + */ + +// POST to `messageConversations/${id}` (reply to a messagConversation) +export const isReplyToMessageConversation = ( + type: FetchType, + { resource }: ResolvedResourceQuery +) => { + const pattern = /^messageConversations\/[a-zA-Z0-9]{11}$/ + return type === 'create' && pattern.test(resource) +} + +// POST to 'messageConversations/feedback' (create a feedback message) +export const isCreateFeedbackMessage = ( + type: FetchType, + { resource }: ResolvedResourceQuery +) => type === 'create' && resource === 'messageConversations/feedback' + +// POST or PUT to `interpretations/${objectType}/${id}` (add or update an interpretation) +export const isCreateOrUpdateInterpretation = ( + type: FetchType, + { resource, id }: ResolvedResourceQuery +) => { + if (type !== 'create' && type !== 'replace') { + return false + } + + let resourcePattern + if (type === 'replace' && id) { + resourcePattern = /^interpretations\/(?:reportTable|chart|visualization|map|eventReport|eventChart|dataSetReport)$/ + const idPattern = /^[a-zA-Z0-9]{11}$/ + + return resourcePattern.test(resource) && idPattern.test(id) + } + + resourcePattern = /^interpretations\/(?:reportTable|chart|visualization|map|eventReport|eventChart|dataSetReport)\/[a-zA-Z0-9]{11}$/ + + return resourcePattern.test(resource) +} + +// POST to `interpretations/${id}/comments` (comment on an interpretation) +export const isCommentOnInterpretation = ( + type: FetchType, + { resource }: ResolvedResourceQuery +) => { + const pattern = /^interpretations\/[a-zA-Z0-9]{11}\/comments$/ + return type === 'create' && pattern.test(resource) +} + +// PUT to `interpretations/${interpretationId}/comments/${commentId}` +// (update an interpretation comment) +export const isInterpretationCommentUpdate = ( + type: FetchType, + { resource, id }: ResolvedResourceQuery +) => { + if (type !== 'replace') { + return false + } + + if (id) { + const idPatternLong = /^[a-zA-Z0-9]{11}\/comments\/[a-zA-Z0-9]{11}$/ + const idPatternShort = /^[a-zA-Z0-9]{11}$/ + const resourcePattern = /^interpretations\/[a-zA-Z0-9]{11}\/comments$/ + + return ( + (resource === 'interpretations' && idPatternLong.test(id)) || + (resourcePattern.test(resource) && idPatternShort.test(id)) + ) + } + + const pattern = /^interpretations\/[a-zA-Z0-9]{11}\/comments\/[a-zA-Z0-9]{11}$/ + return pattern.test(resource) +} + +// POST to `systemSettings/${settingKey}` or `userSettings/${settingKey}` +// (add or update a single system or user setting) +export const isAddOrUpdateSystemOrUserSetting = ( + type: FetchType, + { resource }: ResolvedResourceQuery +) => { + // At least 4 chars because the all start with 'key' (i.e. keyStyle) + const pattern = /^(?:systemSettings|userSettings)\/[a-zA-Z]{4,}$/ + return type === 'create' && pattern.test(resource) +} + +// POST to `configuration/${configurationProperty}` +// (add or update a single configuration property) +export const addOrUpdateConfigurationProperty = ( + type: FetchType, + { resource }: ResolvedResourceQuery +) => { + // NOTE: The corsWhitelist property does expect "application/json" + const pattern = /^(configuration)\/([a-zA-Z]{1,50})$/ + const match = resource.match(pattern) + return type === 'create' && !!match && match[2] !== 'corsWhitelist' +} + +// POST to 'synchronization/metadataPull' (install a metadata package) +export const isMetadataPackageInstallation = ( + type: FetchType, + { resource }: ResolvedResourceQuery +) => type === 'create' && resource === 'synchronization/metadataPull' diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions.ts b/services/data/src/links/RestAPILink/queryToRequestOptions.ts index b329630e..9d45dbac 100644 --- a/services/data/src/links/RestAPILink/queryToRequestOptions.ts +++ b/services/data/src/links/RestAPILink/queryToRequestOptions.ts @@ -1,5 +1,5 @@ import { ResolvedResourceQuery, FetchType } from '../../engine' -import { contentTypeForResource } from './contentTypeForResource' +import { queryToRequestHeaders } from './queryToRequestHeaders' const getMethod = (type: FetchType): string => { switch (type) { @@ -18,15 +18,11 @@ const getMethod = (type: FetchType): string => { export const queryToRequestOptions = ( type: FetchType, - { data, resource }: ResolvedResourceQuery, + query: ResolvedResourceQuery, signal?: AbortSignal ): RequestInit => ({ method: getMethod(type), - body: data ? JSON.stringify(data) : undefined, - headers: data - ? { - 'Content-Type': contentTypeForResource(resource), - } - : undefined, + body: query.data ? JSON.stringify(query.data) : undefined, + headers: queryToRequestHeaders(type, query), signal, }) From 0268ad71a5001a407405857acce01dea15cb9f9e Mon Sep 17 00:00:00 2001 From: Hendrik de Graaf Date: Thu, 22 Oct 2020 11:45:29 +0200 Subject: [PATCH 3/7] feat(rest-api-link): adjust request body according to Content-Type --- .../RestAPILink/queryToRequestHeaders.test.ts | 41 --------- .../RestAPILink/queryToRequestHeaders.ts | 38 --------- .../RestAPILink/queryToRequestOptions.ts | 22 +++-- .../multipartFormDataMatchers.test.ts | 45 ++++++++++ .../multipartFormDataMatchers.ts | 22 +++++ .../requestContentType.test.ts | 83 ++++++++++++++++++ .../requestContentType.ts | 84 +++++++++++++++++++ .../textPlainMatchers.test.ts | 0 .../textPlainMatchers.ts | 0 9 files changed, 249 insertions(+), 86 deletions(-) delete mode 100644 services/data/src/links/RestAPILink/queryToRequestHeaders.test.ts delete mode 100644 services/data/src/links/RestAPILink/queryToRequestHeaders.ts create mode 100644 services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.test.ts create mode 100644 services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.ts create mode 100644 services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.test.ts create mode 100644 services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts rename services/data/src/links/RestAPILink/{queryToRequestHeaders => queryToRequestOptions}/textPlainMatchers.test.ts (100%) rename services/data/src/links/RestAPILink/{queryToRequestHeaders => queryToRequestOptions}/textPlainMatchers.ts (100%) diff --git a/services/data/src/links/RestAPILink/queryToRequestHeaders.test.ts b/services/data/src/links/RestAPILink/queryToRequestHeaders.test.ts deleted file mode 100644 index 100a3a74..00000000 --- a/services/data/src/links/RestAPILink/queryToRequestHeaders.test.ts +++ /dev/null @@ -1,41 +0,0 @@ -import { - queryToRequestHeaders, - contentTypeForResource, -} from './queryToRequestHeaders' - -describe('queryToRequestHeaders', () => { - it('returns undefined if query.data is falsey', () => { - expect(queryToRequestHeaders('create', { resource: 'test' })).toEqual( - undefined - ) - }) - it('return a header object with Content-Type application/json if query.data is defined', () => { - expect( - queryToRequestHeaders('create', { resource: 'test', data: 'test' }) - ).toEqual({ 'Content-Type': 'application/json' }) - }) -}) - -describe('contentTypeForResource', () => { - it('returns "application/json" for a normal resource', () => { - expect( - contentTypeForResource('create', { resource: 'test', data: 'test' }) - ).toEqual('application/json') - }) - it('returns "multipart/form-data" if data contains a FormData instance', () => { - const data = new FormData() - data.append('testKey', 'testValue') - - expect( - contentTypeForResource('create', { resource: 'test', data }) - ).toEqual('multipart/form-data') - }) - it('returns "text/plain" for a specific resource', () => { - expect( - contentTypeForResource('create', { - resource: 'messageConversations/feedback', - data: 'test', - }) - ).toEqual('text/plain') - }) -}) diff --git a/services/data/src/links/RestAPILink/queryToRequestHeaders.ts b/services/data/src/links/RestAPILink/queryToRequestHeaders.ts deleted file mode 100644 index 3d8b29b5..00000000 --- a/services/data/src/links/RestAPILink/queryToRequestHeaders.ts +++ /dev/null @@ -1,38 +0,0 @@ -import { ResolvedResourceQuery, FetchType } from '../../engine' -import * as textPlainMatchers from './queryToRequestHeaders/textPlainMatchers' - -const resourceExpectsTextPlain = ( - type: FetchType, - query: ResolvedResourceQuery -) => - Object.values(textPlainMatchers).some(textPlainMatcher => - textPlainMatcher(type, query) - ) - -export const contentTypeForResource = ( - type: FetchType, - query: ResolvedResourceQuery -) => { - if (resourceExpectsTextPlain(type, query)) { - return 'text/plain' - } - - if (query.data instanceof FormData) { - return 'multipart/form-data' - } - - return 'application/json' -} - -export const queryToRequestHeaders = ( - type: FetchType, - query: ResolvedResourceQuery -) => { - if (!query.data) { - return undefined - } - - return { - 'Content-Type': contentTypeForResource(type, query), - } -} diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions.ts b/services/data/src/links/RestAPILink/queryToRequestOptions.ts index 9d45dbac..58ddfb90 100644 --- a/services/data/src/links/RestAPILink/queryToRequestOptions.ts +++ b/services/data/src/links/RestAPILink/queryToRequestOptions.ts @@ -1,5 +1,9 @@ import { ResolvedResourceQuery, FetchType } from '../../engine' -import { queryToRequestHeaders } from './queryToRequestHeaders' +import { + requestContentType, + requestBodyForContentType, + requestHeadersForContentType, +} from './queryToRequestOptions/requestContentType' const getMethod = (type: FetchType): string => { switch (type) { @@ -20,9 +24,13 @@ export const queryToRequestOptions = ( type: FetchType, query: ResolvedResourceQuery, signal?: AbortSignal -): RequestInit => ({ - method: getMethod(type), - body: query.data ? JSON.stringify(query.data) : undefined, - headers: queryToRequestHeaders(type, query), - signal, -}) +): RequestInit => { + const contentType = requestContentType(type, query) + + return { + method: getMethod(type), + body: requestBodyForContentType(contentType, query), + headers: requestHeadersForContentType(contentType), + signal, + } +} diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.test.ts b/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.test.ts new file mode 100644 index 00000000..2ba7f93a --- /dev/null +++ b/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.test.ts @@ -0,0 +1,45 @@ +import { + fileResourceUpload, + isStaticContentUpload, +} from './multipartFormDataMatchers' + +describe('fileResourceUpload', () => { + it('returns true for a POST to "fileResources"', () => { + expect( + fileResourceUpload('create', { + resource: 'fileResources', + }) + ).toEqual(true) + }) + it('retuns false for a POST to a different resource', () => { + expect( + fileResourceUpload('create', { + resource: 'notFileResources', + }) + ).toEqual(false) + }) +}) + +describe('isStaticContentUpload', () => { + it('returns true for a POST to "staticContent/logo_banner"', () => { + expect( + isStaticContentUpload('create', { + resource: 'staticContent/logo_banner', + }) + ).toEqual(true) + }) + it('returns true for a POST to "staticContent/logo_front"', () => { + expect( + isStaticContentUpload('create', { + resource: 'staticContent/logo_front', + }) + ).toEqual(true) + }) + it('returns false for a request to a different resource', () => { + expect( + isStaticContentUpload('create', { + resource: 'staticContent/no_logo', + }) + ).toEqual(false) + }) +}) diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.ts b/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.ts new file mode 100644 index 00000000..0c42962f --- /dev/null +++ b/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.ts @@ -0,0 +1,22 @@ +import { ResolvedResourceQuery, FetchType } from '../../../engine' + +/* + * Requests that expect a "multipart/form-data" Content-Type have been collected by scanning + * the developer documentation: + * https://docs.dhis2.org/master/en/developer/html/dhis2_developer_manual_full.html + */ + +// POST to 'fileResources' (upload a file resource) +export const fileResourceUpload = ( + type: FetchType, + { resource }: ResolvedResourceQuery +) => type === 'create' && resource === 'fileResources' + +// POST to `staticContent/${key}` (upload staticContent: logo_banner | logo_front) +export const isStaticContentUpload = ( + type: FetchType, + { resource }: ResolvedResourceQuery +) => { + const pattern = /^staticContent\/(?:logo_banner|logo_front)$/ + return type === 'create' && pattern.test(resource) +} diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.test.ts b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.test.ts new file mode 100644 index 00000000..f1508a20 --- /dev/null +++ b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.test.ts @@ -0,0 +1,83 @@ +import { + requestContentType, + requestHeadersForContentType, + requestBodyForContentType, +} from './requestContentType' + +describe('requestContentType', () => { + it('returns "application/json" for a normal resource', () => { + expect( + requestContentType('create', { resource: 'test', data: 'test' }) + ).toEqual('application/json') + }) + it('returns "multipart/form-data" for a specific resource that expects it', () => { + expect( + requestContentType('create', { + resource: 'fileResources', + data: 'test', + }) + ).toEqual('multipart/form-data') + }) + it('returns "text/plain" for a specific resource that expects it', () => { + expect( + requestContentType('create', { + resource: 'messageConversations/feedback', + data: 'test', + }) + ).toEqual('text/plain') + }) +}) + +describe('requestHeadersForContentType', () => { + it('returns undefined if contentType is null', () => { + expect(requestHeadersForContentType(null)).toEqual(undefined) + }) + it('returns a headers object with the contentType supplied to it', () => { + const contentType = 'application/json' + expect(requestHeadersForContentType(contentType)).toEqual({ + 'Content-Type': contentType, + }) + }) +}) + +describe('requestBodyForContentType', () => { + it('returns undefined if data is undefined', () => { + expect( + requestBodyForContentType('application/json', { resource: 'test' }) + ).toEqual(undefined) + }) + it('JSON stringifies the data if contentType is "application/json"', () => { + const dataIn = { a: 'AAAA', b: 1, c: true } + const dataOut = JSON.stringify(dataIn) + + expect( + requestBodyForContentType('application/json', { + resource: 'test', + data: dataIn, + }) + ).toEqual(dataOut) + }) + it('converts to FormData if contentType is "multipart/form-data"', () => { + const file = new File(['foo'], 'foo.txt', { type: 'text/plain' }) + const data = { a: 'AAA', file } + + const result = requestBodyForContentType('multipart/form-data', { + resource: 'test', + data, + }) + + expect(result instanceof FormData).toEqual(true) + expect(result.get('a')).toEqual('AAA') + expect(result.get('file')).toEqual(file) + }) + it('returns the data as received if contentType is "text/plain"', () => { + const data = 'Something' + + expect( + requestBodyForContentType('text/plain', { + resource: 'messageConversations/feedback', + data, + }) + ).toEqual(data) + }) +}) diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts new file mode 100644 index 00000000..106d0e7c --- /dev/null +++ b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts @@ -0,0 +1,84 @@ +import { ResolvedResourceQuery, FetchType } from '../../../engine' +import * as textPlainMatchers from './textPlainMatchers' +import * as multipartFormDataMatchers from './multipartFormDataMatchers' + +type RequestContentType = + | 'application/json' + | 'text/plain' + | 'multipart/form-data' + | null + +type FormData = { + [key: string]: string | Blob +} + +const resourceExpectsTextPlain = ( + type: FetchType, + query: ResolvedResourceQuery +) => + Object.values(textPlainMatchers).some(textPlainMatcher => + textPlainMatcher(type, query) + ) + +const resourceExpectsMultipartFormData = ( + type: FetchType, + query: ResolvedResourceQuery +) => + Object.values(multipartFormDataMatchers).some(multipartFormDataMatcher => + multipartFormDataMatcher(type, query) + ) + +export const requestContentType = ( + type: FetchType, + query: ResolvedResourceQuery +) => { + if (!query.data) { + return null + } + + if (resourceExpectsTextPlain(type, query)) { + return 'text/plain' + } + + if (resourceExpectsMultipartFormData(type, query)) { + return 'multipart/form-data' + } + + return 'application/json' +} + +export const requestHeadersForContentType = ( + contentType: RequestContentType +) => { + if (!contentType) { + return undefined + } + + return { 'Content-Type': contentType } +} + +export const requestBodyForContentType = ( + contentType: RequestContentType, + { data }: ResolvedResourceQuery +) => { + if (typeof data === 'undefined') { + return undefined + } + + if (contentType === 'application/json') { + return JSON.stringify(data) + } + + if (contentType === 'multipart/form-data') { + return Object.entries(data as FormData).reduce( + (formData, [key, value]) => { + formData.append(key, value) + return formData + }, + new FormData() + ) + } + + // 'text/plain' + return data +} diff --git a/services/data/src/links/RestAPILink/queryToRequestHeaders/textPlainMatchers.test.ts b/services/data/src/links/RestAPILink/queryToRequestOptions/textPlainMatchers.test.ts similarity index 100% rename from services/data/src/links/RestAPILink/queryToRequestHeaders/textPlainMatchers.test.ts rename to services/data/src/links/RestAPILink/queryToRequestOptions/textPlainMatchers.test.ts diff --git a/services/data/src/links/RestAPILink/queryToRequestHeaders/textPlainMatchers.ts b/services/data/src/links/RestAPILink/queryToRequestOptions/textPlainMatchers.ts similarity index 100% rename from services/data/src/links/RestAPILink/queryToRequestHeaders/textPlainMatchers.ts rename to services/data/src/links/RestAPILink/queryToRequestOptions/textPlainMatchers.ts From 4048fff2b2853511c58ede4afbb9966e7fd989b8 Mon Sep 17 00:00:00 2001 From: Hendrik de Graaf Date: Thu, 22 Oct 2020 13:08:46 +0200 Subject: [PATCH 4/7] fix(rest-api-link): throw error if data can't be converted to form-data --- .../requestContentType.test.ts | 9 +++++++ .../requestContentType.ts | 24 +++++++++++++------ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.test.ts b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.test.ts index f1508a20..222fbf88 100644 --- a/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.test.ts +++ b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.test.ts @@ -2,6 +2,7 @@ import { requestContentType, requestHeadersForContentType, requestBodyForContentType, + FORM_DATA_ERROR_MSG, } from './requestContentType' describe('requestContentType', () => { @@ -70,6 +71,14 @@ describe('requestBodyForContentType', () => { expect(result.get('a')).toEqual('AAA') expect(result.get('file')).toEqual(file) }) + it('throws an error if contentType is "multipart/form-data" and data does have own string-keyd properties', () => { + expect(() => { + requestBodyForContentType('multipart/form-data', { + resource: 'test', + data: new File(['foo'], 'foo.txt', { type: 'text/plain' }), + }) + }).toThrow(new Error(FORM_DATA_ERROR_MSG)) + }) it('returns the data as received if contentType is "text/plain"', () => { const data = 'Something' diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts index 106d0e7c..094fe31e 100644 --- a/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts +++ b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts @@ -28,6 +28,22 @@ const resourceExpectsMultipartFormData = ( multipartFormDataMatcher(type, query) ) +export const FORM_DATA_ERROR_MSG = + 'Could not convert data to FormData: object does not have own enumerable string-keyed properties' + +const convertToFormData = (data: FormData) => { + const dataEntries = Object.entries(data) + + if (dataEntries.length === 0) { + throw new Error(FORM_DATA_ERROR_MSG) + } + + return dataEntries.reduce((formData, [key, value]) => { + formData.append(key, value) + return formData + }, new FormData()) +} + export const requestContentType = ( type: FetchType, query: ResolvedResourceQuery @@ -70,13 +86,7 @@ export const requestBodyForContentType = ( } if (contentType === 'multipart/form-data') { - return Object.entries(data as FormData).reduce( - (formData, [key, value]) => { - formData.append(key, value) - return formData - }, - new FormData() - ) + return convertToFormData(data) } // 'text/plain' From 207f55ca52ae0b706bc147200c4a0de705288a41 Mon Sep 17 00:00:00 2001 From: Hendrik de Graaf Date: Thu, 22 Oct 2020 16:43:52 +0200 Subject: [PATCH 5/7] fix(rest-api-link): prevent boundary error for multipart/form-data --- .../requestContentType.test.ts | 17 +++++++++++++---- .../queryToRequestOptions/requestContentType.ts | 9 ++++++++- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.test.ts b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.test.ts index 222fbf88..46e11303 100644 --- a/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.test.ts +++ b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.test.ts @@ -33,10 +33,19 @@ describe('requestHeadersForContentType', () => { it('returns undefined if contentType is null', () => { expect(requestHeadersForContentType(null)).toEqual(undefined) }) - it('returns a headers object with the contentType supplied to it', () => { - const contentType = 'application/json' - expect(requestHeadersForContentType(contentType)).toEqual({ - 'Content-Type': contentType, + it('returns undefined if contentType is "multipart/form-data"', () => { + expect(requestHeadersForContentType('multipart/form-data')).toEqual( + undefined + ) + }) + it('returns a headers object with the contentType for "application/json"', () => { + expect(requestHeadersForContentType('application/json')).toEqual({ + 'Content-Type': 'application/json', + }) + }) + it('returns a headers object with the contentType for "text/plain"', () => { + expect(requestHeadersForContentType('text/plain')).toEqual({ + 'Content-Type': 'text/plain', }) }) }) diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts index 094fe31e..1791eca6 100644 --- a/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts +++ b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts @@ -66,7 +66,14 @@ export const requestContentType = ( export const requestHeadersForContentType = ( contentType: RequestContentType ) => { - if (!contentType) { + /* + * Explicitely setting Content-Type to 'multipart/form-data' produces + * a "multipart boundary not found" error. By not setting a Content-Type + * the browser will correctly set it for us and also apply multipart + * boundaries if the request body is an instance of FormData + * See https://stackoverflow.com/a/39281156/1143502 + */ + if (!contentType || contentType === 'multipart/form-data') { return undefined } From 5afa335908158873c0016b802452d7066c985c87 Mon Sep 17 00:00:00 2001 From: Hendrik de Graaf Date: Thu, 22 Oct 2020 17:24:52 +0200 Subject: [PATCH 6/7] fix(rest-api-link): add additional multipart/form-data endpoints --- .../multipartFormDataMatchers.test.ts | 44 +++++++++++++++++-- .../multipartFormDataMatchers.ts | 14 +++++- 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.test.ts b/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.test.ts index 2ba7f93a..61ec2d24 100644 --- a/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.test.ts +++ b/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.test.ts @@ -1,25 +1,44 @@ import { - fileResourceUpload, + isFileResourceUpload, + isMessageConversationAttachment, isStaticContentUpload, + isAppInstall, } from './multipartFormDataMatchers' -describe('fileResourceUpload', () => { +describe('isFileResourceUpload', () => { it('returns true for a POST to "fileResources"', () => { expect( - fileResourceUpload('create', { + isFileResourceUpload('create', { resource: 'fileResources', }) ).toEqual(true) }) it('retuns false for a POST to a different resource', () => { expect( - fileResourceUpload('create', { + isFileResourceUpload('create', { resource: 'notFileResources', }) ).toEqual(false) }) }) +describe('isMessageConversationAttachment', () => { + it('returns true for a POST to "messageConversations/attachments"', () => { + expect( + isMessageConversationAttachment('create', { + resource: 'messageConversations/attachments', + }) + ).toEqual(true) + }) + it('retuns false for a POST to a different resource', () => { + expect( + isMessageConversationAttachment('create', { + resource: 'messageConversations/notAttachments', + }) + ).toEqual(false) + }) +}) + describe('isStaticContentUpload', () => { it('returns true for a POST to "staticContent/logo_banner"', () => { expect( @@ -43,3 +62,20 @@ describe('isStaticContentUpload', () => { ).toEqual(false) }) }) + +describe('isAppInstall', () => { + it('returns true for a POST to "fileResources"', () => { + expect( + isAppInstall('create', { + resource: 'apps', + }) + ).toEqual(true) + }) + it('retuns false for a POST to a different resource', () => { + expect( + isAppInstall('create', { + resource: 'notApps', + }) + ).toEqual(false) + }) +}) diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.ts b/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.ts index 0c42962f..af767fbf 100644 --- a/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.ts +++ b/services/data/src/links/RestAPILink/queryToRequestOptions/multipartFormDataMatchers.ts @@ -7,11 +7,17 @@ import { ResolvedResourceQuery, FetchType } from '../../../engine' */ // POST to 'fileResources' (upload a file resource) -export const fileResourceUpload = ( +export const isFileResourceUpload = ( type: FetchType, { resource }: ResolvedResourceQuery ) => type === 'create' && resource === 'fileResources' +// POST to 'messageConversations/attachments' (upload a message conversation attachment) +export const isMessageConversationAttachment = ( + type: FetchType, + { resource }: ResolvedResourceQuery +) => type === 'create' && resource === 'messageConversations/attachments' + // POST to `staticContent/${key}` (upload staticContent: logo_banner | logo_front) export const isStaticContentUpload = ( type: FetchType, @@ -20,3 +26,9 @@ export const isStaticContentUpload = ( const pattern = /^staticContent\/(?:logo_banner|logo_front)$/ return type === 'create' && pattern.test(resource) } + +// POST to 'apps' (install an app) +export const isAppInstall = ( + type: FetchType, + { resource }: ResolvedResourceQuery +) => type === 'create' && resource === 'apps' From 24dc64c869434b030ca70a2fd4154f7169b4dacd Mon Sep 17 00:00:00 2001 From: Hendrik de Graaf Date: Tue, 27 Oct 2020 11:07:16 +0100 Subject: [PATCH 7/7] fix(rest-api-link): adjust types for convertToFormData (object>FromData) --- .../RestAPILink/queryToRequestOptions/requestContentType.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts index 1791eca6..ff9c23c7 100644 --- a/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts +++ b/services/data/src/links/RestAPILink/queryToRequestOptions/requestContentType.ts @@ -8,10 +8,6 @@ type RequestContentType = | 'multipart/form-data' | null -type FormData = { - [key: string]: string | Blob -} - const resourceExpectsTextPlain = ( type: FetchType, query: ResolvedResourceQuery @@ -31,7 +27,7 @@ const resourceExpectsMultipartFormData = ( export const FORM_DATA_ERROR_MSG = 'Could not convert data to FormData: object does not have own enumerable string-keyed properties' -const convertToFormData = (data: FormData) => { +const convertToFormData = (data: Record): FormData => { const dataEntries = Object.entries(data) if (dataEntries.length === 0) {