-
Notifications
You must be signed in to change notification settings - Fork 50
Support appending header ranges to CorsSettings. #940
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should target main branch, not 1.4.
When it gets merged, we can consider backporting to 1.4.
The code doesn't compile in Scala 2.12 and causes binary compatibility issues for other Scala versions. This will be less of an issue in main branch (no Scala 2.12 support, for example).
http-cors/src/main/scala/org/apache/pekko/http/cors/scaladsl/model/HttpHeaderRange.scala
Outdated
Show resolved
Hide resolved
Will rebase and re-push once you've had a chance for an initial comment.
Looks like that was due to the import changes (I haven't got much 2.12 experience TBH, so that's a bit mysterious) - will tidy all that up and check. Will also have a think about binary compatibility - that might be harder to avoid if the concat API is to be available to the Java DSL, but it could be shunted to a utility method. The javadsl/scaladsl split does make things a bit suboptimal WRT sealing the types, so happy to take suggestions on the typical idioms there. |
|
You can add an excludes file for the mima failures in main branch. |
ef5f9f4 to
2f72cf9
Compare
|
Rebased this on main and changed the target branch for this PR - also fixed the compilation and import issues (have run |
http-cors/src/main/scala/org/apache/pekko/http/cors/javadsl/settings/CorsSettings.scala
Outdated
Show resolved
Hide resolved
|
I've removed new method on |
http-cors/src/main/java/org/apache/pekko/http/cors/javadsl/model/HttpHeaderRange.java
Outdated
Show resolved
Hide resolved
| range match { | ||
| case `*` => `*` | ||
| case Default(headers) => Default(this.headers ++ headers) | ||
| case _ => throw new IllegalArgumentException(s"Unexpected header range implementation type ${range.getClass}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the error message be more like s"Concat is not supported for unexpected header range implementation type ${range.getClass}"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've actually switched this over to use JavaMapping.toScala, which is consistent with other param casting in the Scala DSLs.
Let me know what you think.
Provide a CorsSettings builder method to allow additional header ranges to be added, and the underlying ability to concatenate HttpHeaderRange instances to support it. This supports scenarios where complex CorsSettings defaults are provided by downstream libraries (e.g. pekko-grpc) but still need to be extended/adapted by end user applications.
2f72cf9 to
0de6d0b
Compare
pjfanning
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I work prefer to see other contributors' opinion before merging this.
Ideally, the public methods should have javadoc with @since 2.0.0.
Pushed update with this. |
e78b1b2 to
401aa84
Compare
Provide a CorsSettings builder method to allow additional header ranges to be added, and the underlying ability to concatenate
HttpHeaderRangeinstances to support it.This supports scenarios where complex
CorsSettingsdefaults are provided by downstream libraries (e.g. pekko-grpc) but still need to be extended/adapted by end user applications.A concrete motivating example is the pekko-grpc defaultCorsSettings, which include:
In one of our (development) environments, we need the
Authorizationheader to be added to the allowed headers range, but thewithAllowedHeadersmethod only works by override, requiring us to either maintain our own distinct set of headers (which could diverge from the upstream) or do hacky matches on the unsealedHttpHeaderRangetype.This is a proposal for discussion on a more ergonomic/robust way to extend default settings like this.
The same pattern could equally be applied to the HTTP origin matchers and possibly the exposed headers.
I've tried to make the implementation in-line with conventions (e.g. I would prefer to seal
scaladsl.model.HttpHeaderRangeand have all the matches over it be required exhaustive, but the rest of pekko-http isn't really that way inclined), and theconcatimplementation is also awkward as exposing it to the Java DSL means you have to deal with those type params in the Scala methods and discourage extension.Happy to discuss and rework if maintainers have opinions/better ideas about approach here.