Skip to content
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

Use modeled headers in CORS middleware #6790

Merged
merged 9 commits into from
Nov 8, 2022

Conversation

danicheg
Copy link
Member

@danicheg danicheg commented Nov 6, 2022

This resolves some // TODOs.

Copy link
Contributor

@satorg satorg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@satorg
Copy link
Contributor

satorg commented Nov 6, 2022

One more thing to note (not directly related to this PR though, but still).

I noticed that both Access-Control-Allow-Methods and Access-Control-Max-Age header models are located in files with quite unusual back-ticked names: `Access-Control-Allow-Methods`.scala and `Access-Control-Max-Age`.scala correspondingly. There are only three those files with such names (along with another one `Idempotency-Key`.scala).

I think it would be better to get rid of backticks in filenames – I personally had been struggling to find those files until I realized that there are backticks in their names. Not necessary to be addressed in this PR, of course...

@danicheg
Copy link
Member Author

danicheg commented Nov 7, 2022

There are only three those files with such names (along with another one Idempotency-Key.scala).

I'm not sure about that 🤔
Screenshot 2022-11-07 at 21 12 08

@satorg
Copy link
Contributor

satorg commented Nov 7, 2022

Oh, I'm talking about file names, not class names. Backticked class names look fine to me. But their corresponding file names look this way currently:

.../http4s/core/shared/src/main/scala/org/http4s/headers (series/0.23)$ ls -1
Accept-Charset.scala
Accept-Encoding.scala
Accept-Language.scala
Accept-Patch.scala
Accept-Post.scala
Accept-Ranges.scala
Accept.scala
Access-Control-Allow-Credentials.scala
Access-Control-Allow-Headers.scala
Access-Control-Expose-Headers.scala
Access-Control-Request-Method.scala
Age.scala
Allow.scala
Authorization.scala
Cache-Control.scala
Connection.scala
Content-Disposition.scala
Content-Encoding.scala
Content-Language.scala
Content-Length.scala
Content-Location.scala
Content-Range.scala
Content-Type.scala
Cookie.scala
Cross-Origin-Resource-Policy.scala
DNT.scala
Date.scala
ETag.scala
Expires.scala
Forwarded.scala
ForwardedRenderers.scala
HeaderCompanion.scala
Host.scala
If-Match.scala
If-Modified-Since.scala
If-None-Match.scala
If-Range.scala
If-Unmodified-Since.scala
Keep-Alive.scala
Last-Event-Id.scala
Last-Modified.scala
Link.scala
LinkValue.scala
Location.scala
Max-Forwards.scala
Origin.scala
Proxy-Authenticate.scala
Proxy-Authorization.scala
Range.scala
Referer.scala
Retry-After.scala
Sec-WebSocket-Accept.scala
Sec-WebSocket-Key.scala
Sec-WebSocket-Version.scala
Server.scala
Set-Cookie.scala
SourceMap.scala
Strict-Transport-Security.scala
Trailer.scala
Transfer-Encoding.scala
Upgrade.scala
User-Agent.scala
WWW-Authenticate.scala
X-B3-Flags.scala
X-B3-ParentSpanId.scala
X-B3-Sampled.scala
X-B3-SpanId.scala
X-B3-TraceId.scala
X-Forwarded-For.scala
X-Forwarded-Proto.scala
`Access-Control-Allow-Methods`.scala
`Access-Control-Max-Age`.scala
`Idempotency-Key`.scala
package.scala

So there are only three files in the whole directory that have backticked file names. And such namings break their sorting order in particular. And I think that backticked file names look weird no matter what.

@danicheg
Copy link
Member Author

danicheg commented Nov 7, 2022

Ah, now I see. Ok, let's fix that too.

@danicheg danicheg requested a review from satorg November 7, 2022 20:29
Copy link
Contributor

@satorg satorg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thank you!

@danicheg danicheg merged commit d29c58d into http4s:series/0.23 Nov 8, 2022
@danicheg danicheg deleted the use-predefined-headers branch November 8, 2022 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants