[BUG] [typescript-axios] Query parameter passed as "null" when value is null

neocotic commented 2 years ago

Bug Report Checklist

[x] Have you provided a full/minimal spec to reproduce the issue?
[x] Have you validated the input using an OpenAPI validator (example)?
[ ] Have you tested with the latest master to confirm the issue still exists?
[x] Have you searched for related issues/PRs?
[x] What's the actual output vs expected output?
[ ] [Optional] Sponsorship to speed up the bug fix or feature request (example)

Description

When using the typescript-axios generator to generate an API from a spec with optional query parameters, I've noticed that passing null as a value for such parameters, this results in a query string containing like ?queryParamName=null where I would have expected ?queryParamName= or for it to be omitted from the query string entirely.

This is causing problems as it's resulting in "null" strings being passed into the server unexpectedly, requiring workarounds in some cases to stop "null" being persisted or transmitted internally.

This becomes increasingly likely to happen as the number of parameters grows or when a client user wants to specify Axios config options, requiring them to skip some arguments.

Those importing the client library as TypeScript (or at least, using the TypeScript definitions output file) are unlikely to have this issue as they'll see that the value cannot be null and can be expected to pass undefined instead, however, those using the compiled JavaScript output can and should be expected to pass null for an optional method argument to skip it.

Although I haven't tested with v6 (beta) it looks like the problem exists there too based on what I can see in master.

openapi-generator version

5.4.0

OpenAPI declaration file content or url

openapi: "3.0.0"
info:
  title: Swagger Petstore
  version: "1.0.0"
servers:
  - url: https://petstore.swagger.io
paths:
  /pet:
    get:
      summary: Finds Pets
      operationId: findPets
      parameters:
        - name: "status"
          in: "query"
          required: false
          schema:
            type: "string"
            enum:
              - "available"
              - "pending"
              - "sold"
      responses:
        "200":
          description: "successful operation"
          content:
            application/json:
              schema:
                type: "array"
                items:
                  $ref: "#/components/schemas/Pet"
        "400":
          description: "Invalid status value"
          content:
            application/json: {}
components:
  schemas:
    Category:
      type: "object"
      properties:
        id:
          type: "integer"
          format: "int64"
        name:
          type: "string"
    Tag:
      type: "object"
      properties:
        id:
          type: "integer"
          format: "int64"
        name:
          type: "string"
    Pet:
      type: "object"
      required:
        - "name"
        - "photoUrls"
      properties:
        id:
          type: "integer"
          format: "int64"
        category:
          $ref: "#/components/schemas/Category"
        name:
          type: "string"
          example: "doggie"
        photoUrls:
          type: "array"
          items:
            type: "string"
        tags:
          type: "array"
          items:
            $ref: "#/components/schemas/Tag"
        status:
          type: "string"
          enum:
            - "available"
            - "pending"
            - "sold"

Generation Details

The below configuration is being used:

generatorName: typescript-axios
outputDir: ./test-output
inputSpec: ./test-input/spec.yaml
packageName: test
additionalProperties:
  packageVersion: "1.0.0"
  npmName: "test"
  npmVersion: "1.0.0"

Steps to reproduce

Generate the configuration provided previously and then pass null as the optional query string parameter:

import { DefaultApi } from 'test';

export function findPets() {
  const api = new DefaultApi();
  return api.findPets(null);
}

Related issues/PRs

None.

Suggest a fix

The problem appears to be that null values are being passed to URLSearchParams#set which results in the named parameter being serialised with a "null" value. This is contrary to querystring.stringify which treats null in the same way as undefined and results the parameter being serialised with no value (i.e. empty).

While this issue is being reported against the typescript-axios generator as that's the one I'm using, I've noticed that it's also apparent in some, but not all, other TypeScript generators. Those that used querystring directly or indirectly, do not suffer from this issue. However, for typescript-axios, I believe that the fix would be to simply add a null check to the generated setSearchParams function here.

adrianwix commented 2 years ago

Why don't you pass undefined? Null is a value whereas in "?queryParamName=" has no value

neocotic commented 2 years ago

@adrianwix I would happily do this, however, the problem is that, like a lot of other users of the generator, I simply generate the client but do not use it myself heavily. I can understand why any developer would pass null to skip this argument. It has been a pattern for a very long time with optional function arguments in JS.

A key problem I also see is inconsistency. As I mentioned, some other typescript-based generators don't have this problem as they use querystring. I'm under the impression that this behaviour is in fact somewhat accidental.

adrianwix commented 2 years ago

I see. I am not sure if the same logic applies. But there are some cases where null is needed. For example, in PATCH request a property with null can mean "set the value of the property to null" whereas if the property has the value undefined it means that it was not defined (even if the developer would do property: undefined since that is the same as not defining that property)

When using the typescript-axios generator to generate an API from a spec with optional query parameters, I've noticed that passing null as a value for such parameters, this results in a query string containing like ?queryParamName=null where I would have expected ?queryParamName= or for it to be omitted from the query string entirely.

By analogy, I may want to query some entities where a field is null. So how would I do it, if setting a query to null makes it to be omitted

neocotic commented 2 years ago

While I agree about PATCH in principle, this could still be achieved via the request body. My bug report is specifically for query string parameters. I think this would be far more appropriate, personally.

neocotic commented 2 years ago

By analogy, I may want to query some entities where a field is null. So how would I do it, if setting a query to null makes it to be omitted

You could simply pass "null" string in such a case, which I believe is far more reasonable and deliberate than passing null and it being serialised as a string. Even with typescript consumers in mind this can be achieved and simply cast to the desired type.

ColeMurray commented 2 years ago

Is this still an issue if you set nullable: true on the parameter?

Null
OpenAPI 3.0 does not have an explicit null type as in JSON Schema, but you can use nullable: true to specify that the value may be null. Note that null is different from an empty string "".

https://swagger.io/docs/specification/data-models/data-types/#Null

neocotic commented 2 years ago

I'll give this a shot and let you know how I get on. From the generation code, I'm not sure I see how it would differ.

drew-royster commented 2 years ago

I also found this to be unintuitive. Luckily I am the creator and the consumer of the api-client so I can put undefined in even though it does feel a bit wrong to do so.

OpenAPITools / openapi-generator