From 29be130757b7c149275906b55ad8dcddf60aec4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Thu, 5 Jan 2023 12:39:18 +0100 Subject: [PATCH] fix: enforce weight precision to 1 decimal (#2749) ## About the changes According to our docs, we only support up to 1 decimal place for weights. This is to use integers to represent the percentages (we divide them by 10) and supporting more decimals results in bad maths due to floating point arithmetics. This PRs adds Frontend and Backend validations to enforce this restriction Closes #2222 ## Discussion points Should we reconsider supporting more decimal places, that door remains open, but for now we'll just adhere to our documentation because that change would require some development. --- .../EnvironmentVariantModal.tsx | 1 + src/lib/schema/feature-schema.test.ts | 50 +++++++++++++++++++ src/lib/schema/feature-schema.ts | 8 ++- 3 files changed, 58 insertions(+), 1 deletion(-) diff --git a/frontend/src/component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/EnvironmentVariantModal/EnvironmentVariantModal.tsx b/frontend/src/component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/EnvironmentVariantModal/EnvironmentVariantModal.tsx index 6d68409ccb..3f3b78dc3f 100644 --- a/frontend/src/component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/EnvironmentVariantModal/EnvironmentVariantModal.tsx +++ b/frontend/src/component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/EnvironmentVariantModal/EnvironmentVariantModal.tsx @@ -274,6 +274,7 @@ export const EnvironmentVariantModal = ({ const isValidPercentage = (percentage: string) => { if (!customPercentage) return true; if (percentage === '') return false; + if (percentage.match(/\.[0-9]{2,}$/)) return false; const percentageNumber = Number(percentage); return percentageNumber >= 0 && percentageNumber <= 100; diff --git a/src/lib/schema/feature-schema.test.ts b/src/lib/schema/feature-schema.test.ts index 534f2722db..7ca21e6c3a 100644 --- a/src/lib/schema/feature-schema.test.ts +++ b/src/lib/schema/feature-schema.test.ts @@ -70,6 +70,56 @@ test('should allow weightType=fix', () => { expect(value).toEqual(toggle); }); +test('should not allow weightType=fix with floats', () => { + const toggle = { + name: 'app.name', + type: 'release', + project: 'default', + enabled: false, + impressionData: false, + stale: false, + archived: false, + strategies: [{ name: 'default' }], + variants: [ + { + name: 'variant-a', + weight: 1.5, + weightType: 'fix', + stickiness: 'default', + }, + ], + }; + + const { error } = featureSchema.validate(toggle); + expect(error.details[0].message).toEqual('Weight only supports 1 decimal'); +}); + +test('should not allow weightType=fix with more than 1000', () => { + const toggle = { + name: 'app.name', + type: 'release', + project: 'default', + enabled: false, + impressionData: false, + stale: false, + archived: false, + strategies: [{ name: 'default' }], + variants: [ + { + name: 'variant-a', + weight: 1001, + weightType: 'fix', + stickiness: 'default', + }, + ], + }; + + const { error } = featureSchema.validate(toggle); + expect(error.details[0].message).toEqual( + '"variants[0].weight" must be less than or equal to 1000', + ); +}); + test('should disallow weightType=unknown', () => { const toggle = { name: 'app.name', diff --git a/src/lib/schema/feature-schema.ts b/src/lib/schema/feature-schema.ts index c81afd4f30..29dc66755a 100644 --- a/src/lib/schema/feature-schema.ts +++ b/src/lib/schema/feature-schema.ts @@ -50,7 +50,13 @@ export const variantValueSchema = joi export const variantsSchema = joi.object().keys({ name: nameType, - weight: joi.number().min(0).max(1000).required(), + weight: joi + .number() + .integer() + .message('Weight only supports 1 decimal') + .min(0) + .max(1000) + .required(), weightType: joi.string().valid('variable', 'fix').default('variable'), payload: joi .object()