From 4305dcf0d7b996a12a824f26a3b52643f2278c25 Mon Sep 17 00:00:00 2001 From: Giulio Davide Carparelli Date: Wed, 8 Feb 2023 15:11:33 +0100 Subject: [PATCH] fix: merging of default custom-plugin-lib schema with the user provided one (#328) * fix: merging of two json schemas * added more test cases for schema validation * fix: default schema causing a bad merge * added tests for schema validation on first level properties * fix: discussions * fix: copyright discussion * fix: merging discussion * added changelog and modified former mergeJsonSchemas function to mergeWithDefaultJsonSchema * added test case for warning of merge --------- Co-authored-by: Giulio Davide --- CHANGELOG.md | 3 + index.js | 55 +++++++++++-------- tests/environment.test.js | 22 ++++++++ tests/index.test.js | 42 +++++++++++++- tests/services/advanced-custom-service.js | 3 +- ...advanced-first-level-properties-service.js | 41 ++++++++++++++ 6 files changed, 138 insertions(+), 28 deletions(-) create mode 100644 tests/services/advanced-first-level-properties-service.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 52c5267..3a8fa5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Unreleased +### Fixed +- merging of baseProperties with customProperties causing inconsistent microservices' env Variables schema + ### Added - add optional metrics on request duration. The metrics collection is enabled by mean of the variable `ENABLE_HTTP_CLIENT_METRICS` (default: false) diff --git a/index.js b/index.js index fea2418..73c3858 100644 --- a/index.js +++ b/index.js @@ -94,30 +94,36 @@ const baseSchema = { }, } -function mergeJsonSchemas(schema, otherSchema) { - const { properties: schemaProperties, required: requiredSchema = [], ...schemaRemainingProperties } = schema - const { - properties: otherSchemaProperties, - required: requiredOtherSchema = [], - ...otherSchemaRemainingProperties - } = otherSchema - const mergedSchema = { - type: 'object', - properties: { - ...schemaProperties, - ...otherSchemaProperties, - }, - required: [...requiredSchema, ...requiredOtherSchema], - allOf: [ - schemaRemainingProperties, - otherSchemaRemainingProperties, - ], - additionalProperties: false, +function mergeObjectOrArrayProperty(toBeMergedValues, alreadyMergedValues, isArray) { + return isArray ? [ + ...toBeMergedValues ?? [], + ...alreadyMergedValues, + ] : { + ...toBeMergedValues ?? {}, + ...alreadyMergedValues, } - return mergedSchema +} + +// WARNING: including any first level properties other than the ones already declared +// may have undesired effects on the result of the merge +function mergeWithDefaultJsonSchema(schema) { + const defaultSchema = { + ...baseSchema, + } + + Object.keys(schema).forEach(key => { + defaultSchema[key] = typeof schema[key] === 'object' + ? mergeObjectOrArrayProperty(defaultSchema[key], schema[key], Array.isArray(schema[key])) + : schema[key] + }) + + return defaultSchema } function getOverlappingKeys(properties, otherProperties) { + if (!otherProperties) { + return [] + } const propertiesNames = Object.keys(properties) const otherPropertiesNames = Object.keys(otherProperties) const overlappingProperties = propertiesNames.filter(propertyName => @@ -315,16 +321,17 @@ async function decorateRequestAndFastifyInstance(fastify, { asyncInitFunction, s }) } -const defaultSchema = { type: 'object', required: [], properties: {} } -function initCustomServiceEnvironment(envSchema = defaultSchema) { +function initCustomServiceEnvironment(envSchema) { return function customService(asyncInitFunction, serviceOptions) { async function index(fastify, opts) { - const overlappingPropertiesNames = getOverlappingKeys(baseSchema.properties, envSchema.properties) + const overlappingPropertiesNames = getOverlappingKeys(baseSchema.properties, envSchema?.properties) if (overlappingPropertiesNames.length > 0) { throw new Error(`The provided Environment JSON Schema includes properties declared in the Base JSON Schema of the custom-plugin-lib, please remove them from your schema. The properties to remove are: ${overlappingPropertiesNames.join(', ')}`) } - fastify.register(fastifyEnv, { schema: mergeJsonSchemas(baseSchema, envSchema), data: opts }) + + const schema = envSchema ? mergeWithDefaultJsonSchema(envSchema) : baseSchema + fastify.register(fastifyEnv, { schema, data: opts }) fastify.register(fastifyFormbody) fastify.register(fp(decorateRequestAndFastifyInstance), { asyncInitFunction, serviceOptions }) } diff --git a/tests/environment.test.js b/tests/environment.test.js index 275e974..3bdca96 100644 --- a/tests/environment.test.js +++ b/tests/environment.test.js @@ -230,5 +230,27 @@ tap.test('Test Environment variables', test => { } }) + test.test('Should pass since all of the required fields are present', async assert => { + const env = { + ...baseEnv, + MY_REQUIRED_ENV_VAR: 'value', + } + + await assert.resolves(async() => { + await setupFastify('./tests/services/advanced-first-level-properties-service.js', env) + }) + }) + + test.test('Should fail since one of the required fields is not present', async assert => { + // NOTE: use try catch instead of assert.reject to customize error message assertion + assert.plan(1) + try { + await setupFastify('./tests/services/advanced-first-level-properties-service.js', baseEnv) + } catch (error) { + const errorMessage = 'env must have required property \'MY_REQUIRED_ENV_VAR\'' + assert.strictSame(error.message, errorMessage) + } + }) + test.end() }) diff --git a/tests/index.test.js b/tests/index.test.js index 5f8181c..500f4b5 100644 --- a/tests/index.test.js +++ b/tests/index.test.js @@ -28,6 +28,7 @@ const GROUPS_HEADER_KEY = 'groups-header-key' const CLIENTTYPE_HEADER_KEY = 'clienttype-header-key' const BACKOFFICE_HEADER_KEY = 'backoffice-header-key' const MICROSERVICE_GATEWAY_SERVICE_NAME = 'microservice-gateway' +const MY_REQUIRED_ENV_VAR = 'value' const baseEnv = { USERID_HEADER_KEY, @@ -329,7 +330,7 @@ tap.test('Advanced Custom Service', test => { test.test('Require some environment variables', async assert => { const MY_AWESOME_ENV = 'foobar' - const fastify = await setupFastify({ ...baseEnv, MY_AWESOME_ENV }) + const fastify = await setupFastify({ ...baseEnv, MY_AWESOME_ENV, MY_REQUIRED_ENV_VAR }) const response = await fastify.inject({ method: 'GET', url: '/env', @@ -344,7 +345,7 @@ tap.test('Advanced Custom Service', test => { test.test('Decorate fastify with custom functionalities', async assert => { const MY_AWESOME_ENV = 'foobar' const payload = { hello: 'world' } - const fastify = await setupFastify({ ...baseEnv, MY_AWESOME_ENV }) + const fastify = await setupFastify({ ...baseEnv, MY_AWESOME_ENV, MY_REQUIRED_ENV_VAR }) const response = await fastify.inject({ method: 'POST', url: '/custom', @@ -359,7 +360,7 @@ tap.test('Advanced Custom Service', test => { }) test.test('default env var', async assert => { - const fastify = await setupFastify(baseEnv) + const fastify = await setupFastify({ ...baseEnv, MY_REQUIRED_ENV_VAR }) const response = await fastify.inject({ method: 'GET', url: '/env', @@ -478,3 +479,38 @@ tap.test('Service with API formats', t => { t.end() }) + +tap.test('baseEnv with first level properties', t => { + async function setupFastify(envVariables) { + const fastify = await lc39('./tests/services/if-then-else-env-validation-custom-service.js', { + logLevel: 'silent', + envVariables, + }) + return fastify + } + + t.test('it should fail the build', async t => { + try { + await setupFastify({ + ...baseEnv, + if: { + properties: { + field: { + const: 'value', + }, + }, + }, + then: { + required: [ + 'field1', + ], + }, + }) + t.fail() + } catch (error) { + t.ok(error) + } + }) + + t.end() +}) diff --git a/tests/services/advanced-custom-service.js b/tests/services/advanced-custom-service.js index d561f24..a79c37a 100644 --- a/tests/services/advanced-custom-service.js +++ b/tests/services/advanced-custom-service.js @@ -17,9 +17,10 @@ 'use strict' const envSchema = { type: 'object', - required: ['MY_AWESOME_ENV'], + required: ['MY_AWESOME_ENV', 'MY_REQUIRED_ENV_VAR'], properties: { MY_AWESOME_ENV: { type: 'string', default: 'the default value' }, + MY_REQUIRED_ENV_VAR: { type: 'string' }, }, } diff --git a/tests/services/advanced-first-level-properties-service.js b/tests/services/advanced-first-level-properties-service.js new file mode 100644 index 0000000..644fd74 --- /dev/null +++ b/tests/services/advanced-first-level-properties-service.js @@ -0,0 +1,41 @@ +/* + * Copyright 2023 Mia srl + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. +*/ + +'use strict' +const envSchema = { + type: 'object', + required: ['MY_AWESOME_ENV', 'MY_REQUIRED_ENV_VAR'], + properties: { + MY_AWESOME_ENV: { type: 'string', default: 'the default value' }, + MY_REQUIRED_ENV_VAR: { type: 'string' }, + }, + patternProperties: { + '^S_': { 'type': 'string' }, + '^I_': { 'type': 'number' }, + }, + minProperties: 1, + additionalProperties: true, +} + +const customService = require('../../index')(envSchema) + +module.exports = customService(async function clientGroups(service) { + async function handlerApiRest(_, reply) { + reply.send(this.config.required) + } + + service.addRawCustomPlugin('GET', '/api-rest', handlerApiRest) +})