docs: add descriptions and examples to tag schemas #4194

gastonfournier · 2023-07-10T11:42:08Z

About the changes

Adding descriptions and examples to tag and tag types schemas
Adding standard errors, summaries, and descriptions to tag and tag types endpoints
Some improvements on compilation errors

vercel · 2023-07-10T11:42:14Z

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

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
unleash-docs	⬜️ Ignored (Inspect)	Visit Preview		Jul 10, 2023 1:45pm
unleash-monorepo-frontend	⬜️ Ignored (Inspect)	Visit Preview		Jul 10, 2023 1:45pm

gastonfournier · 2023-07-10T12:25:49Z

src/lib/services/tag-type-service.ts

@@ -68,9 +68,11 @@ export default class TagTypeService {
        return Promise.resolve(true);
    }

-    async validate(tagType: ITagType): Promise<void> {
+    async validate(tagType: Partial<ITagType> | undefined): Promise<void> {


Solving a compilation problem

gastonfournier · 2023-07-10T12:26:00Z

src/lib/services/tag-type-service.ts

@@ -58,7 +58,7 @@ export default class TagTypeService {
        return data;
    }

-    async validateUnique({ name }: Pick<ITagType, 'name'>): Promise<boolean> {
+    async validateUnique(name: string): Promise<boolean> {


This solves a compilation problem

…tag-types

thomasheartman

Looks good! Most comments are non-blocking suggestions, but I have a few questions I'd like to get an answer to before accepting the change.

thomasheartman · 2023-07-10T12:28:26Z

src/lib/openapi/spec/tag-type-schema.ts

        },
        icon: {
            type: 'string',
            nullable: true,
+            description: 'The icon of the tag type.',
+            example: 'not-really-used',


This example is a little confusing. Are you saying we don't use the icons provided? If they provide icons, what format should it be in?

We're definitely not using them in the admin UI

https://sandbox.getunleash.io/enterprise/tag-types/create

https://sandbox.getunleash.io/enterprise/tag-types/edit/group

But icons do exist in our database and in our API. So I think this could be just an idea that never got implemented.

Maybe @chriswk who worked on this 2 years ago might remember something.

What Gaston said, it was meant to be an extra feature so you could paste an URL to a valid icon, so that :slack: / :jira: had a more obvious illustration than just the first letter of the type capitalised.

I don't think it ever got prioritised.

Got it. Do we have a format for that in any way?

Based on that, I'd definitely put that in the description and maybe even deprecate the properties?

Right now it can be anything, it will depend on what we want to do when we finally implement this. It's like predicting the future... we can tell customers to write it as a full-url, but maybe in the future we want to use an enum that is not a URL. So 🤷

Deprecating it also doesn't feel right because it's like some feature that has always been a work in progress. Maybe we can hide it from the documentation, but it was already there... so this is one of those unfortunate cases where we've dropped the ball and we didn't know about it.

We can update the description, but also the example helps documenting that it's not being used

Yeah, that sounds sensible. I'd add a note to the description that this isn't used yet, but might be in the future, then.

thomasheartman · 2023-07-10T12:29:46Z

src/lib/openapi/spec/tag-types-schema.ts

    properties: {
        version: {
            type: 'integer',
+            description:
+                'The version of the schema used to model the tag types.',
+            example: 1,


How many versions do we have? Could we maybe use an enum to show all the versions here?

Only 1 AFAIK, and it will probably never change. This is reserved for major changes, but I don't think modeling a version as an enum is a good idea as adding a new version would be a breaking change for older clients (which will not be gracefully handled)

Hmm, I see your reasoning here. However, I feel like giving users a clear description of what values we expect to deliver would be also be good. Maybe an enum and stating explicitly that more versions can be added in the future, but that any breaking changes would go in a major version?

If not, I'm happy to leave it as is too.

Maybe an enum and stating explicitly that more versions can be added in the future

I think the version being an integer states exactly that

Talking about it with @chriswk there might be some benefits of having it as an enum. I did some digging and found this and this, none of those were conclusive. I asked ChatGPT and this is what I got:

Including a version field in the response of an OpenAPI specification can be a good idea in certain situations, especially when dealing with APIs that are subject to frequent updates or have different versions coexisting. However, whether to represent the version field as an enum or a string/integer depends on your specific use case and requirements. Let's discuss the considerations for each option:

Enum:
Using an enum to represent the version field in the response has some benefits. It provides a predefined set of allowed values, ensuring that the version is always one of the specified options. This can help with consistency, validation, and avoiding potential typos or accidental variations in the version representation.

However, using an enum for versioning can also introduce some challenges. Enums are typically used for representing a finite set of known values, assuming that the set won't change frequently. If your API has a dynamic versioning system with a large number of potential versions, it may be impractical to maintain an enum that includes all possible values. Additionally, if the versioning system allows for arbitrary or user-defined values, an enum may not be suitable.

String or Integer:
Representing the version field as a string or an integer provides more flexibility compared to an enum. It allows for arbitrary version values, including custom or vendor-specific formats. This flexibility can be advantageous when dealing with complex versioning schemes or when the API consumers need to parse and interpret the version information in a specific way.

Using a string or integer for the version field also makes it easier to add new versions without modifying the API specification. You can simply introduce a new value without needing to update the enum definition or regenerate client code.

Ultimately, the decision between using an enum, string, or integer for the version field depends on the specific requirements of your API and how you want to manage versioning. Consider factors such as the expected number of versions, whether the set of versions is known and stable, and how the version information will be used by API consumers.

Overall, this doesn't seem like a decision to take on lightly in this particular API. I think this is worth some general alignment and changing this across the board.

Most of our versions are numbers, a few are enums and there's no consistency on how we define versions. I think this should be our focus, but for now, I'd like to keep it as an integer which is what majority of our APIs are using

Yeah, I agree. Thanks for flagging that and having the discussion! Let's figure it out later 😄

thomasheartman · 2023-07-10T12:30:42Z

src/lib/openapi/spec/tag-with-version-schema.ts

    properties: {
        version: {
            type: 'integer',
+            description: 'The version of the schema used to model the tag.',
+            example: 1,


Same as above. Can we and do we want to use enums here?

I don't think it's a good idea to model auto-incremental values with enums

I ended up considering the option of modeling versions with enums as potentially good: #4194 (comment) but I think we should be consistent with the rest of our schemas, and if we want to pursue this change we should do it on all schemas. So I'm keeping this for this PR but I'm open to follow up on this topic and changing it later if we deem it valuable

thomasheartman · 2023-07-10T12:31:16Z

src/lib/openapi/spec/update-tag-type-schema.ts

        },
        icon: {
            type: 'string',
+            description: 'The icon of the tag type.',
+            example: 'not-really-used',


Same as the previous question regarding tag types.

thomasheartman · 2023-07-10T12:32:30Z

src/lib/routes/admin-api/tag-type.ts

@@ -69,8 +77,11 @@ class TagTypeController extends Controller {
                openApiService.validPath({
                    tags: ['Tags'],
                    operationId: 'createTagType',
+                    summary: 'Create a tag type',
+                    description: 'Create a new tag type.',


What happens if a tag type with the same name already exists? Is that a 409? We should communicate that to the end user.

Yes, it's a 409, I missed adding it

Added here: bf74c88

src/lib/routes/admin-api/tag-type.ts

thomasheartman · 2023-07-10T12:36:32Z

src/lib/routes/admin-api/tag-type.ts

                    responses: {
                        200: emptyResponse,
+                        ...getStandardResponses(401, 403),


Do you get a 404 if the tag doesn't exist or just a 200?

Always 200 when the tag doesn't exist

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

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

…into docs/openapi-tag-and-tag-types

thomasheartman

I think this is good to go in now 😄 I'd like some clarification on the icon property for tag types, but that can always be added later 💁🏼

docs: add descriptions and examples to tag schemas

65749b2

github-actions bot assigned gastonfournier Jul 10, 2023

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

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

Summaries and descriptions

12157d1

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

gastonfournier commented Jul 10, 2023

View reviewed changes

gastonfournier requested a review from thomasheartman July 10, 2023 12:26

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

Merge remote-tracking branch 'origin/main' into docs/openapi-tag-and-…

61431bb

…tag-types

thomasheartman reviewed Jul 10, 2023

View reviewed changes

gastonfournier and others added 3 commits July 10, 2023 15:27

Apply suggestions from code review

e8c49a7

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

Document 409

bf74c88

Merge remote-tracking branch 'origin/docs/openapi-tag-and-tag-types' …

bc1dd51

…into docs/openapi-tag-and-tag-types

gastonfournier requested a review from thomasheartman July 10, 2023 13:29

Lint

8c237ef

thomasheartman approved these changes Jul 11, 2023

View reviewed changes

gastonfournier merged commit 58151d8 into main Jul 11, 2023
10 checks passed

gastonfournier deleted the docs/openapi-tag-and-tag-types branch July 11, 2023 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: add descriptions and examples to tag schemas #4194

docs: add descriptions and examples to tag schemas #4194

gastonfournier commented Jul 10, 2023 •

edited

vercel bot commented Jul 10, 2023 •

edited

gastonfournier Jul 10, 2023

gastonfournier Jul 10, 2023

thomasheartman left a comment

thomasheartman Jul 10, 2023

gastonfournier Jul 10, 2023

chriswk Jul 10, 2023

chriswk Jul 10, 2023

thomasheartman Jul 11, 2023

gastonfournier Jul 11, 2023

thomasheartman Jul 11, 2023

thomasheartman Jul 10, 2023

gastonfournier Jul 10, 2023

thomasheartman Jul 11, 2023

gastonfournier Jul 11, 2023

gastonfournier Jul 11, 2023

thomasheartman Jul 11, 2023

thomasheartman Jul 10, 2023

gastonfournier Jul 10, 2023

gastonfournier Jul 11, 2023

thomasheartman Jul 10, 2023

thomasheartman Jul 10, 2023

gastonfournier Jul 10, 2023

gastonfournier Jul 10, 2023

thomasheartman Jul 10, 2023

gastonfournier Jul 10, 2023

thomasheartman left a comment

docs: add descriptions and examples to tag schemas #4194

docs: add descriptions and examples to tag schemas #4194

Conversation

gastonfournier commented Jul 10, 2023 • edited

About the changes

vercel bot commented Jul 10, 2023 • edited

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

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

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

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

gastonfournier commented Jul 10, 2023 •

edited

vercel bot commented Jul 10, 2023 •

edited