-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Axios Safe JSON parsing #2927
base: master
Are you sure you want to change the base?
Axios Safe JSON parsing #2927
Conversation
…may change the data to return as a valid JS object/array/etc. While not perfect, it will simply return the object it has in the case that it throws an error. Also adds in the jsonParse function call for HandleReferences in case an override wants to be provided though code.
@RicoSuter Any idea on when this can get looked at and implemented? |
@thijscrombeen @JyrkiHei @nunonux can you have a look? |
Found the template directory functionality so I'm not hurting for this, and not wed to the implementation. Thanks for anyone that digs in. :) |
I've checked your branch and was able to successfully generate a typescript file with your changes.
While above code doesn't give any compiler errors, it is not really correct. We could change the generated code to explain that it can return either boolean or null but then it no longer works with your change.
In which cases do you use interceptors that would make the JSON.parse fail? |
So, given these parameters: Here's an example generation based on a pretty simple "get by id" type endpoint client:
Note, this stack overflow answer that basically says that Axios automatically parses json results, unless it specifically is fed transformers (or even, a lack of them, because it will remove the default one). https://stackoverflow.com/questions/41013082/disable-json-parsing-in-axios Note this important bit of code from above: It's specifically modifying the response from the request above. The only valid thing that can come through as _response.data is a string representation of JSON. Currently there isn't anything that, for example, removes the transformers so that is the case all the time, and that seems kind of arbitrarily limiting doesn't it? This is a super specific thing that it needs in order for it to function, and I'm halfway convinced it probably shouldn't even be trying to parse this data. |
I just checked the Axios code and the default implementation of transformResponse is as follows:
Would it be best to by default overwrite this so that axios itself does not do any parsing? |
@thijscrombeen I'd think we just don't try to parse it. Seems like it's trying to solve for a non-problem within this specific generation context. I think that would solve for all of it yeah? |
At the moment (nswag 13.7.0.0), when /template:Axios and /typeStyle:Interface, then the output of the response process throws Json exception SyntaxError: Unexpected token o in JSON at position 1 in this line (when trying to parse JSON) let resultData200 = _responseText; guess this PR gonna fix this, right? @NewDark90 |
I have exactly the same issue as the one reported by @mohsenZaim when toggling Interfaces with Axios. Last week, I opened #2966 to suggest a fix for that issue (then closed it as I saw the current PR would be even better) but I believe that the current PR covers more edge cases. |
Is there any way to use the .liquid based templates locally and generate a correct output for the axios template? in swagger i used to modify .mustach files and use those instead of default templates to generate my code. here for this nswag, i havent found a way yet? @RicoSuter do we have such feature to use locally modified .liquid templates to generate files, so we can use this axios based template ? or the only solution at the moment would be to wait for this bug to be fixed? :) |
You can test the changed tpl with an override: As soon as i have the ok that its fine ill merge the pr. |
{% else -%} | ||
result{{ response.StatusCode }} = JSON.parse(resultData{{ response.StatusCode }}); | ||
try { result{{ response.StatusCode }} = (_responseText === "" ? null : <{{ response.Type }}>JSON.parse(resultData{{ response.StatusCode }}, this.jsonParseReviver)); } | ||
catch(err) { result{{ response.StatusCode }} = _responseText; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether this is a good idea here.
The generated code should not require external transformations in general (leaky abstraction) and should work out-of-the-box and thus deserialization should work as expected and a JSON exception should be thrown, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my suggested fix (see https://github.com/RicoSuter/NSwag/pull/2966/files), I just removed JSON.parse(...)
. It seems that it is not needed at all in the context of axios if I am not wrong. See explanation here: #2966 (comment) 🤔
Thanks @RicoSuter :) |
@mohsenZaim @dubzzz Could you provide a swagger/open api json document and maybe the api controller implementation if the api is in c#? |
@thijscrombeen The failure happens whenever I try to return a simple |
Arrays for example. Here is swagger contract sample: "/api/Locations": { here is the output when use axios + type interface inside the process method. if (status === 200) { Not sure why the template is JSON parsing here ? and why not just return the response.body. or at least check if it is parsable. and here is sample C# in the backend
|
As explained above Axios is also doing a JSON.parse. When using interface typestyle NSwag is also doing a JSON.parse which happens after the one from Axios. At that time it's no longer a string but it's a json object and it fails. This can be overridden by setting the following on the axios client:
For example in the constructor of the generated client:
If needed I can make a seperate PR for this change to add the transformresponse in the case of typestyle interface. |
Would be great if you can add this feature in a seperate PR that can be merged and release a new version, so we use that instead of overriding the template on our side. |
@thijscrombeen But why override that functionality to stop parsing, just to do it ourselves anyway down the line? Doesn't make sense to me |
Based on the discussion above, I recently tried to use the following instance of axios in my codebase and it worked like charm with interface mode on: axios.create({
transformResponse: [
(data) => {
// The output of this function is a string that can be passed to JSON.parse
try {
// types such as strings, numbers are already parsed so JSON.parse will fail on them
// complex objects on the contrary come as strings
JSON.parse(data);
return data;
} catch (err) {
return JSON.stringify(data);
}
},
],
}); I use it as a temporary workaround for the moment. UPDATE: The trick above is incomplete. Date fields will not be Date anymore but raw strings. |
This issue is still here. I am not sure why the template is trying to parse the response as Axios already does that for us. Generating a client from the pet store: https://petstore.swagger.io/v2/swagger.json with axios and interface DTOs will create a client that has this behavior. Here is an example config: {
"runtime": "Net50",
"defaultVariables": null,
"documentGenerator": {
"fromDocument": {
"url": "https://petstore.swagger.io/v2/swagger.json",
"output": null,
"newLineBehavior": "Auto"
}
},
"codeGenerators": {
"openApiToTypeScriptClient": {
"className": "{controller}Client",
"moduleName": "",
"namespace": "",
"typeScriptVersion": 2.7,
"template": "Axios",
"promiseType": "Promise",
"httpClass": "HttpClient",
"withCredentials": false,
"useSingletonProvider": false,
"injectionTokenType": "OpaqueToken",
"rxJsVersion": 6.0,
"dateTimeType": "String",
"nullValue": "Undefined",
"generateClientClasses": true,
"generateClientInterfaces": true,
"generateOptionalParameters": false,
"exportTypes": true,
"wrapDtoExceptions": false,
"exceptionClass": "ApiException",
"clientBaseClass": null,
"wrapResponses": false,
"wrapResponseMethods": [],
"generateResponseClasses": true,
"responseClass": "SwaggerResponse",
"protectedMethods": [],
"configurationClass": null,
"useTransformOptionsMethod": false,
"useTransformResultMethod": false,
"generateDtoTypes": true,
"operationGenerationMode": "MultipleClientsFromFirstTagAndOperationId",
"markOptionalProperties": true,
"generateCloneMethod": false,
"typeStyle": "Interface",
"enumStyle": "Enum",
"useLeafType": false,
"classTypes": [],
"extendedClasses": [],
"extensionCode": null,
"generateDefaultValues": true,
"excludedTypeNames": [],
"excludedParameterNames": [],
"handleReferences": false,
"generateConstructorInterface": true,
"convertConstructorInterfaceData": false,
"importRequiredTypes": true,
"useGetBaseUrlMethod": false,
"baseUrlTokenName": "API_BASE_URL",
"queryNullValue": "",
"useAbortSignal": false,
"inlineNamedDictionaries": false,
"inlineNamedAny": false,
"templateDirectory": null,
"typeNameGeneratorType": null,
"propertyNameGeneratorType": null,
"enumNameGeneratorType": null,
"serviceHost": null,
"serviceSchemes": null,
"output": "index.ts",
"newLineBehavior": "Auto"
}
}
} |
Because some properties (eg dates) need to be serialized differently, and sometimes the property names on the wire (in JSON) do not match the property names in the generated TypeScript classes (eg on the wire we have 'first-name' which cannot be used as a TS/JS property name and is renamed to "firstName"). If you do not care for date conversion or property rename then use interface instead of class output, in this case we could keep axios auto-deserialization enabled - otherwise we need to turn it off. |
In the case of $ref resolution it probably would be needed as axios does not pass the special reviver function in... |
I do not use Axios and I have no clue whether this here is fine or not. What's the current state? |
@thijscrombeen @RicoSuter is there any update on this? I would love to take the nswag generated clients into proper use in our microservice environment, but I need it work well. Update: I'll be using this without |
Is this PR still valid? |
@RicoSuter Yes, I believe the PR is still valid and would fix several open issues as described here: #5003 |
JSON.parse will throw on any input that isn't a string. Interceptors may change the data to return as a valid JS object/array/etc even if the client instance was controlled enough to ensure that every returned response data point is a JSON parseable string.
While not perfect, it will simply return the object it has in the case that it throws an error. This update also adds in the jsonParse function call for HandleReferences in case an override wants to be provided though code.
One mention of the issue here. 5d548d7#r37786449