Add headers to all API methods

[eric-maynard]

Currently, Polaris APIs drop any headers that are not explicitly called out in the API spec.

To support cases where a client may pass in a specific header to control some behavior of the API -- particularly cases where this API is an Iceberg API and we would prefer not to explicitly add the header to the spec -- we can add the headers to the generated methods which handle the REST APIs. This also helps in cases where someone has deployed their own Polaris instance and wants to introduce some custom behavior to an API without changing the spec.

[singhpk234]

Thank you so much @eric-maynard !

I think it makes a lot of sense to support accepting any headers that the client send, and help people managing their own IRC server, specially since considering IRC client can send any custom headers.
https://github.com/apache/iceberg/commits/main/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

would be really nice if we can just add one e2e test !

[singhpk234]

would be really nice if we can add some e2e test, for this ?

[eric-maynard]

Yeah I would love to, but there's nothing actually consuming the headers right now... in a sense, the template change is the actual change, and the test is that we needed to update all the other files in order to make things pass.

[dimas-b]

WDYT about using a filter plus a request-scoped bean for passing headers to implementation classes?

From my POV that approach:

• Keeps method parameter tuples narrowed down to what is defined in the API
• Affects only the code that needs to know headers
• Can be switched on/off in specific builds / deployments without affecting the main codebase.

[dimas-b]

to clarify: normally HttpHeaders can be injected by CDI automatically in HTTP contexts. A custom bean may be required only to support non-HTTP requests like async tasks.

[eric-maynard]

Keeps method parameter tuples narrowed down to what is defined in the API

Plus RealmContext and SecurityContext

Affects only the code that needs to know headers

This sounds like a good alternative but I'm a little unsure how this works. For example, suppose we add some new feature that relied on a custom header passed to e.g. loadTable. We would end up injecting the request-scoped bean into the entire IcebergCatalogAdapter, right? Or is there some way to inject it just into that method?

Can be switched on/off in specific builds / deployments without affecting the main codebase.

Adding the headers here doesn't functionally affect the main codebase, and if you're talking about a nonfunctional impact (e.g. maintenance cost) then it sounds like the proposed alternative would still have that impact?

[dimas-b]

If IcebergCatalogAdapter needs heads in any method, the headers will have to be injected all the time (via constructor). However, the methods that do not need the headers will not have to have them in method parameters.

The overhead of this injection is probably negligible because the headers are processed one way or another for every request anyway.

Using method parameters, on the other hand forces all implementations to have those parameters if only one of them actually needs the headers.

[dimas-b]

Also, if headers are needed in a class N levels down the call chain, the top N-1 levels do not need to pass the headers down - injection will happen into the target object directly (once at level N).

[adutra]

I agree that we need to weigh the pros and cons of each approach. The method signatures are becoming quite big, while components like HttpHeaders, SecurityContext (and RealmContext) are all injectable. Quarkus lists all context objects that are injectable here: https://quarkus.io/guides/rest#accessing-context-objects.

[eric-maynard]

I see. Yeah, this sounds like a good idea. However based on that link, it looks like we should probably be doing the same with with HttpHeaders that we do with e.g. SecurityContext?

[snazy]

Why not use the specific @HeaderParam?

[eric-maynard]

Why not use the specific @HeaderParam?

This is discussed in brief in the description, but there are a couple of points here.

1. You may want to add a HeaderParam(s) to the IRC part of the spec, which we try to keep as close to upstream as possible.
2. The Iceberg REST spec supports clients sending custom headers without the OpenAPI spec having to be specific about this. From what I can see, OpenAPI doesn't actually have a way to capture the fact that arbitrary headers can be a valid input and this is just sort of implied. The change in this PR makes our code generation more in line with this apparent implication.
3. Deployments of Polaris may want to alter the behavior of one API without having to diverge from the spec, which should be supported per (2) but is currently not due to the fact that we drop all headers except those called out in a HeaderParam during code generation.

With all this being said, I think using HeaderParam is a valid alternative.