Add pekko-http-cors module

[mdedetrich]

This PR adds pekko-http-cors support to pekko-http via the merge of akka-http-cors/pekko-http-cors (see lomigmegard/pekko-http-cors#33). Notable points

• Legal disclaimers have been added (both to the root LEGAL file and to the META-INF jar indicating that the code was granted to ASF via SGA)
• The code is exactly the same as https://github.com/lomigmegard/pekko-http-cors with the expected package/import changes with these exceptions
  • One breaking code change where org.apache.pekko.http.cors.javadsl.settings.CorsSettings.getMaxAge was changed from Optional[Long] to OptionalLong since this is consistent within the rest of the Akka/Pekko ecosystem (and is also idiomatic for Java use)
  • A non breaking change where explicit return types where added to the get* public Java API methods in CorsSettings
  • application.conf was renamed to reference.conf (using application.conf was likely an oversight, it should be used for actual applications where as reference.conf should be used for libraries). This was an accident on my part
  • The root typesafe config was changed from pekko-http-cors to pekko.http.cors to be consistent with the rest of the Pekko Http.
• Migration notes have been added including the previously listed changes
• Docs have been added to the standard place (i.e. docs/src/main/paradox/common/cors.md) which is largely inspired by https://github.com/lomigmegard/pekko-http-cors/blob/master/README.md however I have made some liberties in changes so its more consistent with Pekko documentation, specifically
  • The original docs had inline source code that was Scala, this was updated to use different groupings for Java/Scala which means that Java equivalents had to be added.
  • The mention of default values has been removed since this is already documented in reference.conf and has been added to docs/src/main/paradox/configuration.md.
  • The sample code is directly included using the Scala/java @@snip rather than just referencing it.
  • In general there is an argument that the various sub headings such as #### allowGenericHttpRequests / getAllowGenericHttpRequests should also be removed because they are duplicated in the scala docs and is also not typical with Pekko Http documentation (let me know what you think on this point).
  • This is the first case where have new code derived from somewhere outside of Akka via a SGA which means that our standard sbt-header-check needed to be updated in order to ignore the pekko-http-cors project. Note that only sources have been ignored, resources (such as reference.conf) still get checked mainly for the # SPDX-License-Identifier: Apache-2.0 header. This is on the presumption that headers are not recommended/necessary for non derived/SGA approved Apache source code.

@lomigmegard Pinging you so you can also review the PR and let me know your thoughts. The most critical thing here is any changes that you think would need to be done now otherwise they would break users in the future (i.e. the Optional[Long] to OptionalLong is an example of this) which you weren't able to do in akka-http-cors due to breaking downstream users.

[pjfanning]

We don't need this if the code was granted to us

[pjfanning]

if we include something, it should be in the NOTICE file but I don't think we need to include anything

[He-Pin]

Better we have a durable record somewhere?

[pjfanning]

Since the code is granted to us, I don't see how this differs much from a standard PR that we get. And with PRs generally, we do not modify the LICENSE and we insist that all source files are given Apache license headers.

The LICENSE addenda are to point out 3rd party code that has different licenses or that has copyright declarations in those licenses (if the original code was ASL but had a specific copyright in it).

I would favour highlighting the source of the code but I would suggest that we do that in the source files adding something like:

// based on code from pekko-http-cors <https://github.com/lomigmegard/pekko-http-cors> which has been granted
// to the Apache Pekko project - the Software Grant agreement is on record with the Secretary of the ASF

Edited to use pekko-http-cors based on @lomigmegard 's agreement not to delete that repo (just to archive it)

[lomigmegard]

We could link to https://github.com/lomigmegard/pekko-http-cors but there is a risk that @lomigmegard could delete that repo.

I was thinking of just archiving it, thus any link will still work.

[pjfanning]

I don't think we need this - StandardLicense should be ok for this module

[pjfanning]

the license changes need to be thought through

[mdedetrich]

@justinmclean Is it possible to comment on #208 (review), i.e. do we need to mention in LICENSE code that has been granted via a SGA?

[mdedetrich]

Compile failures are due to pekko RC2 being dropped, when @pjfanning creates pekko RC3 this will be resolved.

One thing that I noticed is that we need to do modifications to sbt-header, this is because our standard header i.e.

/*
 * Licensed to the Apache Software Foundation (ASF) under one or more
 * license agreements; and to You under the Apache License, version 2.0:
 *
 *   https://www.apache.org/licenses/LICENSE-2.0
 *
 * This file is part of the Apache Pekko project, which was derived from Akka.
 */

Is not correct for pekko-http-cors since it was not derived from Akka (emphasis on This file is part of the Apache Pekko project, which was derived from Akka.). I will look at this in the afternoon.

[pjfanning]

RC3 jars area ready - see #209

[mdedetrich]

I have fixed the issue of sbt-header applying on files that it shouldn't, see the last point in #208 (comment) for more details.

[lomigmegard]

Thanks for the PR @mdedetrich.

I reviewed it and found only a couple places where some utilities from pekko could be re-used, but it can be done in another commits (or even PRs).

Regarding your question for breaking changes, the main point I see are the default values for the configuration. Especially allowed-origins and allowed-headers which are very permissive. It's a tradeoff between safer defaults, and defaults that "just work" for simple servers.

[lomigmegard]

application.conf was renamed to reference.conf (using application.conf was likely an oversight, it should be used for actual applications where as reference.conf should be used for libraries).

It was already reference.conf :)

[pjfanning]

all source files must have license headers - please use the standard Apache one

[mdedetrich]

I swear I saw it somewhere that headers for original/SGA code was a convention but I might be misremembering. I will fix this after I resolve @lomigmegard review comments.

[pjfanning]

the code is granted to us - we can add headers and we need an Apache header

if the original code had headers, we' d have to keep them or get @lomigmegard 's permission to change them

[mdedetrich]

So just to confirm, since the original code has no headers I need to add the standard Apache one?

[pjfanning]

could you use the full header from https://www.apache.org/legal/src-headers.html ?

[mdedetrich]

I just updated the PR before your comment, I ended up using

/*
 * Licensed to the Apache Software Foundation (ASF) under one or more
 * license agreements; and to You under the Apache License, version 2.0:
 *
 *   https://www.apache.org/licenses/LICENSE-2.0
 */

which is the same as the standard header but without the "derived from Akka" part

I will change it to full license header now.

[mdedetrich]

@lomigmegard

Regarding your question for breaking changes, the main point I see are the default values for the configuration. Especially allowed-origins and allowed-headers which are very permissive. It's a tradeoff between safer defaults, and defaults that "just work" for simple servers.

Thanks, when I was talking about breaking changes I was alluding to binary compatibility and not behavioural. However the point about cors permissions being overly permissive is one that should be noted.

It was already reference.conf :)

Hmm, I must have gave it the wrong name when copying, my bad!

[mdedetrich]

@lomigmegard I addressed all of your comments and sent a re-review but when going through the code I noticed some things.

[mdedetrich]

@jrudolph

Has it been considered whether we should keep this as a separate module or merge into the http modules proper?

I did briefly think about it and I came to similar pros/cons as you did but I opted to put it as a separate module for the following reasons

• While its definitely true that cors is commonly used, I am not sure if its common enough to warrant inclusion into core mainly based on the fact that it wasn't included into core in the first place and it seems like it wasn't too much of a problem?
  • I was also copying http-caching here, which you could also claim is a fairly frequent problem in HTTP however is also its own separate module
• Its less friction/work rather than putting it into core

That being said if you feel strongly that it should go into core then its something that can be explored

@pjfanning I have fixed all of the legal/license/header stuff you mentioned before (turns out I entirely forgot that sbt-header will accept source files with standard apache header). Can you re-review it?

[pjfanning]

@pjfanning I have fixed all of the legal/license/header stuff you mentioned before (turns out I entirely forgot that sbt-header will accept source files with standard apache header). Can you re-review it?

I think we should use the full header from https://www.apache.org/legal/src-headers.html

[mdedetrich]

I think we should use the full header from https://www.apache.org/legal/src-headers.html

I noticed your comment after I pushed PR, fixing it now

[mdedetrich]

@pjfanning Just pushed with the full Apache license header

[pjfanning]

lgtm

[pjfanning]

@mdedetrich before we release, we probably need to raise an issue at https://issues.apache.org/jira/projects/LEGAL/issues to discuss how to acknowledge this code. Pertinent points are:

• We have a Software Grant Agreement on hand with the Secretary of the ASF
• https://github.com/lomigmegard/pekko-http-cors has Apache License, v2.0 with no custom copyright declaration
• pekko-http-cors has no NOTICE file
• pekko-http-cors source code has no license headers

My suggestion would be a short mention in our NOTICE file. Something a bit like the BEA mention in https://github.com/apache/poi/blob/trunk/legal/NOTICE

The first answer on https://issues.apache.org/jira/browse/LEGAL-150 - I think this gives us the basis to apply the ASL headers that we have already added.

[mdedetrich]

My suggestion would be a short mention in our NOTICE file. Something a bit like the BEA mention in https://github.com/apache/poi/blob/trunk/legal/NOTICE

@pjfanning I just added a mention in the NOTICE file, if the text needs to be updated let me know.

[pjfanning]

@mdedetrich I think that NOTICE statement looks good

[mdedetrich]

Perfect I will leave this PR open till early next week incase @jrudolph @lomigmegard have any further additional comments, then I will merge it.

[lomigmegard]

LGTM, just a few minor comments.

(I usually use the https://conventionalcomments.org/ notation for my comments)

[mdedetrich]

Another thing I noticed is that we happen to have public final case class, this should be converted to a normal final class (with manually implemented toString/equals,apply etc etc) because adding fields to a case class breaks binary compatibility and at least from some quick searching pekko-http doesn't any public final case class.

@jrudolph @lomigmegard wdyt?

[pjfanning]

Another thing I noticed is that we happen to have public final case class, this should be converted to a normal final class (with manually implemented toString/equals,apply etc etc) because adding fields to a case class breaks binary compatibility and at least from some quick searching pekko-http doesn't any public final case class.

@jrudolph @lomigmegard wdyt?

I wouldn't worry about binary compatibility yet - we have no releases to be binary compatible with. I would favour using case classes over classes with custom toString/hashCode/equals/etc.

[mdedetrich]

I wouldn't worry about binary compatibility yet - we have no releases to be binary compatible with. I would favour using case classes over classes with custom toString/hashCode/equals/etc.

Its not about what people are currently using, its about any potential future changes. If you have a case class like this

final case class MyCaseClass(string: String)

And you add a field some point in the future, i.e.

final case class MyCaseClass(string: String, int: Int)

This will break binary compatibility for MyCaseClass because of unapply/copy (which case class automatically implements). This is why for both pekko and pekko-http no public classes happen to be a case class. The same is of other Scala ecosystems/projects (i.e. cats) which actually follow bincompat properly. If we want to follow semantic versioning, unless we can always guarantee that we will never add a new field it should be converted to a class.

This is a known problem and there is a massive thread on Scala contributors about it https://contributors.scala-lang.org/t/pre-sip-structural-data-structures-that-can-evolve-in-a-binary-compatible-way/5684

[lomigmegard]

I agree about to be careful on the binary compatibility. However in the case of this library, all the occurrences of case class are wrappers around a single value, which semantically would make no sense to extend to more fields. For example the rejection, as all other rejections in pekko-http are a case class:

final case class CorsRejection(cause: CorsRejection.Cause)

CorsSettingsImpl is an exception but it's private and we could/should make use of the workaround of the private constructor as shown by M. Odersky here. We could also use this trick in other places, limiting the boilerplate code (I don't think it's worth implementing equals/hashCode by hand).

[mdedetrich]

I agree about to be careful on the binary compatibility. However in the case of this library, all the occurrences of case class are wrappers around a single value, which semantically would make no sense to extend to more fields. For example the rejection, as all other rejections in pekko-http are a case class:

final case class CorsRejection(cause: CorsRejection.Cause)

Ah yes, I completely forgot that in this case pekko/akka-http uses case classes more like a new type in order for its routing dsl rather than a data structure

CorsSettingsImpl is an exception but it's private and we could/should make use of the workaround of the private constructor as shown by M. Odersky here.

Since its private I don't think its necessary in this case unless I am missing something

We could also use this trick in other places, limiting the boilerplate code (I don't think it's worth implementing equals/hashCode by hand).

Yes, I completely forgot about this technique and there are definitely places where this needs to be done, have a look at https://github.com/apache/incubator-pekko-connectors/blob/main/s3/src/main/scala/org/apache/pekko/stream/connectors/s3/model.scala !

[mdedetrich]

@lomigmegard FYI on this topic of case class binary compatibility, the technique you mentioned (i.e. scala/improvement-proposals#50 (comment)) only works with Scala 3. With Scala 2 even if you have a private constructor in the case class the generated copy method is still public (which is one of the problematic methods that breaks bincompat when more fields are added to the case class). It seems that this was changed as an improvement in Scala 3, see apache/pekko-connectors#190 (comment) for more info.

[mdedetrich]

Im going to go ahead and merge this now. Many thanks @lomigmegard for your help on the review!