From 4e0678dfb1b33fa7b39dfd014ac2f34d389ecc55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Mon, 12 Jun 2023 10:15:00 +0200 Subject: [PATCH] fix: allow empty appName as it may come in the url (#3953) ## About the changes Edit application under https://app.unleash-hosted.com/demo/applications/test-app is currently not working as the appName is expected to come in the request body, but it's actually part of the url. We have two options here: 1. We change the UI to adapt to the expectations of the request by adding appName to the request body (and eventually removing appName from the URL, which would be a breaking change) 2. We remove the restriction of only sending the appName in the body and take the one that comes in the URL. We have a validation that verifies that at least one of the two sets the appName ([here](https://github.com/Unleash/unleash/blob/e376088668428b3ec9016de9fd481fbbb03ad5cb/src/lib/services/client-metrics/instance-service.ts#L208) we validate using [this schema](https://github.com/Unleash/unleash/blob/e376088668428b3ec9016de9fd481fbbb03ad5cb/src/lib/services/client-metrics/schema.ts#L55-L70)) In terms of REST API, we can assume that the appName will be present in the resource `/api/admin/metrics/applications` (an endpoint we don't have), but when we're updating an application we should refer to that application by its URL: `/api/admin/metrics/applications/` and the presence of an appName in the body might indicate that we're trying to change the name of the application (something we currently not support) Based on the above, I believe going with the second option is best, as it adheres to REST principles and does not require a breaking change. Despite that, we only support updating applications as the creation is done from metrics ingestion Fixes: #3580 --- src/lib/openapi/spec/create-application-schema.ts | 1 - src/lib/routes/admin-api/metrics.test.ts | 14 ++++++++++++++ .../openapi/__snapshots__/openapi.e2e.test.ts.snap | 3 --- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/lib/openapi/spec/create-application-schema.ts b/src/lib/openapi/spec/create-application-schema.ts index edcac29957..c6e99d4c16 100644 --- a/src/lib/openapi/spec/create-application-schema.ts +++ b/src/lib/openapi/spec/create-application-schema.ts @@ -5,7 +5,6 @@ export const createApplicationSchema = { type: 'object', additionalProperties: true, description: 'Reported application information from Unleash SDKs', - required: ['appName'], properties: { appName: { description: 'Name of the application', diff --git a/src/lib/routes/admin-api/metrics.test.ts b/src/lib/routes/admin-api/metrics.test.ts index 885a1924a9..640e1ff989 100644 --- a/src/lib/routes/admin-api/metrics.test.ts +++ b/src/lib/routes/admin-api/metrics.test.ts @@ -84,6 +84,20 @@ test('should store application', () => { .expect(202); }); +test('should store application coming from edit application form', () => { + expect.assertions(0); + const appName = '123!23'; + + return request + .post(`/api/admin/metrics/applications/${appName}`) + .send({ + url: 'http://test.com', + description: 'This is an optional description', + icon: 'arrow-down', + }) + .expect(202); +}); + test('should store application details without strategies', () => { expect.assertions(0); const appName = '123!23'; diff --git a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap index e5b3ee4dc0..33461af429 100644 --- a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap +++ b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap @@ -1576,9 +1576,6 @@ The provider you choose for your addon dictates what properties the \`parameters "type": "string", }, }, - "required": [ - "appName", - ], "type": "object", }, "createFeatureSchema": {