Chain Chi middlewares in reverse order so they execute first-to-last

[ericvolp12]

Resolves reversed-ordering of Chi middleware chaining.

After this change, Chi middlewares will be applied in first-to-last ordering i.e. earlier wired/invoked middlewares will run before later wired middlewares.

This addresses #687 and #786

Not sure how I'd go about testing this since the existing unit tests don't seem to have complex middlewares, could use some advice if we want to include a unit test for this.

Note that this will change behavior for Chi users who wired their middlewares in reverse order to compensate, I think most people at this point have adopted version pinning for code generation since we've had some breaking changes in the past few versions but it's worth noting in the release notes.

[ericvolp12]

Oh, I just realized these issues were already addressed in #688 but it didn't end up being followed-up on.

[jamietanna]

Hey, thanks for this - I think we'll need to make this an opt-in configurable change, rather than a breaking change.

The breaking change I think you're referring to is when we changed the change to how to invoke the generator?

Although, yes, we could've rolled that out in a less breaking fashion, I think that's more noticeable as a breaking change to us subtly changing underlying behaviour, which may not be clear to consumers, and break a lot of applications.

[ericvolp12]

I was more referencing this rollback release after we broke the type signature of some of the generated code for Gin - https://github.com/deepmap/oapi-codegen/releases/tag/v1.8.1

I'll look into turning this into a configuration-driven change though it might be a few days before I can get around to it.

[ericvolp12]

Okay JK I got bored and decided to work on this.

The changes as shown now pass a new TemplateParameters object into the template engine which include an Operations property that is the old ops object that used to be the root of most templates (a []OperationDefinition) as well as an Options property which is just the whole Configuration object.

This allows us to use Configuration values inside the template to conditionally render the template in different ways.

In this case, all I've done is gated the new mechanism for Chi middleware chaining behind the new Compatibility configuration variable Options.Compatibility.ApplyChiMiddlewareFirstToLast.

{{if $.Options.Compatibility.ApplyChiMiddlewareFirstToLast}}
for i := len(siw.HandlerMiddlewares) -1; i >= 0; i-- {
  handler = siw.HandlerMiddlewares[i](handler)
}
{{else}}
for _, middleware := range siw.HandlerMiddlewares {
  handler = middleware(handler)
}
{{end}}

To start, we can have this be an opt-in compatibility option but using this new method of template rendering, it should be trivial to make the first-to-last chaining order an opt-out when we decide enough time has passed to establish it as the default.

This will also open up the ability to have template-specific feature flags in configuration that don't require maintaining lots of additional templates but just feature Option gating logic inside the templates instead.

This change required subtly updating most of the template files to index into the .Operations field of the Template Parameters at the top-level range statements but the end result is strictly the desired change to the Chi template when the compatibility option is selected so there should be no breakage or backward incompatibility without an explicit opt-in from Chi users.

[deepmap-marcinr]

That's a lot of work you did, but options are globally stored on globalstate.options, precisely because it's a pain to pass them around everywhere, so your change can be done without all the changes to all the templates. All you really need to change is that one chi template and pass it an additional flag in its context, rather than changing all of them.

[ericvolp12]

That's a lot of work you did, but options are globally stored on globalstate.options, precisely because it's a pain to pass them around everywhere, so your change can be done without all the changes to all the templates. All you really need to change is that one chi template and pass it an additional flag in its context, rather than changing all of them.

Okay, I didn't see that getting injected as a Template Func, thanks for the pointer!

In other news, I learned what Go Template Funcs are today!

Hopefully this change is a lot simpler and makes more sense, the activation functionality is the same but we're eschewing all my previous changes that were injecting options a second time.

[ericvolp12]

I saw this line while Go Generating locally:

go generate ./...
go: downloading github.com/matryer/moq v0.2.7

Looks like something got updated that the CI is unhappy with there?

[deepmap-marcinr]

It looks like a dependency got updated, it's no big deal, you just haven't fetched it before.

[ericvolp12]

It looks like a dependency got updated, it's no big deal, you just haven't fetched it before.

Does this look okay to merge now or is there anything else you'd like me to change?

[deepmap-marcinr]

Yep, I merged it, thank you. I'm looking to do a new release soon, just waiting on some PR wrapup.

…

________________________________________ From: Eric Volpert ***@***.***> Sent: Saturday, October 22, 2022 10:06 AM To: deepmap/oapi-codegen Cc: Marcin Romaszewicz; Comment Subject: Re: [deepmap/oapi-codegen] Chain Chi middlewares in reverse order so they execute first-to-last (PR #787) It looks like a dependency got updated, it's no big deal, you just haven't fetched it before. Does this look okay to merge now or is there anything else you'd like me to change? — Reply to this email directly, view it on GitHub<#787 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ALKC5DGJ5OBZVPF4H777WWTWEQNINANCNFSM6AAAAAARE46H4A>. You are receiving this because you commented.Message ID: ***@***.***>