-
-
Notifications
You must be signed in to change notification settings - Fork 533
Support "readOnly" and "writeOnly" OpenAPI properties #604
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
Comments
Interesting! I had been wondering if 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 |
That's pretty much what I'm doing now (but with // 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 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 export interface components {
schemas: {
Data: {
id: number; // response only
username: string;
password: string; // request only
};
}
} |
Ok, put a bit of time into this - and got code in there that works, however using |
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 In short, we simply add a generic param on the top-level interfaces ( A few key points:
If the approach makes sense, I can get the PR up shortly. |
Any progress on this? Running into similar issues. |
I tried the approach described by @olivierbeaulieu with minor changes. |
How did you implement the approach mentioned by @olivierbeaulieu ? Do you mind sharing what changes to make after generating the types using this library? |
Sorry for asking but any update? |
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 :/ |
I'm considering using @olivierbeaulieu fork because this feature is really important to me, Would be awesome to see his work merged :) |
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! |
My one comment would be having 4 options - |
Is there any plan to add this? Or how to handle it for time being? 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 |
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',
}; |
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];
}; |
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. |
@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 |
@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. |
@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 😄 |
@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 |
@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 |
@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. |
@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 🙂 |
@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. |
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 |
@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? |
@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 |
@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! |
really interested to have this feature! |
Minor update: The attached PR (#1863) gets it as close as ever, and this has been a long-requested feature. Will have to run this by the other maintainers but it’s a candidate for 8.x since it would be a breaking change. Tracking it in the project for further review. |
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 -
Currently this just generates a single interface similar to -
Unfortunately that makes it unusable for any endpoint as GET doesn't show the
password
and POST etc doesn't want anid
. 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 -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) -
If there's no
readOnly
orwriteOnly
properties then nothing would need to change for any schemas that don't use them.The text was updated successfully, but these errors were encountered: