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.

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

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

Open
Rycochet opened this issue May 20, 2021 · 29 comments
Open

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

Rycochet opened this issue May 20, 2021 · 29 comments
Labels
enhancement New feature or request good first issue Straightforward problem, solvable for first-time contributors without deep knowledge of the project openapi-ts Relevant to the openapi-typescript library PRs welcome PRs are welcome to solve this issue!

Comments

@Rycochet
Copy link

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
Copy link
Contributor

drwpow commented May 26, 2021

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
Copy link
Author

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
Copy link
Author

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
Copy link

olivierbeaulieu commented Feb 15, 2023

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
Copy link

Any progress on this? Running into similar issues.

@tnagorra
Copy link

tnagorra commented Sep 8, 2023

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

@ashe0047
Copy link

ashe0047 commented Nov 27, 2023

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
Copy link

Sorry for asking but any update?

@elmarsto
Copy link

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
Copy link

NexRX commented Mar 10, 2024

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

@drwpow
Copy link
Contributor

drwpow commented Mar 12, 2024

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!

@drwpow drwpow added enhancement New feature or request PRs welcome PRs are welcome to solve this issue! good first issue Straightforward problem, solvable for first-time contributors without deep knowledge of the project openapi-ts Relevant to the openapi-typescript library labels Mar 12, 2024
@Rycochet
Copy link
Author

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link
Author

@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
Copy link

@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
Copy link
Author

@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
Copy link

@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
Copy link
Author

Rycochet commented Jun 25, 2024

@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
Copy link

@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
Copy link
Author

@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
Copy link

@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
Copy link

BrendanJM commented Jun 30, 2024

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
Copy link

@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
Copy link
Contributor

drwpow commented Aug 17, 2024

@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
Copy link

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Straightforward problem, solvable for first-time contributors without deep knowledge of the project openapi-ts Relevant to the openapi-typescript library PRs welcome PRs are welcome to solve this issue!
Projects
None yet
Development

No branches or pull requests