feat(rest-api-link): add support for text/plain and multipart/form-data

[HendrikThePendric]

This PR adds functionality to the RestAPILink to support requests that require a non-standard Content-Type header and body. This support is fully automatic: app runtime will analyse the request type and query and determine whether the request is one of the following types, and set headers and body accordingly:

Type	headers	body
application/json (default)	{ 'Content-Type': 'application/json' }	JSON.stringify(query.data)
text/plain	{ 'Content-Type': 'text/pain' }	query.data
multipart/form-data	undefined*	query.data converted to FormData

[*] The browser will deduct the correct Content-Type from the FormData body and will also inject the multipart-boundaries into the request body.

Below are some examples of mutation objects that will lead to different types of requests. This is mainly to illustrate that an app developer can just declare mutation objects in virtually the same way for each type of request.

/* Passing this to `useDataMutation` would lead to a request 
   with a headers object with Content-Type 'application/json' 
   and JSON stringified data as the request body */
const applicationJsonMutation = {
    resource: 'indicators',
    type: 'create',
    data: {
        // indicator props
    }
}

/* Passing this to `useDataMutation` would lead to a request 
   with a headers object with Content-Type 'text/plain' 
   and 'Message' as the request body */
const textPlainMutation = {
    resource: 'messageConversations/feedback',
    type: 'create',
    params: { 
        subject: 'Subject line'
    },
    data: 'Message',
}

/* Passing this to `useDataMutation` would lead to a request 
   with an undefined headers object, which the browser will
   convert to one with Content-Type 'multipart/form-data' 
   and in the request body we will find FormData containing 
   a 'file' field that holds the File instance */
const multipartFormDataMutation = {
    resource: 'fileResources',
    type: 'create',
    params: {
        domain: 'MESSAGE_ATTACHMENT',
    },
    data: {
        file: attachment, // an actual File instance
    },
}

The requests that need alternative handling have been taken from the docs and the following have been implemented in this PR:

text/plain

• POST to messageConversations/${id} (reply to a messagConversation)
• POST to messageConversations/feedback (create a feedback message)
• POST|PUT to interpretations/${objectType}/${id} (add or update an interpretation)
• POST to interpretations/${id}/comments (comment on an interpretation)
• POST to systemSettings/${settingKey}
• POST to userSettings/${settingKey}
• POST to configuration/${configurationProperty}
• POST to synchronization/metadataPull (install a metadata package)

multipart/form-data

• POST to fileResources (upload a file resource)
• POST to messageConversations/attachments (upload a message conversation attachment)
• POST to staticContent/${key} (upload staticContent: logo_banner | logo_front)
• POST to apps (install an app)

[amcgee]

This looks awesome, thanks @HendrikThePendric!

I think we need to also analyze id (in addition to resource), I'll take another look at this later today or next week.

[Mohammer5]

I like the current solution but would like to add something to what @amcgee already said in his comment:

Could we introduce an id placeholder in the resource string? Internally we could check if the placeholder is present and

• replace it with the id
• throw an error if no id has been provided
• or append the id if no placeholder was found but an id has been provided

It'd have the advantage that we wouldn't have to use regular expressions in the array of plain-text resources and we wouldn't have to add a resource string to the id in the query itself:

{
  "resource": "messageConversations/{{id}}/priority",
  "id": "g8mcplgksV2"
}

---

I wouldn't mind creating a PR for this

[HendrikThePendric]

I've had a week to reflect on this and I'm really not sure on this approach anymore. I've already expressed concerns reg. the maintenance burden this type of "micro-management" is going to produce in the long run. After stewing on this a bit longer I've also started to get some reservations about the level of "magic" we are introducing here. As a developer I personally would expect and prefer an API that just requests things with one specific content-type, unless I specifically instruct it to do it differently.

[varl]

From an app developer perspective I think this would be confusing. If I noted in the API docs that I needed to change the content-type of a request, then I would expect to do so in the context of where I am making the request.

That a library would handle that for me seems awkward and fragile in terms of maintenance.

Defaulting to application/json is a sane default, but I might want to get the XML content-type for my use case.

I would prefer if we could pass in a custom headers object instead of relying on a hardcoded map lookup.

Am I missing the intended use case here? This abstraction level in the app-runtime seems wrong to me.

---

edit

I misunderstood this entirely, and thought this was related to the response body and not the request body.

[Mohammer5]

I'd say that the intended abstraction level is very similar to what graphql is doing. Everything is 100% explicit and as the consumer of the library, you only have to worry about what you want to have, not how you're getting it.

In graphql this particular problem would be handled by the graphql server and its resolvers, which we obviously don't have, but by moving this to the library, we're creating a consistent solution for apps until either a graphql server in the middle exists or this has been addressed by the backend.

[HendrikThePendric]

I would prefer if we could pass in a custom headers object instead of relying on a hardcoded map lookup.

This would be my preferred approach as well, we could rewrite this function:

app-runtime/services/data/src/links/RestAPILink/queryToRequestOptions.ts

Lines 18 to 31 in af7b291

	export const queryToRequestOptions = (
	type: FetchType,
	{ data }: ResolvedResourceQuery,
	signal?: AbortSignal
	): RequestInit => ({
	method: getMethod(type),
	body: data ? JSON.stringify(data) : undefined,
	headers: data
	? {
	'Content-Type': 'application/json',
	}
	: undefined,
	signal,
	})

To something like this:

export const createHeaders = (headers, data) => {
    if (headers) {
        return headers
    }

    if (data) {
        return { 'Content-Type': 'application/json' }
    }
}

export const queryToRequestOptions = (
    type: FetchType,
    { data, headers }: ResolvedResourceQuery,
    signal?: AbortSignal
): RequestInit => ({
    method: getMethod(type),
    body: data ? JSON.stringify(data) : undefined,
    headers: createHeaders(headers, data)
    signal,
})

[amcgee]

I'd say that the intended abstraction level is very similar to what graphql is doing. Everything is 100% explicit and as the consumer of the library, you only have to worry about what you want to have, not how you're getting it.

This is exactly correct - the entire point of the data engine is to be declarative (what) rather than imperative (how) - headers are imperative, so they should not be exposed to the runtime consumer.

Additionally, it's important as we start to add data caching and offline capabilities that we don't leak HTTP request details to runtime consumers - soon there won't necessarily be a 1-to-1 query-to-request mapping (i.e. some fields might already exist from the respose to a previous request, the mutation might be applied in-memory while offline before synchronizing with HTTP requests after the fact)

On a purely practical level, if we require the runtime consumer to worry about API implementation details (and particularly details which are in fact bugs or omissions that will be changed later) we will run into a situation where we proliferate complex conditional and exception handling logic all over all our codebases. Placing this normalization layer within the data engine itself instead of in application code prevents us from having situations like this:

const mutation = {
   resource: 'messageConversations',
   type: 'create',
   body: 'xyz'
   params: ({ serverVersion }) => ({
     a: serverVersion.minor < 42 ? '2' : 2,
     z: 'y'
   })
   headers: ({ serverVersion }) => ((serverVersion.minor === 35 && serverVersion.patch < 2) || serverVersion.minor >= 36) ? undefined : { 'Content-Type': 'text' }
}

By moving this logic into the engine we consolidate all idiosyncrasies and inconsistancies of the API in one place, which gives us a running list of things that need to be fixed in the backend API without needing to care about them in applications or upstream libraries. Once they're fixed we can skip the transform step in the runtime (by checking the server version) without changing any application code. And once they're long-fixed, we can remove the transform completely.

As @Mohammer5 mentioned, we eventually want this to be a client-defined API on the other side of the network layer (for bandwidth optimization among other things), but for now the set of API normalization transforms should be applied as close to the HTTP network layer as possible in the data engine Rest API link.

[Mohammer5]

I think the imperative solution would be wrong here as we'd introduce two different paradigms to the library. That would lower the barrier to add more inconsistencies.

The idea is to be purely declarative. The issue we're having right now is that the endpoint is odd, so we'd have to fix it there. With the imperative approach @HendrikThePendric mentioned above, we'd introduce a fix for this in both the app and the app-runtime.

If I remember correctly, the purpose of the data service is to unify the api usage on the apps' side.

---

From an app developer perspective I think this would be confusing. If I noted in the API docs that I needed to change the content-type of a request, then I would expect to do so in the context of where I am making the request.

This is a fair point though (although @varl confused the response and request bodies), we should document the decision if we agree on handling this on the library's side

---

EDIT: @amcgee was faster 😬

[varl]

Haha, yeah. Thanks for the clarifications. Now that I know what we are talking about. I am onboard with the declarative approach and would prefer if the app-runtime handled this inconsistency in the API for me so I can focus on more important things. ;)

[HendrikThePendric]

Thanks for explaining @Mohammer5 and @amcgee. I've also had a quick chat about this with @varl and it has now become clear to me that this approach can add a lot of convenience, so the heavy maintenance burden and additional magic should be worth it.

Since this is all internal we can extend the approach gradually and start with something a bit like the current implementation. I think what I have currently is just not good enough because of what @amcgee mentioned in #651 (comment), and because it came to light in this Slack thread that both of the following mutation objects are valid (in theory):

const createMutation = {
    resource: `messageConversations/${messageConversationId}/status`,
    type: 'create',
    params: {
        messageConversationStatus: value,
    },
}

const updateMutation = {
    resource: `messageConversations`,
    id: `${messageConversationId}/status`,
    type: 'update',
    params: {
        messageConversationStatus: value,
    },
}

The current approach would not work correctly with the second mutation object. I'll try to find another approach that would accept both objects.

[HendrikThePendric]

So in 62fda33 I went with a pretty different approach to avoid problems like the one highlighted in #651 (comment). I think that matching against the path that we are actually going to use for the request we can avoid several problems:

• We don't have to worry about developers using the DataMutation API in unexpected ways
• We can add action paths
• In general, if we add more logic and/or exceptions in how the path is constructed in queryToResourcePath we don't have to update the logic in requestContentTypeForResource to match

Also I've had a chat with @Mohammer5 yesterday and we established that what is suggested here #651 (comment) could be very useful, but should be seen completely seperate from the issue at hand.

[Mohammer5]

I'm still not very fond of the current solution.
The proposed solution works on a derived value (resourcePath) instead of the query itself.
This adds some unnecessary complexity (e. g. removing the api path).

I'd prefer to check this through funcitons instead that receive the actual query:

const RESOURCES_EXPECTING_TEXT_PLAIN: Array<Function> = [
    query => query.resource === 'messageConversations/feedback',
    query =>
        query.resource === 'messageConversations'
        && query.id.match(/[a-zA-Z0-9]{11}\/priority$/),
]

export const requestContentTypeForResource = (query: Query) => {
    const isResourceExpectingPlainText = RESOURCES_EXPECTING_TEXT_PLAIN.some(
        plainTextCheck => plainTextCheck(query)
    )

    return isResourceExpectingPlainText ? 'text/plain' : 'application/json'
}

export const queryToRequestOptions = ({
    type,
    query,
    signal,
    resourcePath,
    apiPath,
}: Options): RequestInit => {
    const { data } = query
    return {
        method: getMethod(type),
        body: data ? JSON.stringify(data) : undefined,
        headers: data
            ? {
                  'Content-Type': requestContentTypeForResource(query),
              }
            : undefined,
        signal,
    }
}

[HendrikThePendric]

I agree the current implementation isn't the prettiest. Your suggestion of using functions instead of regexes/strings might enables us to tidy things up a bit, but there are some other problems which don't get addressed in the code you posted above. I'll try to explain things....

• We have a recommended way of doing mutations with an id parameter, but as it stands we don't enforce that. So a developer could chose to include the id within the resource string and things would still work. Also see feat(rest-api-link): add support for text/plain and multipart/form-data #651 (comment)
• In theory we could probably add a check to enforce devs to follow this recommended pattern, but it has also come to light that currently for some request we have to include the id into the resource string. Specifically this is for some POST requests, and the the id property is not allowed in mutation object of type create. Also see this Slack thread and the runtime docs

From the above we can conclude that currently there are two "valid" configurations we would have to allow for:

const flavourA = {
    resource: `messageConversations/${messageConversationId}/status`,
}
const flavourB = {
    resource: `messageConversations`,
    id: `${messageConversationId}/status`,
}

So the above was reason for me to move to my current approach where we don't have to consider this anymore....

Having said all of that, and reflecting on this a bit, I guess we could say that my latest commit has made the code a lot less readable, and the problem I was trying to address could also be addressed using your approach too. We would simply have to include some extra checks in cases such as these where we have multiple valid configs.

For example, this part:

query =>
        query.resource === 'messageConversations'
        && query.id.match(/[a-zA-Z0-9]{11}\/priority$/)

Would have to be changed to this:

query =>
        (query.resource === 'messageConversations' && /[a-zA-Z0-9]{11}\/priority$/.test(query.id))
        || /^messageConversations\/[a-zA-Z0-9]{11}\/priority$/.test(query.resource)

Probably, this is going to lead to more maintainable code, and quite likely this is more future proof as well.

So good suggestion @Mohammer5 , thanks 🙏

---

It's also come to light that we need a related fix to support file uploads. I'm going to incorporate that into this PR tomorrow.

[HendrikThePendric]

I've refactored things a lot and am now using using functions instead of regexes and strings, as @Mohammer5 advised. Also, I have gone through the developer docs and identified all occurrences where we would need "text/plain", so we should have all of these exceptions covered. And finally, I've also incorporated support for 'multipart/form-data' (and have updated the PR title accordingly). I'm pretty happy with the current state of the code, so a re-review would be appreciated.

[Mohammer5]

👍 👍 👍 👍 👍 👍 I love the descriptive functions ❤️

[amcgee]

Code looks good! Minor incorrect typing change - let me know if you want a hand figuring out the right static types here @HendrikThePendric

[amcgee]

Amazing 🎉 , great work @HendrikThePendric !

[dhis2-bot]

🎉 This PR is included in version 2.5.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀