[typescript] add call-time middleware support

[davidgamero]

ref #18846

make it easier for callers to supply additional middleware for a single method call, without having to pass a whole new Configuration object. The current implementation only allows passing a full new Configuration, which erases SecurityAuthentication headers among other effects, making it difficult to make small edits to requests like overriding a header.

implemented by changing the _options parameter type from undefined | Configuration to undefined | Configuration | []Middleware to preserve backwards compatibility with all existing signatures.

Array.isArray guards are used to narrow the types.

allowing for patterns where middleware can be added for a single call:

api.putPet(
  { name: "mr pet"},
  WithHeader('Content-Type','application/json-patch+json')
)

//WithHeader implements Middleware[] Interface
function WithHeader(key: string, value: string): Middleware[]{
	return [{
		pre: (c: RequestContext)=> {
			return new Promise<RequestContext>((resolve)=>{
				c.headers[key] = value
				resolve(c)
			}
		)},
		post: (c: ResponseContext)=> {return new Promise<ResponseContext>((resolve)=>{ resolve(c) })},
	}]
}

As the PromiseMiddleware is exported in index.ts as Middleware the internal Middleware interface is exported as ObservableMiddleware to allow adding middleware across both the promise and observable layers of the generated client.

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package || exit
  ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
  ./bin/utils/export_docs_generators.sh || exit
  

  (For Windows users, please run the script in Git BASH)
  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
  IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
• File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02) @davidgamero (2022/03) @mkusaka (2022/04) @joscha (2024/10)

[amakhrov]

The linked issue mentions providing headers as an option (a property of _options). It would be a much simpler way to achieve the task, IMO.
What are other uses cases that would require passing a full-featured middleware instead?

[davidgamero]

I dont have a concrete example that requires more than headers.

My initial goal was to be consistent with the existing codebase's patterns which were mostly built around chaining middleware.

Happy to switch to header-only and revisit if it's insufficient later

More like:

args?: {
  headers?: [key: string]: string,
  configuration ?: Configuration
}

[amakhrov]

Using middleware for this is ambiguous because Configuration.middleware is an array.

So if we provide a new array with just a single header-appending middleware, does it mean we want to append it to the existing ones? prepend? or replace?

[davidgamero]

Agreed it is't immediately clear where the middleware would go.

Since additional middleware is most specific (as opposed to replacing a whole configuration), i lean towards running it last on request and first on response so it can override the existing RequestContext as needed.

As headers can also be manipulated in middleware, for the header-only approach i expect it would be executed last as an override too. The specified headers would replace existing keys if they exist.

Potentially clearer to call the arg headerOverrides

[amakhrov]

Btw, if we go with the middleware approach -isn't it already possible to to what we need by providing middleware in the _options arg?
maybe what's missing is the code to merge _options with the standard configuration in this.configuration (as currently _options will completely replace the latter)?

[davidgamero]

Yes that's a possibility, we could allow something like a json merge patch.

The merge would have to allow appending middleware at the end since the order of execution would matter for those.
The rest of the fields are relatively static so merging non-empty fields could work if the types are optional.

At that point potentially just having a middleware merge strategy option could cover the full use case. The other fields would only replace if they are non-empty.

[davidgamero]

based on input so far, i've modified the approach to include the following features:

• no type union & destructuring support (@macjohnny )
• avoiding ambiguity for middleware override in two places (@amakhrov )

in this case, the core changes are making the _options type a new interface that is fully optional. Passing a full configuration will override all properties exactly as before.

export interface ConfigurationOptions {
    baseServer?: BaseServerConfiguration;
    httpApi?: HttpLibrary;
    middleware?: Middleware[];
    middlewareMergeStrategy?: 'replace' | 'append' | 'prepend'; // default merge is to replace for backward compatibility
    authMethods?: AuthMethods;
}

Undefined properties will fall back to the base config. (Still working on this bit)

The default behavior for all properties is a replace.

As middleware is an array, an optional middlewareMergeStrategy allows choosing to concat instead

[davidgamero]

still adding some tests to ensure pre/post is working how we expect

[macjohnny]

@amakhrov can you give your final approval so we can merge?

[davidgamero]

updates:
pushed tests covering the 'prepend', 'replace', and 'append' merge strategies via a middleware function call-order fake that records each middleware method called

this caught the reversed-order bug for post() calls in which the post method of middleware is called in a top-down order instead of the expected bottom-up order

ex:

when using two middleware: Outer and Inner, 
passed in an array as [Outer,Inner] the expected call order of pre/post functions is

Outer.pre()
Inner.pre()
Inner.post()
Outer.post()

this was not the case under the current traversal, and the post() calls were instead called in reverse order.

added a fix by traversing middleware in reverse order for post() method pipe chaining

tests were added covering the three middlewareMergeStrategies for a single middleware in the base configuration and a single middleware passed at call time

[davidgamero]

@amakhrov i've updated to add test coverage for the merge strategies validating default replacement behavior

[davidgamero]

bump on this, would love to get more feedback and get this merged

[macjohnny]

@amakhrov?

[joscha]

Bit late, sorry, don't let my comments block you if they don't seem to make immediate sense.

[joscha]

I might be missing something or could this be:

Suggested change

	_config = {
	baseServer: _options.baseServer || this.configuration.baseServer,
	httpApi: _options.httpApi || this.configuration.httpApi,
	authMethods: _options.authMethods || this.configuration.authMethods,
	middleware: allMiddleware || this.configuration.middleware
	}
	_config = {
	...this.configuration,
	..._options,
	middleware: allMiddleware || this.configuration.middleware,
	};

when inversify is on, we have _config = _options as well, which suggests these two objects are interchangeable in their shape (without checking)

[davidgamero]

i tried this out, but some undefined fields were still making it through instead of being preferred from the configuration

[joscha]

Am I missing code here where allMiddleware is a reversed list of the original before?

Thought: If I had two middlewares before this change that did something based on each other - for the sake of an example: one that parses JSON and one that transforms it - i.e. they couldn't be run in any order but one is dependent on the mutation of the other, then this change would mean that after an upgrade of my generated code it would now try to transform end then parse?

[davidgamero]

you are correct that the middleware was not a reversed list before, so your example is correct in that by upgrading this code the order of two post() middleware methods would be changed.

my interpretation of the current implementation is that it is not actually executing a middleware pattern

in your example, you'd have two middlewares: a ParseJSON and a TransformJSON each composed of a pre and post method. the presumed goal would be to allow the TransformJSON to wrap the ParseJSON such that it does not access non-json content

the current call order would be
ParseJSON.pre() => TransformJSON.post() => external response => ParseJSON.post() => TransformJSON.post()

since the middleware are being called in the same order while traversing down and up the call stack, there is no implementation that allows encapsulating something like a JSON Parse on both ends. at one side, the Transform is being called on non-parsed JSON. Reversing this list correctly implements the middleware pattern to allow each middleware to send requests to and receive responses from the next middleware. Unparsed JSON would have to reach the transform middleware on either the request or response calls since they're in the same order, assuming the pre and post methods are doing inverse steps of parsing/stringifying

the current _options parameter replaces the whole middleware array, so stacking middleware requires reconstructing the full configuration object, which is why i suspect this wasn't identified sooner.

this would change the order, so we can push that particular change to a later release if needed, but I understood this to be a bug

[macjohnny]

@joscha do you think this PR is ready to be merged?