hono icon indicating copy to clipboard operation
hono copied to clipboard

feat(middleware/predicate): Introduce predicate middleware

Open usualoma opened this issue 1 year ago • 2 comments

Proposes middleware to compose multiple middleware. This middleware provides the following three functions.

  • some : Create a composed middleware that runs the first middleware that returns true.
  • every : Create a composed middleware that runs all middleware and throws an error if any of them fail.
  • except : Create a composed middleware that runs all middleware except when the condition is met.

Usage is as indicated in comments and tests.

This change will allow users to write code similar to that illustrated in the comments below.

https://github.com/honojs/hono/pull/2813#issuecomment-2154572994

import { Hono } from 'hono'
import { ipLimit } from 'hono/ip-limit'
import { bearerAuth } from 'hono/bearer-auth'
import { rateLimit } from '@/my-rate-limit'
import { some, every } from 'hono/predicate' // New middleware
import { getConnInfo } from 'hono/...'

const app = new Hono()

// Very complex access restriction rules.
app.use(
  '*',
  some([
    every([
      ipLimit(getConnInfo, { allow: [ '192.168.0.2' ] },
      bearerAuth({ token })
    ]), // If both are satisfied, rateLimit is not necessary.
    rateLimit()
  ])
)
app.get('/', (c) => c.text('Hello world!'))

Name of middleware

I named it "predicate", but I am not particularly particular about it, so it could be named something else. I also thought of "compose", but it is now "predicate" because it is confusing because it is covered by "compose", which we use internally.

The author should do the following, if applicable

  • [x] Add tests
  • [x] Run tests
  • [x] bun run format:fix && bun run lint:fix to format the code
  • [x] Add TSDoc/JSDoc to document the code

usualoma avatar Jun 09 '24 05:06 usualoma

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.91%. Comparing base (fe7cfcf) to head (af3e7c5). Report is 60 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2941      +/-   ##
==========================================
+ Coverage   94.42%   95.91%   +1.49%     
==========================================
  Files         136      138       +2     
  Lines       13343    13662     +319     
  Branches     2219     2299      +80     
==========================================
+ Hits        12599    13104     +505     
+ Misses        744      558     -186     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 09 '24 06:06 codecov[bot]

Hey @usualoma .

This middleware is unique and exciting, and implementations like except are very interesting. I think there are other use cases like this where we want to control middleware conditions, so let me think about that for a while!

yusukebe avatar Jun 10 '24 15:06 yusukebe

Hi @usualoma

Sorry for the super late reply. My concern is only the naming! Honestly, I don't like a predicate. I also think the compose is best, but as you said, we already have the compose.ts By the way, I've asked ChatGPT:

CleanShot 2024-07-06 at 17 24 44@2x

CleanShot 2024-07-06 at 17 27 40@2x

Hmm. I prefer to arrange or aggregate. Do you have any good other names?

yusukebe avatar Jul 06 '24 08:07 yusukebe

@yusukebe Thank you!

I see. This middleware creates 'composite middleware' in the same way as 'composite function', so I think composite-middleware might be possible if the name is long enough. This name is only written at import time, so it is not a concern if it is somewhat long.

import { some, every } from 'hono/composite-middleware'

But I agree with you on arrange.

Do you want to arrange?

usualoma avatar Jul 08 '24 21:07 usualoma

I think compose is the best choice.

I understand the duplication with an already existing function (compose.ts), but that function is a private function within Hono. So from the user's point of view, there should be no problem.

import { some, every } from 'hono/compose'

EdamAme-x avatar Jul 09 '24 02:07 EdamAme-x

My idea

import { some, every } from 'hono/grouping'

but, duplication with .route

EdamAme-x avatar Jul 09 '24 02:07 EdamAme-x

Hey @usualoma @EdamAme-x !

I'm considering it. Your suggestions are good, but:

  • hono/composite-middleware is long.
  • hono/compose and hono/grouping: users may imagine this as another feature.

And arrange seemed good, but that word also means "to plan, prepare, or organize something".

Then. What about combine?

import { Hono } from 'hono'
import { every, some } from 'hono/combine'

yusukebe avatar Jul 12 '24 02:07 yusukebe

hi @yusukebe How about this? hono/join

join is simple and clear.

EdamAme-x avatar Jul 12 '24 02:07 EdamAme-x

@EdamAme-x

I prefer combine to join. The meaning of combine is better. According to ChatGPT:

In summary, "combine" is about mixing things to create something new, while "join" is about linking things together.

For example, I think it's weird that hono/join exports except. Plus, from the context of Array.prototype.join(), it will just link, but this middleware does more complex things. And combine is enough to be simple and clean.

yusukebe avatar Jul 12 '24 03:07 yusukebe

combine is short, simple, and easy to remember!

I prefer combine to join. The meaning of combine is better. According to ChatGPT:

EdamAme-x avatar Jul 12 '24 06:07 EdamAme-x

OK, let's proceed with combine.

usualoma avatar Jul 12 '24 07:07 usualoma

@usualoma Thanks!

yusukebe avatar Jul 12 '24 10:07 yusukebe

Updated af3e7c5 My work is completed.

Dependency on TrieRouter

There is no functional problem, but I am a little concerned about whether it is acceptable to have a dependency on TrieRouter.

https://github.com/honojs/hono/pull/2941/files#diff-0127ebff640ea2834e4434792875961100daac21bcdaae2d033a6a59815da3bfR132

It would be best if I could use the router I use for that app. But there's no way to get that, so it's not an option at the moment, though.

usualoma avatar Jul 12 '24 11:07 usualoma

@usualoma

There is no functional problem, but I am a little concerned about whether it is acceptable to have a dependency on TrieRouter.

I have considered it for a while and concluded that using TrieRouter is good, as is yours. As you said, it would be best if it could use the current app's router, but using TrieRouter is fine. Many users use the default SmartRouter, which includes TrieRouter.

yusukebe avatar Jul 13 '24 06:07 yusukebe

Hi @usualoma

I've reviewed it. There is no problem, and it looks good! Thank you for your awesome work. I'll merge this into the next branch.

yusukebe avatar Jul 13 '24 09:07 yusukebe

@yusukebe Thank you!

usualoma avatar Jul 13 '24 10:07 usualoma