Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Sep 12, 2023
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d90dd00
fix: create strategy typing
gastonfournier Sep 8, 2023
64c9b2b
fix: align with the type in enterprise
gastonfournier Sep 8, 2023
b5e14d2
Merge remote-tracking branch 'origin/main' into api-improvements
gastonfournier Sep 8, 2023
b60e581
Fix schema
gastonfournier Sep 8, 2023
e87a037
Fix schema to also consider deprecated field
gastonfournier Sep 8, 2023
dd8e9b8
Fix example
gastonfournier Sep 8, 2023
e2ea521
Use OpenAPI schema to define internal service types
gastonfournier Sep 12, 2023
cd1a26f
Fix type for enterprise open api spec
gastonfournier Sep 12, 2023
fe25a90
Be more precise with types to match OpenAPI spec
gastonfournier Sep 12, 2023
7577c09
Merge remote-tracking branch 'origin/main' into api-improvements
gastonfournier Sep 12, 2023
2891ff9
Deprecation notice
gastonfournier Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/lib/openapi/spec/create-strategy-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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),
        );
  1. 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:
image

properties: {
name: {
type: 'string',
description: 'The name of the strategy type. Must be unique.',
example: 'my-custom-strategy',
},
title: {
type: 'string',
description: 'The title of the strategy',
example: 'My awesome strategy',
},
description: {
type: 'string',
description: 'A description of the strategy type.',
Expand Down
2 changes: 1 addition & 1 deletion src/lib/routes/admin-api/strategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ class StrategyController extends Controller {
}

async createStrategy(
req: IAuthRequest<unknown, CreateStrategySchema>,
req: IAuthRequest<unknown, unknown, CreateStrategySchema>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Body should be the third parameter

res: Response<StrategySchema>,
): Promise<void> {
const userName = extractUsername(req);
Expand Down
6 changes: 2 additions & 4 deletions src/lib/services/project-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
IProjectRoleUsage,
ProjectAccessUserRolesDeleted,
IFeatureNaming,
CreateProject,
} from '../types';
import { IProjectQuery, IProjectStore } from '../types/stores/project-store';
import {
Expand Down Expand Up @@ -190,10 +191,7 @@ export default class ProjectService {
};

async createProject(
newProject: Pick<
IProject,
'id' | 'name' | 'mode' | 'defaultStickiness'
>,
newProject: CreateProject,
user: IUser,
): Promise<IProject> {
const data = await projectSchema.validateAsync(newProject);
Expand Down
8 changes: 8 additions & 0 deletions src/lib/types/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,14 @@ export interface IImportData extends ImportCommon {
data: any;
}

// Create project aligns with #/components/schemas/createProjectSchema
// joi is providing default values when the optional inputs are not provided
// const data = await projectSchema.validateAsync(newProject);
export type CreateProject = Pick<IProject, 'id' | 'name'> & {
mode?: ProjectMode;
defaultStickiness?: string;
};
Comment on lines +407 to +411
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


export interface IProject {
id: string;
name: string;
Expand Down