Skip to content

Query parameter serialization for objects is not the openapi default #1533

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

Closed
1 task
acerola1 opened this issue Feb 14, 2024 · 4 comments · Fixed by #1534
Closed
1 task

Query parameter serialization for objects is not the openapi default #1533

acerola1 opened this issue Feb 14, 2024 · 4 comments · Fixed by #1534
Labels
bug Something isn't working openapi-fetch Relevant to the openapi-fetch library

Comments

@acerola1
Copy link

Description

When serializing query parameters for objects using the library, the serialization method employed does not match the OpenAPI 3.0 specification's default behavior. Specifically, the library uses a "deepObject" serialization style instead of the expected "form" serialization with explode=true, leading to an incorrect query parameter format.
Openapi spec

Reproduction
Calling the:

const {
  data, // only present if 2XX response
  error, // only present if 4XX or 5XX response
} = await GET("/user", {
  params: {
    path: {id: {role: "admin", firstName: "Alex"}},
  },
});

the url parameters are: "id[role]=admin&id[firstName]=Alex"

Expected result

the url parameters should be: "role=admin&firstName=Alex"

Checklist

@acerola1 acerola1 added bug Something isn't working openapi-fetch Relevant to the openapi-fetch library labels Feb 14, 2024
@drwpow
Copy link
Contributor

drwpow commented Feb 14, 2024

You are right the docs are incorrect and says the default is form/explode when it’s deepObject. The docs should be fixed, but I’m unsure if the default should be changed again, because there were many issues opened about simpler serializers being used.

No matter what the default is, there will always be a large number of users whose backends implement a different style. In that vein, would it be worthwhile to ship some additional querySerializers that handle alternate syntaxes? Perhaps something like:

import createClient from 'openapi-fetch';
import { formExplode } from 'openapi/serializers';

createClient({
  querySerializer: formExplode
});

@acerola1
Copy link
Author

Yes it is not easy. Maybe it should be more finer grained. Array and object serialization can be different. I created a serializer for formExplode for object. It may help someone.

const serializePrimitive = (key, value) => `${key}=${encodeURIComponent(String(value))}`;

const serializeArray = (key, value, serializer) => {
  if (!value.length) return undefined;
  return value
    .map((item) => serializer(key, item))
    .filter(Boolean)
    .join('&');
};

const serializeObject = (value, serializer) => {
  const entries = Object.entries(value).filter(([_, v]) => v !== undefined && v !== null);
  if (!entries.length) return undefined;
  return entries
    .map(([k, v]) => serializer(k, v))
    .filter(Boolean)
    .join('&');
};

const queryParamSerializer = (key, value) => {
  if (value === null || value === undefined) return undefined;

  const type = typeof value;
  if (type === 'string' || type === 'number' || type === 'boolean') return serializePrimitive(key, value);
  if (Array.isArray(value)) return serializeArray(key, value, queryParamSerializer);
  if (type === 'object') return serializeObject(value, queryParamSerializer);

  return undefined;
};

export const querySerializer = (q) => {
  const search: string[] = [];
  if (q && typeof q === 'object') {
    for (const [k, v] of Object.entries(q)) {
      const value = queryParamSerializer(k, v);
      if (value) {
        search.push(value);
      }
    }
  }
  return search.join('&');
};

@drwpow
Copy link
Contributor

drwpow commented Feb 14, 2024

I really like the idea of separating object + array serialization! I don’t think anyone’s suggested that before. Adding that would make it easier for people to customize their own serializers for sure, and that could be added in a backwards-compatible way.

@drwpow
Copy link
Contributor

drwpow commented Feb 14, 2024

Also digging into this, I realize that we don’t support path serialization very well either, but should. Will add a minor breaking change to the library to handle this (and provide for easier overrides).

Since this is a breaking change, this will probably be released in 0.9.0 along with #1521.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working openapi-fetch Relevant to the openapi-fetch library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants