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

Custom query parser #3667

Open
bompi88 opened this issue Nov 14, 2024 · 9 comments
Open

Custom query parser #3667

bompi88 opened this issue Nov 14, 2024 · 9 comments
Labels
enhancement New feature or request.

Comments

@bompi88
Copy link

bompi88 commented Nov 14, 2024

What is the feature you are proposing?

I want to be able to switch out the default query parser with something like qs.

I am able to use a validator to "hack something together", but if I'm using @hono/zod-openapi that will not be able to use the output of that validator.

Something like the getPath method would be sufficient enough for my use case.

EDIT:

Adding more context to the ticket. I'm using a special query "syntax" which is supported by qs. For example I can do ?amount[lte]=300 which resolves to:

{
  amount: {
    lte: 300
  }
}

For comparison here is how you can change the query parser in express. Not that we should follow their approach because I think that one looks messy.

var express = require('express');
var qs = require('qs');
var app = express();

app.set('query parser', function (str) {
  return qs.parse(str, opts);
});
@yusukebe
Copy link
Member

Hi @bompi88

I like the idea of using something like the getPath method. I will also consider this issue.

@yusukebe
Copy link
Member

It is unrelated to query parsing, but it would be interesting to allow users to specify a custom JSON serializer as a good example of using something like getPath.

@bompi88
Copy link
Author

bompi88 commented Nov 15, 2024

@yusukebe thanks! I've updated the ticket with the example from the #3565 + example how it is done in for example express (please not get too inspired by that approach).

@bompi88
Copy link
Author

bompi88 commented Nov 15, 2024

@yusukebe I know that there is currently an ongoing PR regarding the URLSearchParams that is kind of related (and not), but let me know if there anything I can facilitate regarding this ticket.

@bompi88
Copy link
Author

bompi88 commented Nov 26, 2024

Hi @yusukebe, I wanted to follow up as I haven’t heard back from you yet. We’re highly interested in adding a way to override this functionality. Would you be open to accepting contributions on this topic?

@yusukebe
Copy link
Member

@bompi88

Sorry for the late reply! I am not eager to proceed because the implementation would be complicated. Also, as I said before, I am concerned about the behavior depending on the parameters. If you make a PR, I will review it, but that does not ensure that it will be merged.

@bompi88
Copy link
Author

bompi88 commented Nov 27, 2024

@yusukebe We've started to look into how the code is structured, and we see what concerns you have. I hope that you enable a more plugin approach for the query parser in the future.

We have an ugly hack that we currently use to get around the problem using a middleware.

// middlewares.ts
import qs from 'qs';
import { createMiddleware } from 'hono/factory';
import { URL } from 'url';

export const queryParseMiddleware = createMiddleware((c, next) => {
  const query = qs.parse(new URL(c.req.url).search, { ignoreQueryPrefix: true });
  // eslint-disable-next-line no-param-reassign, @typescript-eslint/no-explicit-any
  c.req.queries = () => query as any;
  return next();
});

// utils.ts
import { createRoute as openApiCreateRoute } from '@hono/zod-openapi';

export const createRoute: typeof openApiCreateRoute = (config) =>
  openApiCreateRoute({
    ...config,
    middleware: config.middleware
      ? [...(Array.isArray(config.middleware) ? config.middleware : [config.middleware]), queryParseMiddleware]
      : queryParseMiddleware,
  });


// routes.ts
import { createRoute } from './utils';

export const list = createRoute({
  path: '/whatever',
  method: 'get',
  request: {
    query: z.object({
      value: z.object({ lte: z.number() })
    })
  },
  tags,
  responses: {
    [StatusCodes.OK]: jsonContent(z.any(), 'The list of whatever'),
  },
});

@yusukebe
Copy link
Member

@bompi88

Setting a value for c.req.queries is only for c.req.valid(), so it does not replace the query parsing in HonoRequest. Therefore, since this middleware is only for validation, I have no motivation to make it a built-in middleware.

@bompi88
Copy link
Author

bompi88 commented Dec 3, 2024

@yusukebe I've been thinking, it does not make sense to me making a PR unless I know you're motivated by the changes. I also do not want to touch more code than necessary. So the part of qs I really care about is the ability to "group" certain query params. I'm not really interested in using qs in itself. I would imagine that it would behave the same way as it currently does in hono, only that it groups params if enabled. So, with grouping "turned on" (can be discussed if it should be a toggle or not):

# simple example
/?g[a]=1&g[b]=2&c=3
{
  g: {
    a: '1',
    b: '2'
  },
  c: '3'
}

# If for some reason someone provides a plain `g`
/?g[a]=1&g[b]=2&g=whatever
{
  g: [
    {
      a: '1',
      b: '2'
    },
    'whatever'
  }
}

And I dont think having more levels than that makes that much sense. And by enabling more levels would complicate things a lot.

Would this proposal align with your vision for Hono? Is it something you’d be interested in integrating into the core?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request.
Projects
None yet
Development

No branches or pull requests

2 participants