feat: openapi schema for user admin #4146

kwasniew · 2023-07-05T10:05:42Z

About the changes

Full OpenAPI spec for user-admin controller.

This should be around 100-200 LOC. 1k lines is from the snapshot test.
Removed all working schemas from the meta schema exception list
adding 400 when body or query/path params are validated, 401 always, 403 when ADMIN is required, 404 when resource may not exist
fixing a few 500 when id was not a number, those should be 400 errors
making BadDataError reflect incorrect data in body or params
making user roles being number or string explicit. When creating/updating a user we can specify both and we return what you specified. When reading a user we always return a number.

Important files

Discussion points

vercel · 2023-07-05T10:05:48Z

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	Jul 5, 2023 3:07pm
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 5, 2023 3:07pm

kwasniew · 2023-07-05T10:07:07Z

src/lib/error/bad-data-error.ts

@@ -16,7 +16,7 @@ class BadDataError extends UnleashError {
        errors?: [ValidationErrorDescription, ...ValidationErrorDescription[]],
    ) {
        const topLevelMessage =
-            'Request validation failed: your request body contains invalid data' +
+            'Request validation failed: your request body or params contain invalid data' +


we throw this error when query or path params are incorrect so the error message should be more generic

kwasniew · 2023-07-05T10:08:06Z

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

+    description: 'An Unleash user after creation',
+    required: ['id'],
+    properties: {
+        ...userSchema.properties,


same as user schema but it returns either number or string for the role name depending on what was in the create/update request

user schema hasn't changed because this schema was added

kwasniew · 2023-07-05T10:12:36Z

src/lib/services/user-service.ts

@@ -192,7 +192,7 @@ class UserService {

        const exists = await this.store.hasUser({ username, email });
        if (exists) {
-            throw new Error('User already exists');
+            throw new BadDataError('User already exists');


Error was 500, BadDataError is 400

kwasniew · 2023-07-05T10:12:48Z

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

    }

    async deleteUser(req: IAuthRequest, res: Response): Promise<void> {
        const { user, params } = req;
        const { id } = params;
+        if (!Number.isInteger(Number(id))) {


previously it was 500, not it is 400

kwasniew · 2023-07-05T10:13:24Z

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

+        if (!Number.isInteger(Number(id))) {
+            throw new BadDataError('User id should be an integer');
+        }
+        const normalizedRootRole = Number.isInteger(Number(rootRole))


openapi coerces root role to string on number | string union so we need to coerce it back to number if possible

kwasniew · 2023-07-05T10:32:37Z

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

-            type: 'number',
+            description:
+                "The role to assign to the user. Can be either the role's ID or its unique name.",
+            oneOf: [


oneOf with integer or string + coercion: true in our openapi lib causes rootRole to always be a string. We need to compensate for it in the controller by coercing back

sjaanus · 2023-07-05T10:37:37Z

Also enable strictSchemaValidation in user-admin e2e test to validate all of this.

thomasheartman

The changes look good to me overall! 💯 Got a few questions and minor typo fixes that I'd like to have addressed, though, but I trust you to take that into account before merging.

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

thomasheartman · 2023-07-05T11:33:33Z

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

+                    parameters: [
+                        {
+                            name: 'q',
+                            description:
+                                'The pattern to search in the username or email',
+                            schema: { type: 'string' },
+                            in: 'query',
+                        },
+                    ],


If you need to type this somehow (you might not), then you could look into using the FromQueryParams method and putting it in its own file (similar to how we do it in src/lib/openapi/spec/export-query-parameters.ts). I'm not sure whether that's necessary here. As long as it shows up correctly in the output schema, I'm happy 😄

I copied this pattern from other places where it was done inline. It does show up in the swagger UI and works fine :)

thomasheartman · 2023-07-05T11:34:40Z

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

@@ -256,8 +318,13 @@ export default class UserAdminController extends Controller {
                openApiService.validPath({
                    tags: ['Users'],
                    operationId: 'updateUser',
+                    summary: 'Update a user',
+                    description: 'Updates use with new fields',


Are all fields required? Are non-submitted fields ignored? In other words: does this act more like a "true PUT" or a "PATCH"? It'd be good to mention that in the description.

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

thomasheartman · 2023-07-05T11:43:11Z

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

-            201,
+            200,


Why did we change this from a 201 to a 200?

I think one of our validators was complaining that 201 should have a Location header

Yeah, that sounds about right. You should add one: respondWithValidation takes an optional last parameter called headers. You can add it in there, pointing to the newly created user.

Ok I found the location header setting pattern in other parts of the code. Our schema was lying before with 200 and the code was returning 201. Let's stick to 201 in both

…n-api-spec

Co-authored-by: Thomas Heartman <thomas@getunleash.ai>

… into user-admin-open-api-spec

feat: openapi schema for user admin

f04ad24

github-actions bot assigned kwasniew Jul 5, 2023

kwasniew commented Jul 5, 2023

View reviewed changes

vercel bot deployed to Preview – unleash-docs July 5, 2023 10:10 View deployment

kwasniew commented Jul 5, 2023

View reviewed changes

vercel bot deployed to Preview – unleash-monorepo-frontend July 5, 2023 10:13 View deployment

kwasniew commented Jul 5, 2023

View reviewed changes

feat: openapi schema for user admin

1fdcc21

vercel bot deployed to Preview – unleash-docs July 5, 2023 10:30 View deployment

vercel bot deployed to Preview – unleash-monorepo-frontend July 5, 2023 10:32 View deployment

kwasniew commented Jul 5, 2023

View reviewed changes

thomasheartman approved these changes Jul 5, 2023

View reviewed changes

kwasniew and others added 4 commits July 5, 2023 13:44

Merge branch 'main' of github.com:Unleash/unleash into user-admin-ope…

eb0545a

…n-api-spec

Update src/lib/routes/admin-api/user-admin.ts

88ec9f6

Co-authored-by: Thomas Heartman <thomas@getunleash.ai>

Update src/lib/routes/admin-api/user-admin.ts

1b3fa2d

Co-authored-by: Thomas Heartman <thomas@getunleash.ai>

Merge branch 'user-admin-open-api-spec' of github.com:Unleash/unleash…

eacf42b

… into user-admin-open-api-spec

vercel bot deployed to Preview – unleash-docs July 5, 2023 11:48 View deployment

vercel bot deployed to Preview – unleash-monorepo-frontend July 5, 2023 11:49 View deployment

feat: openapi schema for user admin

122f8b2

vercel bot deployed to Preview – unleash-monorepo-frontend July 5, 2023 13:16 View deployment

vercel bot deployed to Preview – unleash-docs July 5, 2023 13:20 View deployment

feat: openapi schema for user admin

490f149

vercel bot deployed to Preview – unleash-monorepo-frontend July 5, 2023 13:41 View deployment

vercel bot deployed to Preview – unleash-docs July 5, 2023 13:42 View deployment

feat: openapi schema for user admin

13f8cb3

vercel bot deployed to Preview – unleash-monorepo-frontend July 5, 2023 14:55 View deployment

vercel bot deployed to Preview – unleash-docs July 5, 2023 14:57 View deployment

feat: openapi schema for user admin

f1a8f58

vercel bot deployed to Preview – unleash-monorepo-frontend July 5, 2023 15:05 View deployment

vercel bot deployed to Preview – unleash-docs July 5, 2023 15:07 View deployment

kwasniew merged commit 79b3412 into main Jul 6, 2023
17 checks passed

kwasniew deleted the user-admin-open-api-spec branch July 6, 2023 06:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: openapi schema for user admin #4146

feat: openapi schema for user admin #4146

kwasniew commented Jul 5, 2023 •

edited

vercel bot commented Jul 5, 2023 •

edited

kwasniew Jul 5, 2023

kwasniew Jul 5, 2023

kwasniew Jul 5, 2023

kwasniew Jul 5, 2023

kwasniew Jul 5, 2023

kwasniew Jul 5, 2023

kwasniew Jul 5, 2023

sjaanus commented Jul 5, 2023

thomasheartman left a comment

thomasheartman Jul 5, 2023

kwasniew Jul 5, 2023

thomasheartman Jul 5, 2023

thomasheartman Jul 5, 2023

kwasniew Jul 5, 2023

thomasheartman Jul 5, 2023

kwasniew Jul 5, 2023

feat: openapi schema for user admin #4146

feat: openapi schema for user admin #4146

Conversation

kwasniew commented Jul 5, 2023 • edited

About the changes

Important files

Discussion points

vercel bot commented Jul 5, 2023 • edited

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

sjaanus commented Jul 5, 2023

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

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

kwasniew commented Jul 5, 2023 •

edited

vercel bot commented Jul 5, 2023 •

edited