fix: update createUpload to accept environment id in its params [INTEG-1492] #2030

ryunsong-contentful · 2023-11-01T21:26:55Z

Somewhat recently there was a CEP for updating the upload-api that was approved and a subsequent PR that was merged. This effectively means the Upload API now takes in both the Environment Id and Space Id for a given request.

ruderngespra

No expert on the changes that have been introduced to the upload endpoint, so happy to discuss this further async as well :)

ruderngespra · 2023-11-02T16:18:30Z

lib/create-environment-api.ts

@@ -1152,11 +1152,13 @@ export default function createEnvironmentApi(makeRequest: MakeRequest) {
     */
    createUpload: function createUpload(data: { file: string | ArrayBuffer | Stream }) {
      const raw = this.toPlainObject() as EnvironmentProps
+      const envId = raw.sys.aliasedEnvironment?.sys.id || ''


I think this is not targeting the public environment id. As far as I understand the code this either:

in the case that we are looking at an alias route, using the underlying environment id

in the case that no aliases are used, just providing an empty string.

Not sure about all the details of how the upload endpoint has been adjusted, but I am assuming it will always use either the alias-id or the environment-id, but never the environment-id that is being targeted by the alias, not empty string.

ruderngespra · 2023-11-02T16:27:28Z

I think this is the file that the upload endpoint is properly handled and that would need to be able to handle the environment as part of the route.

jsdalton · 2023-11-08T15:49:19Z

lib/adapters/REST/endpoints/upload.ts

 ) => {
  const httpUpload = getUploadHttpClient(http)
-
-  return raw.get(httpUpload, `/spaces/${params.spaceId}/uploads/${params.uploadId}`)
+  const spacePath = `/spaces/${params.spaceId}/uploads/${params.uploadId}`


This is the third repetition of the same code and I think it would not be premature to abstract it into a helper function (eg in this file)

lib/common-types.ts

lib/adapters/REST/endpoints/upload.ts

ruderngespra · 2023-11-09T11:09:33Z

lib/adapters/REST/endpoints/upload.ts

+  const path = params.environmentId ? environmentPath : spacePath
+
+  // Return path as is if uploadId is not provided, otherwise append it to the path
+  if (!params.uploadId) return path


If I understand this correctly, this now changes the behavior to also on post requests use the asset id as part of the route, whereas before we would never use the uploadId on post requests. Is that in accordance with the CEP changes?

Hmm... I don't think so. The path for the post route would look like /spaces/${params.spaceId}/uploads OR like /spaces/${params.spaceId}/environments/${params.environmentId}/uploads. So no asset ids, or upload ids in the route being added here. This is basically just a refactoring of the path logic since the post doesn't need an upload id but get and deletes do.

The asset itself is passed as a function parameter like this

const path = getUploadUrlPath(params) return raw.post(httpUpload, path, file, { headers: { 'Content-Type': 'application/octet-stream', }, }) }

👍🏻 But then we shouldn't add the uploadId to the path here, right?

Correct, const path = getUploadUrlPath(params) will ensure that the path will contain the upload id for get/delete but not for the create operation.

Got it. 👍🏻 A bit of a nit but I think I would prefer to have this reflected in the types of the create method then, basically the create method then should not have GetSpaceEnvironmentUploadParams as type, because it will never accept the optional uploadId (or if it did, it would result in a 400 api call), but only GetSpaceEnvironmentParams right?

Done, I agree, the create method should take in only the GetSpaceEnvironmentParams

Yeah I have a few cents to add here as well... I think the comments and the suggestions on the approach make a lot of sense.

My opinion is that we should not overload the method here to handle both uses cases but treat them as different things. This also makes the type definition itself a bit cleaner.

So to give you an example picked at random from the same folder we're in here (REST/endpoints):

contentful-management.js/lib/adapters/REST/endpoints/organization-membership.ts

Lines 15 to 19 in cebe70a

const getBaseUrl = (params: GetOrganizationParams) =>

`/organizations/${params.organizationId}/organization_memberships`

const getEntityUrl = (params: GetOrganizationMembershipParams) =>

`${getBaseUrl(params)}/${params.organizationMembershipId}`

In general this is true of every "restful" resource, there will always be two endpoints the "single" (for read/update/delete) and the "collection" (for create/query). So I suggest we embrace it, create two methods which have different input types, and then update GetSpaceEnvironmentUploadParams type to require the uploadid (so it can be reused throughout).

I think with this small change you might even end up with less lines of code, certainly one less conditional, and hopefully a cleaner solution.

Pretty small thing at the end of the day but i think worth getting to a nice clean state.

lib/common-types.ts

ryunsong-contentful · 2023-11-10T15:28:06Z

test/integration/asset-integration.js

@@ -99,7 +99,8 @@ describe('Asset api', function () {
      await unarchivedAsset.delete()
    })

-    test('Create and process asset with multiple locales', async () => {
+    // Skip because this is a flakey test


These are something the DX team will fix once they take ownership of this repo - ticket has been created in their backlog

ruderngespra

lgtm 🎉
I would double check this with the owners of the upload api as well though!

ruderngespra · 2023-11-10T15:53:40Z

lib/adapters/REST/endpoints/upload.ts

+  const path = params.environmentId ? environmentPath : spacePath
+
+  // Return path as is if uploadId is not provided, otherwise append it to the path
+  if (!params.uploadId) return path


Got it. 👍🏻 A bit of a nit but I think I would prefer to have this reflected in the types of the create method then, basically the create method then should not have GetSpaceEnvironmentUploadParams as type, because it will never accept the optional uploadId (or if it did, it would result in a 400 api call), but only GetSpaceEnvironmentParams right?

ruderngespra · 2023-11-10T15:54:11Z

lib/adapters/REST/endpoints/upload.ts

 export const create: RestEndpoint<'Upload', 'create'> = (
  http: AxiosInstance,
-  params: GetSpaceParams,
+  params: GetSpaceEnvironmentUploadParams,


Basically this should then be GetSpaceEnvironmentParams right?

Yes you are right!

lib/adapters/REST/endpoints/upload.ts

jsdalton

I had some more thoughts in this thread of discussion around the path method and types, LMK if makes sense!

jsdalton · 2023-11-10T16:41:57Z

lib/adapters/REST/endpoints/upload.ts

+  const path = params.environmentId ? environmentPath : spacePath
+
+  // Return path as is if uploadId is not provided, otherwise append it to the path
+  if (!params.uploadId) return path


Yeah I have a few cents to add here as well... I think the comments and the suggestions on the approach make a lot of sense.

My opinion is that we should not overload the method here to handle both uses cases but treat them as different things. This also makes the type definition itself a bit cleaner.

So to give you an example picked at random from the same folder we're in here (REST/endpoints):

contentful-management.js/lib/adapters/REST/endpoints/organization-membership.ts

Lines 15 to 19 in cebe70a

const getBaseUrl = (params: GetOrganizationParams) =>

`/organizations/${params.organizationId}/organization_memberships`

const getEntityUrl = (params: GetOrganizationMembershipParams) =>

`${getBaseUrl(params)}/${params.organizationMembershipId}`

In general this is true of every "restful" resource, there will always be two endpoints the "single" (for read/update/delete) and the "collection" (for create/query). So I suggest we embrace it, create two methods which have different input types, and then update GetSpaceEnvironmentUploadParams type to require the uploadid (so it can be reused throughout).

I think with this small change you might even end up with less lines of code, certainly one less conditional, and hopefully a cleaner solution.

Pretty small thing at the end of the day but i think worth getting to a nice clean state.

lib/common-types.ts

jsdalton

Thanks for the changes, look great! 😎

contentful-automation · 2023-11-13T15:35:26Z

🎉 This PR is included in version 11.5.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

contentful-automation · 2023-11-17T20:58:37Z

🎉 This issue has been resolved in version 11.5.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

…ms (#2030)" This reverts commit 4287e47.

…ms [INTEG-1492]" (#2084) * fix: revert "update createUpload to accept environment id in its params (#2030)" This reverts commit 4287e47. * fix: failing webhook integration test

ryunsong-contentful requested a review from a team as a code owner November 1, 2023 21:26

ryunsong-contentful changed the title ~~fix: update createUpload to accept environment id in its params [INTEG-1492~~ fix: update createUpload to accept environment id in its params [INTEG-1492] Nov 1, 2023

ruderngespra requested changes Nov 2, 2023

View reviewed changes

ryunsong-contentful added the Do Not Merge label Nov 3, 2023

ryunsong-contentful force-pushed the INTEG-1492 branch 4 times, most recently from 5f77acd to e613dc2 Compare November 8, 2023 15:44

jsdalton reviewed Nov 8, 2023

View reviewed changes

ryunsong-contentful force-pushed the INTEG-1492 branch 2 times, most recently from 15a9896 to c39d8a2 Compare November 8, 2023 16:10

jsdalton reviewed Nov 8, 2023

View reviewed changes

lib/common-types.ts Outdated Show resolved Hide resolved

ryunsong-contentful force-pushed the INTEG-1492 branch 2 times, most recently from ce7488e to 2b28653 Compare November 8, 2023 16:36

ruderngespra reviewed Nov 9, 2023

View reviewed changes

ryunsong-contentful force-pushed the INTEG-1492 branch 3 times, most recently from 9bcac61 to 018840f Compare November 9, 2023 22:03

ryunsong-contentful commented Nov 10, 2023

View reviewed changes

ruderngespra approved these changes Nov 10, 2023

View reviewed changes

ryunsong-contentful force-pushed the INTEG-1492 branch from 018840f to cebe70a Compare November 10, 2023 16:02

jsdalton requested changes Nov 10, 2023

View reviewed changes

ryunsong-contentful force-pushed the INTEG-1492 branch 2 times, most recently from 82a542f to 155bac7 Compare November 10, 2023 17:29

fix: update createUpload to accept environment id in its params

a58bfff

ryunsong-contentful force-pushed the INTEG-1492 branch from 155bac7 to a58bfff Compare November 10, 2023 17:32

jsdalton approved these changes Nov 10, 2023

View reviewed changes

ryunsong-contentful merged commit 4287e47 into master Nov 13, 2023
2 checks passed

ryunsong-contentful deleted the INTEG-1492 branch November 13, 2023 15:23

contentful-automation bot added the released label Nov 13, 2023

ryunsong-contentful removed the Do Not Merge label Nov 15, 2023

t-col added a commit that referenced this pull request Dec 5, 2023

Revert "fix: update createUpload to accept environment id in its para…

58f7846

…ms (#2030)" This reverts commit 4287e47.

t-col mentioned this pull request Dec 5, 2023

fix: revert "update createUpload to accept environment id in its params [INTEG-1492]" #2084

Merged

t-col added a commit that referenced this pull request Dec 5, 2023

fix: revert "update createUpload to accept environment id in its para…

7778966

…ms (#2030)" This reverts commit 4287e47.

t-col added a commit that referenced this pull request Dec 5, 2023

fix: revert "update createUpload to accept environment id in its para…

43c5252

…ms (#2030)" This reverts commit 4287e47.

jsdalton mentioned this pull request Dec 8, 2023

fix: environment-scoped uploads #2088

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update createUpload to accept environment id in its params [INTEG-1492] #2030

fix: update createUpload to accept environment id in its params [INTEG-1492] #2030

ryunsong-contentful commented Nov 1, 2023 •

edited

Loading

ruderngespra left a comment

ruderngespra Nov 2, 2023

ruderngespra commented Nov 2, 2023

jsdalton Nov 8, 2023

ruderngespra Nov 9, 2023

ryunsong-contentful Nov 9, 2023

ruderngespra Nov 9, 2023

ryunsong-contentful Nov 9, 2023

ruderngespra Nov 10, 2023

ryunsong-contentful Nov 10, 2023

jsdalton Nov 10, 2023

ryunsong-contentful Nov 10, 2023 •

edited

Loading

ruderngespra left a comment

ruderngespra Nov 10, 2023

ruderngespra Nov 10, 2023

ryunsong-contentful Nov 10, 2023

jsdalton left a comment

jsdalton Nov 10, 2023

jsdalton left a comment

contentful-automation bot commented Nov 13, 2023

contentful-automation bot commented Nov 17, 2023

	const getBaseUrl = (params: GetOrganizationParams) =>
	`/organizations/${params.organizationId}/organization_memberships`

	const getEntityUrl = (params: GetOrganizationMembershipParams) =>
	`${getBaseUrl(params)}/${params.organizationMembershipId}`

fix: update createUpload to accept environment id in its params [INTEG-1492] #2030

fix: update createUpload to accept environment id in its params [INTEG-1492] #2030

Conversation

ryunsong-contentful commented Nov 1, 2023 • edited Loading

ruderngespra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruderngespra commented Nov 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryunsong-contentful Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

ruderngespra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdalton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdalton left a comment

Choose a reason for hiding this comment

contentful-automation bot commented Nov 13, 2023

contentful-automation bot commented Nov 17, 2023

ryunsong-contentful commented Nov 1, 2023 •

edited

Loading

ryunsong-contentful Nov 10, 2023 •

edited

Loading