fix: API improvements aligning the types to our schemas #4650

gastonfournier · 2023-09-08T14:25:19Z

Some of our types in OSS have drifted apart from our OpenAPI schemas. This will help them be aligned again

vercel · 2023-09-08T14:25:25Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
unleash-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 12, 2023 1:34pm
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 12, 2023 1:34pm

gastonfournier · 2023-09-08T14:25:36Z

src/lib/routes/admin-api/strategy.ts

@@ -238,7 +238,7 @@ class StrategyController extends Controller {
    }

    async createStrategy(
-        req: IAuthRequest<unknown, CreateStrategySchema>,
+        req: IAuthRequest<unknown, unknown, CreateStrategySchema>,


Body should be the third parameter

gastonfournier · 2023-09-08T14:26:32Z

src/lib/types/model.ts

+// const data = await projectSchema.validateAsync(newProject);
+export type CreateProject = Pick<IProject, 'id' | 'name'> & {
+    mode?: ProjectMode;
+    defaultStickiness?: string;
+};


Aligning with the schema where only required fields are id and name: https://github.com/ivarconr/unleash-enterprise/blob/4544d0abfefbdcab5be633e4d0252c7e7564ce09/src/openapi/spec/create-project-schema.ts#L6

Nice. Could we use OpenAPI to set defaults instead? It has a default property you can use. I think that's better than using Joi. Honestly, I was hoping we could get rid of Joi now that we've openapi'd everything. Having two layers of API validation seems excessive (not to mention it can get confusing when you're not sure where an error is coming from).

The challenge here is that the spec is in the enterprise repository. Based on our latest conversations, this logic should be moved to enterprise. So in theory, I think that's possible but I don't know how much effort it will be to move all project creation logic to enterprise, and then do proper unit testing to validate that joi and OpenAPI validate and use defaults in the same way.

So, I rather leave that for another day

gastonfournier · 2023-09-08T15:08:42Z

src/lib/openapi/spec/create-strategy-schema.ts

@@ -6,12 +6,18 @@ export const createStrategySchema = {
    description:
        'The data required to create a strategy type. Refer to the docs on [custom strategy types](https://docs.getunleash.io/reference/custom-activation-strategies) for more information.',
    required: ['name', 'parameters'],
+    additionalProperties: false,


Important one so we have to declare all properties in the schema. This is input, so I'm not sure how specific we want to be with input data. Maybe we don't need this

@thomasheartman maybe you remember some decisions around additional properties. My take is that for input is not so important, as customers might want to extend Unleash, but I'm not 100% sure... I see in some cases we're doing it so should be fine:

unleash/src/lib/openapi/spec/create-user-schema.ts

Lines 3 to 6 in b60e581

export const createUserSchema = {

$id: '#/components/schemas/createUserSchema',

type: 'object',

additionalProperties: false,

the discussion we had previously was:
always allow additional properties on inputs, never on outputs.

Following the old adage to be liberal in what you accept, but strict in what you return, we thought it best to allow any additional properties coming in. We didn't want to break a user's experience because they added a field that we don't recognize.

On the output front, we want to be very clear about what we expect and we want the types to be accurate. For that to be true, we needed conversions to fail if they contained extra data.

Based on that, I don't think we should have additionalProperties: false here. Did you do it for a reason?

On the other hand, one drawback of never using additionalProperties: false on input data, is that it makes creating union types harder (or almost impossible without a discriminator).

But that said, if we require a property to be supplied by the user, then we can use the required property.

Could you elaborate a bit on this? I'm not sure I understand.

Important one so we have to declare all properties in the schema.

Anyway, my suggestion is to remove this line.

Suggested change

additionalProperties: false,

I did this to unveil the properties that were not declared in the spec. Turns out, this is not needed, but I discovered a way out of OpenAPI that allows us to define new properties without having to declare them in the spec.

Of course, this is not what we want, and I believe this was done by mistake or by not knowing that the type was backed by an OpenAPI spec. The trick is that this pattern also removes all safeguards from type safety.

The pattern (that we have to avoid) goes as follows:

In the controller layer, receive the request body and pass it down to the service:

async createStrategy( req: IAuthRequest<unknown, unknown, CreateStrategySchema>, res: Response<StrategySchema>, ): Promise<void> { const strategy = await this.strategyService.createStrategy( req.body, extractUsername(req), );

In the service layer, add a different type which is defined not as an OpenAPI spec:

async createStrategy( value: IMinimalStrategy, // this type userName: string, ): Promise<IStrategy> { const strategy = await strategySchema.validateAsync(value);

Now, because we allow additional properties on the first type, anything optional can be added to the second type without having to define the property in the OpenAPI spec (this doesn't apply to mandatory fields because all additional properties are inherently optional).

This leads to accidentally adding new optional parameters to internal types that do not get proper documentation in our spec.

What's the solution?
One possible solution is to avoid defining custom types and instead always refer to the schema type. Instead of:

export interface IMinimalStrategy { name: string; description?: string; editable?: boolean; parameters?: any[]; title?: string; }

We can use the schema and pick the fields we want from the schema:

export type IMinimalStrategy = Pick< CreateStrategySchema, 'name' | 'description' | 'editable' | 'parameters' | 'title' >;

This then sheds light on the missing fields at compile time:

thomasheartman

Nice work! I want to figure out the discussion points before approving this, but it's looking very good 😄

thomasheartman · 2023-09-11T05:44:33Z

src/lib/openapi/spec/create-strategy-schema.ts

@@ -6,12 +6,18 @@ export const createStrategySchema = {
    description:
        'The data required to create a strategy type. Refer to the docs on [custom strategy types](https://docs.getunleash.io/reference/custom-activation-strategies) for more information.',
    required: ['name', 'parameters'],
+    additionalProperties: false,


the discussion we had previously was:
always allow additional properties on inputs, never on outputs.

Following the old adage to be liberal in what you accept, but strict in what you return, we thought it best to allow any additional properties coming in. We didn't want to break a user's experience because they added a field that we don't recognize.

On the output front, we want to be very clear about what we expect and we want the types to be accurate. For that to be true, we needed conversions to fail if they contained extra data.

Based on that, I don't think we should have additionalProperties: false here. Did you do it for a reason?

On the other hand, one drawback of never using additionalProperties: false on input data, is that it makes creating union types harder (or almost impossible without a discriminator).

But that said, if we require a property to be supplied by the user, then we can use the required property.

Could you elaborate a bit on this? I'm not sure I understand.

Important one so we have to declare all properties in the schema.

Anyway, my suggestion is to remove this line.

Suggested change

additionalProperties: false,

thomasheartman · 2023-09-11T05:48:25Z

src/lib/types/model.ts

+// const data = await projectSchema.validateAsync(newProject);
+export type CreateProject = Pick<IProject, 'id' | 'name'> & {
+    mode?: ProjectMode;
+    defaultStickiness?: string;
+};


Nice. Could we use OpenAPI to set defaults instead? It has a default property you can use. I think that's better than using Joi. Honestly, I was hoping we could get rid of Joi now that we've openapi'd everything. Having two layers of API validation seems excessive (not to mention it can get confusing when you're not sure where an error is coming from).

chriswk

This seems fair. I'd like to see us move away from the id for Permission and just use the name as the primary key, since we do require the name to be unique anyway. It can come as a separate PR though, so I'm inclined to give a 👍 on this.

gastonfournier · 2023-09-12T13:29:07Z

I'd like to see us move away from the id for Permission and just use the name as the primary key

+1 maybe I can mark the type as deprecated

gastonfournier added 2 commits September 8, 2023 15:40

fix: create strategy typing

d90dd00

fix: align with the type in enterprise

64c9b2b

github-actions bot assigned gastonfournier Sep 8, 2023

gastonfournier commented Sep 8, 2023

View reviewed changes

gastonfournier added 2 commits September 8, 2023 16:33

Merge remote-tracking branch 'origin/main' into api-improvements

b5e14d2

Fix schema

b60e581

gastonfournier requested review from andreas-unleash, sjaanus and thomasheartman September 8, 2023 15:01

gastonfournier commented Sep 8, 2023

View reviewed changes

gastonfournier added 2 commits September 8, 2023 17:11

Fix schema to also consider deprecated field

e87a037

Fix example

dd8e9b8

thomasheartman reviewed Sep 11, 2023

View reviewed changes

Use OpenAPI schema to define internal service types

e2ea521

vercel bot deployed to Preview – unleash-docs September 12, 2023 08:53 View deployment

vercel bot deployed to Preview – unleash-monorepo-frontend September 12, 2023 08:54 View deployment

Fix type for enterprise open api spec

cd1a26f

vercel bot deployed to Preview – unleash-docs September 12, 2023 09:07 View deployment

vercel bot deployed to Preview – unleash-monorepo-frontend September 12, 2023 09:08 View deployment

Be more precise with types to match OpenAPI spec

fe25a90

vercel bot deployed to Preview – unleash-monorepo-frontend September 12, 2023 10:03 View deployment

vercel bot deployed to Preview – unleash-docs September 12, 2023 10:04 View deployment

gastonfournier requested a review from thomasheartman September 12, 2023 10:14

Merge remote-tracking branch 'origin/main' into api-improvements

7577c09

chriswk approved these changes Sep 12, 2023

View reviewed changes

Deprecation notice

2891ff9

vercel bot deployed to Preview – unleash-docs September 12, 2023 13:33 View deployment

vercel bot deployed to Preview – unleash-monorepo-frontend September 12, 2023 13:34 View deployment

gastonfournier merged commit c39d815 into main Sep 12, 2023
13 checks passed

gastonfournier deleted the api-improvements branch September 12, 2023 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: API improvements aligning the types to our schemas #4650

fix: API improvements aligning the types to our schemas #4650

gastonfournier commented Sep 8, 2023

vercel bot commented Sep 8, 2023 •

edited

Loading

gastonfournier Sep 8, 2023

gastonfournier Sep 8, 2023

thomasheartman Sep 11, 2023

gastonfournier Sep 11, 2023

gastonfournier Sep 8, 2023

thomasheartman Sep 11, 2023

gastonfournier Sep 11, 2023

thomasheartman left a comment

thomasheartman Sep 11, 2023

thomasheartman Sep 11, 2023

chriswk left a comment

gastonfournier commented Sep 12, 2023

	export const createUserSchema = {
	$id: '#/components/schemas/createUserSchema',
	type: 'object',
	additionalProperties: false,

fix: API improvements aligning the types to our schemas #4650

fix: API improvements aligning the types to our schemas #4650

Conversation

gastonfournier commented Sep 8, 2023

vercel bot commented Sep 8, 2023 • edited Loading

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

thomasheartman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriswk left a comment

Choose a reason for hiding this comment

gastonfournier commented Sep 12, 2023

vercel bot commented Sep 8, 2023 •

edited

Loading