openapi-ts / openapi-typescript

Generate TypeScript types from OpenAPI 3 specs
https://openapi-ts.dev
MIT License
5.64k stars 455 forks source link

Support "readOnly" and "writeOnly" OpenAPI properties #604

Open Rycochet opened 3 years ago

Rycochet commented 3 years ago

See https://swagger.io/docs/specification/data-models/data-types/#readonly-writeonly

These modifiers are on specific properties and allow for a single definition to be used in both read & write endpoints, but provide a different shape to each -

// ... "Data": { ...
"properties": {
  "id": { // Returned by GET, not used in POST/PUT/PATCH
    "type": "integer",
    "readOnly": true
  },
  "username": {
    "type": "string"
  },
  "password": { // Used in POST/PUT/PATCH, not returned by GET
    "type": "string",
    "writeOnly": true
  }
}

Currently this just generates a single interface similar to -

export interface components {
  schemas: {
    Data: {
      id: number;
      username: string;
      password: string;
    };
  };
}

Unfortunately that makes it unusable for any endpoint as GET doesn't show the password and POST etc doesn't want an id. One way to change this is to use multiple interfaces (alternatively using a suffix for the schema might be suitable - but there is a possibility of a name clash that way) - possibly something like -

export interface components {
  read_schemas: {
    Data: {
      id: number;
      username: string;
    };
  };
  write_schemas: {
    Data: {
      username: string;
      password: string;
    };
  };
  schemas: {
    Data: components["read_schemas"]["Data"] | components["read_schemas"]["Data"];
  }
}

Alternatively it could go the other way around, although I feel this makes the typing slightly less useful (adding type guards to code is pretty simple, but they like the union types more) -

export interface components {
  read_schemas: {
    Data: components["schemas"]["Data"] & {
      id: number;
    };
  };
  write_schemas: {
    Data: components["schemas"]["Data"] & {
      password: string;
    };
  };
  schemas: {
    Data: {
      username: string;
    };
  }
}

If there's no readOnly or writeOnly properties then nothing would need to change for any schemas that don't use them.

drwpow commented 3 years ago

Interesting! I had been wondering if readOnly and writeOnly needed representation in TypeScript somehow, but I had personally never used them in a schema. Your example makes sense.

In the past, I’ve been bitten by trying to remap or rename generated types. Because no matter how clever you think you are, there’s always some wild schema someone uses that breaks your assumptions, and you have to release a breaking version (that’s v1 of this library in a nutshell)! So I’m hesitant to have dynamically-generated components that don’t exist in your schema, but I do agree with coming up with some solution.

We could possibly do something clever with Extract<> or Pick<> inside responses and requests? In other words, we leave components["schemas"]["Data"] alone, but modify the properties returned for responses[…] and requests[…] (basically wherever it’s referenced)?

Rycochet commented 3 years ago

That's pretty much what I'm doing now (but with Omit<> due to length - so I only need to add the ones that come from the "opposite" side):

// as Omit
export type IReadData = Omit<"password", components["schemas"]["Data"]>;
export type IWriteData = Omit<"id", components["schemas"]["Data"]>;

// as Pick
export type IReadData = Pick<"id" | "username", components["schemas"]["Data"]>;
export type IWriteData = Pick<"username" | "password", components["schemas"]["Data"]>;

Of the two the Pick is definitely clearer what is inside there, but if the properties are pretty long it could make for a lot of duplication - not exactly a problem for generated code, and far easier to just look at it to see what's included in there - Prettier can handle the line length!)

The potential downside I can see is that components["schemas"]["Data"] is not a union, and is instead assumed to be totally that content, which means code using it could be wrong by accessing both id and password in the same code (type guard + union ftw lol).

Having those read and write within the requests and responses would make it a lot safer though, and for that reason alone makes a lot of sense.

At the very least, adding comments to the end of the schema lets the person reading see that there's something going on? (Could do before, but that can get mixed up with the description if it exists, looking at the code adding a comment to the end isn't done anywhere?)

export interface components {
  schemas: {
    Data: {
      id: number; // response only
      username: string;
      password: string; // request only
    };
  }
}
Rycochet commented 3 years ago

Ok, put a bit of time into this - and got code in there that works, however using Pick<> style actually doesn't work - nested properties breaks it. The root property will be correct, but anything further down doesn't know that it should be in a readonly / writeonly state - will have a try with the multiple-schema style instead but wanted to report that back before anyone else spends time on it!

olivierbeaulieu commented 1 year ago

Hi! I've keeping an eye on this issue for a while and finally decided to take a stab at it. I think I've found a pretty simple solution that doesn't rely on Pick or Omit, and it seems to work pretty well. I haven't written tests yet, but I wanted to share the approach here and get some feedback: https://github.com/drwpow/openapi-typescript/compare/main...olivierbeaulieu:pr/read-write-only?expand=1

In short, we simply add a generic param on the top-level interfaces (components), and set the mode to 'read' or 'write' depending on whether we're looking at a request or a response context. We pass down this generic value every time we consume a $ref, allowing this to work with nested properties.

A few key points:

  1. Importing the schema directly keeps the same behaviour as before, readOnly and writeOnly are ignored
  2. It's easy to get a reference to either version of the schema (components<'read'>['schemas']['MySchema'])

If the approach makes sense, I can get the PR up shortly.

bryanhoulton commented 1 year ago

Any progress on this? Running into similar issues.

tnagorra commented 1 year ago

I tried the approach described by @olivierbeaulieu with minor changes. The approach works nicely.

ashe0047 commented 10 months ago

I tried the approach described by @olivierbeaulieu with minor changes. The approach works nicely.

How did you implement the approach mentioned by @olivierbeaulieu ? Do you mind sharing what changes to make after generating the types using this library?

H01001000 commented 7 months ago

Sorry for asking but any update?

elmarsto commented 7 months ago

Is there anything I can do to expedite this? Some frameworks, like Django, use this feature of OpenAPI extensively. At the moment I'm stuffing extra fields into my requests just to make openapi-fetch happy, which is a pity because the whole point of using it was to get a 1:1 correspondence between frontend and backend :/

NexRX commented 6 months ago

I'm considering using @olivierbeaulieu fork because this feature is really important to me, Would be awesome to see his work merged :)

drwpow commented 6 months ago

Chiming in because I realize I haven’t replied to @olivierbeaulieu’s suggestion: I think that is an elegant solution! And you’re right that’s much more “TypeScript-y” than some of the original ideas about doing deep-schema traversal and modifying things in a more heavy-handed (and unpredictable way). TypeScript’s inference can do a lot when you lean into it!

I’d be open to adding a flag to support this behavior—this seems simple enough—and do expect some edge cases where the inference fails, but maybe that’s OK? If people have been using this method with different schemas for a while, and getting good results, then I’d be happy to add it to the project. Sorry for being accidentally silent on this one (wasn’t intentional), and also very happy to see a lot of people have tested (and validated) this approach in the meantime. At initial proposal I didn’t know if this would be the right way forward or not, but it’s starting to look like it is 🙂.

Happy for anyone to open a PR to add this as an opt-in flag, provided they add some tests as well!

Rycochet commented 6 months ago

My one comment would be having 4 options - "read", "write", "none", "both" - and make "both" the default (which means this would be a non-breaking change for legacy code) - far prefer this method over my original one!

terragady commented 3 months ago

Is there any plan to add this? Or how to handle it for time being? I am creating my own types with Omit and Pick for Payloads (POST) and using plain generated type for GETs responses. It somehow works but is nasty when there are nested interfaces etc. Problem is for example a mentioned above "id" or "createdDate" which is always required and not optional when getting response but very not needed when posting.

I haven't find a simple way to exclude readOnly properties from the object, there is some typescript solution which can work with flat objects but anything nested and more deep objects are failing here.

Writing mostly to have a hook in this issue for any changes :)

Using v7

philbegg commented 3 months ago

Not sure if this will help anyone else but we are using this so that our client requests are typed correctly. Have only just implemented today, so far so good.

It essentially Picks the properties that are not readonly. I then use the exported "paths" from this instead of the openAPI generated ones.

// Import the generated types
import { components, paths as oldPaths } from "./openapi-schema";

// Utility type to identify `readonly` properties
type Writable<T> = Pick<
    T,
    {
        [P in keyof T]-?: (<U>() => U extends { [Q in P]: T[P] }
            ? 1
            : 2) extends <U>() => U extends { -readonly [Q in P]: T[P] } ? 1 : 2
            ? P
            : never;
    }[keyof T]
>;

// Utility type to remove `readonly` properties from request bodies
type RemoveReadonlyFromRequestBody<T> = {
    [P in keyof T]: T[P] extends {
        requestBody: { content: { "application/json": infer U } };
    }
        ? {
              requestBody: {
                  content: {
                      "application/json": Writable<U>;
                  };
              };
          } & Omit<T[P], "requestBody">
        : T[P];
};

// Redefine `paths` type to apply the transformation
export type paths = {
    [P in keyof oldPaths]: RemoveReadonlyFromRequestBody<oldPaths[P]>;
};

export { components };
terragady commented 3 months ago

Not sure if this will help anyone else but we are using this so that our client requests are typed correctly. Have only just implemented today, so far so good.

It essentially Picks the properties that are not readonly. I then use the exported "paths" from this instead of the openAPI generated ones.

// Import the generated types
import { components, paths as oldPaths } from "./openapi-schema";

// Utility type to identify `readonly` properties
type Writable<T> = Pick<
    T,
    {
        [P in keyof T]-?: (<U>() => U extends { [Q in P]: T[P] }
            ? 1
            : 2) extends <U>() => U extends { -readonly [Q in P]: T[P] } ? 1 : 2
            ? P
            : never;
    }[keyof T]
>;

// Utility type to remove `readonly` properties from request bodies
type RemoveReadonlyFromRequestBody<T> = {
    [P in keyof T]: T[P] extends {
        requestBody: { content: { "application/json": infer U } };
    }
        ? {
              requestBody: {
                  content: {
                      "application/json": Writable<U>;
                  };
              };
          } & Omit<T[P], "requestBody">
        : T[P];
};

// Redefine `paths` type to apply the transformation
export type paths = {
    [P in keyof oldPaths]: RemoveReadonlyFromRequestBody<oldPaths[P]>;
};

export { components };

yes but this does not work with nested objects so for example here address will scream that it is missing city and street:

interface Test {
  name: string;
  readonly age: number;
  address?: {
    readonly street: string;
    readonly city: string;
    country: string;
  };
}

const testConst: Writable<Test> = {
  address: {
    country: 'Norway',
  },
  name: 'John',
};
terragady commented 3 months ago

ok I have modified your script and this works fine with nested values, it omits readonly and preserves optional properties:

type IfEquals<X, Y, A = X, B = never> =
  (<T>() => T extends X ? 1 : 2) extends <T>() => T extends Y ? 1 : 2 ? A : B;

export type WritableOnly<T> = {
  [P in keyof T as IfEquals<
    { [Q in P]: T[P] },
    { -readonly [Q in P]: T[P] },
    P
  >]: T[P] extends readonly (infer U)[]
    ? WritableOnly<U>[]
    : T[P] extends (...args: unknown[]) => unknown
      ? T[P]
      : T[P] extends object | undefined
        ? WritableOnly<T[P]>
        : T[P];
};
vickkhera commented 3 months ago

With release 7.0.0, I'm observing that readOnly fields in my OpenAPI spec are marked as readonly in the generated typescript. This is causing typescript errors that I'm trying to assign a value to a readonly field when I construct my object to return from my API code. I don't think that this is a correct interpretation of the OpenAPI flag... the code needs to be able to write those fields as they are part of the response object of the API.

Rycochet commented 3 months ago

@vickkhera I'd call this out specifically as a bad idea - if something in the spec is read-only then general use should have it as readonly - only when filling that data field should it be writable, so it needs either a specific way of getting the writable version, or use something like https://github.com/ts-essentials/ts-essentials/tree/master/lib/deep-writable when you want too write so it's not providing mutation anywhere it shouldn't be. I've not had a need for this for a while but others have been playing with it - if I came back to it now I'd potentially be looking at using a generic for the base, and then extra types for the readonly and writeonly versions, which would make it easier to add a generic option for the "initialise" state too

vickkhera commented 3 months ago

@Rycochet Thanks for the response. I'll check out deep-writable. Is that going to be the recommended way to get the writable version of the type when I'm creating my API response with these generated types? As you note, there needs to be a way to allow writing when you are in the context of creating the response.

Rycochet commented 3 months ago

@vickkhera Purely "this is one generic way to do it in TS" - I'm not involved in this beyond creating the original ticket / proof-of-concept code 😄

vickkhera commented 3 months ago

@Rycochet understood. Despite the downvotes on my original comment, I think there needs to be a way to distinguish from the consumer vs. producer side of the API with respect to the readOnly and writeOnly flags in OpenAPI spec. This interpretation is from the consumer side. I have my workaround, so I can fix up my code and move on.

Rycochet commented 3 months ago

@vickkhera Definitely - locally we've moved to gRPC, and I tend towards "const data = {...} as ReadOnlyData" or similar - it still flags up the errors (though not quite as nicely) - I'm wondering if using the satisfies operator might be a better pattern for this...

terragady commented 3 months ago

@vickkhera hmm I think you are mixing this up, openAPI documentation is for consumer and fields marked as readOnly should not be writeable back, those are for example id, createdDate etc. Why would you want to create a response yourself? Response is what you are getting from API and payload is what you send. Payload should omit readonly keys.

Rycochet commented 3 months ago

@terragady Code needs to be written to take a network packet (normally) and translate it into the right shape of JS objects - if that's written in TS then at some point those are all created, and if that is done in a loop by definition it needs things to be writable - using the data wants the complete shape, but actually providing it to the rest of the application has to break those rules slightly 🙂

vickkhera commented 3 months ago

@terragady I like to have one source of truth for my data structures, so there's no possibility of drift. At some point something has to create this object to send out as the response to the API. I use the generated types to ensure the shape is correct with the help of Typescript, so it needs to be writable.

I disagree with your assertion that the OpenAPI documentation is for consumers only.

BrendanJM commented 3 months ago

Thanks for the discussion from everyone. Running into this now myself, also using Django as a backend. I'm pretty inexperienced with TS, but if I could summarize the above, just adding blank values for unneeded fields is a functional/recommended workaround for now?

e.g. for this schema.yml created by Django:

components:
  schemas:
    AuthToken:
      type: object
      properties:
        username:
          type: string
          writeOnly: true
        password:
          type: string
          writeOnly: true
        token:
          type: string
          readOnly: true
      required:
      - password
      - token
      - username
const response = await client.POST('/api/login/', {
  body: {
    username,
    password,
    token: ''  // TODO: Remove once https://github.com/openapi-ts/openapi-typescript/issues/604 is resolved
  },
  ...
});

I also see the WritableOnly solution but it's a little difficult for me to understand how to use this in practice.

dan-cooke commented 1 month ago

@drwpow I would like to finish the PR @olivierbeaulieu by adding some tests.

I am unable to make a pull request as i am not a contributor.

Would you be able to add me so I can get this solution merged in?

drwpow commented 1 month ago

@dan-cooke you’re welcome to just make your own fork and start another PR! I believe you can start where the original author left off with the GitHub CLI

dan-cooke commented 1 month ago

@drwpow oh duh my bad, I was trying to reopen the original authors fork PR so obviously that isn’t going to work

I’ll get started soon

Thanks!