add setting ignore-illegal-header-for #687 #1942

shnmorimoto · 2018-03-16T08:07:24Z

akka-ci · 2018-03-16T08:10:04Z

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

ktoso · 2018-03-16T08:30:30Z

OK TO TEST

ktoso · 2018-03-16T08:30:54Z

Another PR from the hackathon, please review @raboof @johanandren ? :)

akka-ci · 2018-03-16T08:44:05Z

Test FAILed.

raboof · 2018-03-16T09:11:40Z

Thanks for your contribution @shnmorimoto, this is indeed something people run into a lot, and it would be great if we could give them more fine-grained tools to suppress such warnings.

You might have noticed the builds failed, this is because of our automated binary incompatibility checks:

[error]  * abstract method getIgnoreIllegalHeaderFor()scala.collection.immutable.Set in class akka.http.javadsl.settings.ParserSettings is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.http.javadsl.settings.ParserSettings.getIgnoreIllegalHeaderFor")
[error]  * method copy(java.lang.String,java.lang.String)akka.http.scaladsl.model.ErrorInfo in class akka.http.scaladsl.model.ErrorInfo does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("akka.http.scaladsl.model.ErrorInfo.copy")
[error]  * method this(java.lang.String,java.lang.String)Unit in class akka.http.scaladsl.model.ErrorInfo does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("akka.http.scaladsl.model.ErrorInfo.this")
[error]  * method apply(java.lang.String,java.lang.String)akka.http.scaladsl.model.ErrorInfo in object akka.http.scaladsl.model.ErrorInfo does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("akka.http.scaladsl.model.ErrorInfo.apply")
[error]  * abstract method ignoreIllegalHeaderFor()scala.collection.immutable.Set in class akka.http.scaladsl.settings.ParserSettings is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.http.scaladsl.settings.ParserSettings.ignoreIllegalHeaderFor")

The warnings on ParserSettings can be ignored because ParserSettings is marked @DoNotInherit (we have an issue open to automate this logic). You can add them to the list of ignored warnings as described in CONTRIBUTING.md.

The warnings on ErrorInfo are a bit more tricky: these pop up because ErrorInfo is a case class, and the automatically-generated methods on this case class changed because the fields changed. I think it would be fine to turn ErrorInfo into a regular class and hand-implemented the missing API's (perhaps private?) You can see something similar has been done to RouterSettings in ababace.

Would you be up for fixing those?

shnmorimoto · 2018-03-16T10:38:21Z

@raboof Thank you for your review.

I understood about the reason for the builds failed.

I will check the reference code and I will fix it.

akka-ci · 2018-03-16T14:19:18Z

Test PASSed.

shnmorimoto · 2018-03-16T14:19:20Z

@raboof cc: @ktoso I fix binary incompatibility.

raboof

Starting to look good, added a couple of additional comments!

raboof · 2018-03-16T14:22:54Z

akka-http-core/src/main/mima-filters/10.0.11.backwards.excludes

@@ -86,3 +86,5 @@ ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.http.javadsl.TimeoutA
 ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.http.scaladsl.TimeoutAccess.timeout")
 ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.http.scaladsl.TimeoutAccess.getTimeout")

+ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.http.javadsl.settings.ParserSettings.getIgnoreIllegalHeaderFor")
+ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.http.scaladsl.settings.ParserSettings.ignoreIllegalHeaderFor")


Shouldn't we introduce a 10.1.0.backwards.excludes?

move configuration to 10.1.0.backwards.excludes.

raboof · 2018-03-16T14:23:03Z

akka-http-core/src/main/resources/reference.conf

@@ -386,6 +386,9 @@ akka.http {
    # `error-logging-verbosity`.
    illegal-header-warnings = on

+    # TODO comment


Describe the setting here

add comment.

raboof · 2018-03-16T14:25:01Z

akka-http-core/src/main/scala/akka/http/scaladsl/model/ErrorInfo.scala

  def withSummary(newSummary: String) = copy(summary = newSummary)
  def withSummaryPrepended(prefix: String) = withSummary(if (summary.isEmpty) prefix else prefix + ": " + summary)
+  def withHeaderNamePrepend(headerName: String) = setErrorHeaderName(errorHeaderName = headerName)


This doesn't actually prepend the header name, right?

Surely you are right.　rename method.

raboof · 2018-03-16T14:26:09Z

akka-http-core/src/main/scala/akka/http/scaladsl/model/ErrorInfo.scala

+    new ErrorInfo(summary, detail, errorHeaderName)
+  }
+
+  private[akka] def setErrorHeaderName(errorHeaderName: String): ErrorInfo = new ErrorInfo(summary, detail, errorHeaderName)


Perhaps make this public and call it withErrorHeaderName?

Surely you are right. delete this method and rename withHeaderNamePrepend method to withErrorHeaderName.

raboof · 2018-03-16T14:27:17Z

akka-http-core/src/main/scala/akka/http/scaladsl/model/ErrorInfo.scala

@@ -26,6 +54,10 @@ object ErrorInfo {
   * Used for example when catching exceptions generated by the header value parser, which doesn't provide
   * summary/details information but structures its exception messages accordingly.
   */
+  private[akka] def apply(summary: String = "", detail: String = ""): ErrorInfo = new ErrorInfo(summary, detail, "")


You introduced these new methods between the existing scaladoc documentation and the method they document below - can you reorder that?

I made a mistake. reorder methods. thanks.

shnmorimoto · 2018-03-17T12:19:04Z

I'd like to consult one point.
Should we check the header name in settings sensitive? Or insensitive?

When set 'user-agent' in settings, ignore warn.
But set 'User-agent' in settings, logged warn now.

akka-ci · 2018-03-17T12:19:35Z

Test PASSed.

ktoso · 2018-03-17T12:22:31Z

As per the HTTP spec header names should be treated case insensitive :) good question

shnmorimoto · 2018-03-17T12:24:51Z

Thanks! I will fix it.

akka-ci · 2018-03-17T12:59:09Z

Test PASSed.

akka-ci · 2018-04-25T04:24:52Z

Test PASSed.

shnmorimoto · 2018-04-25T04:26:40Z

@ktoso I will be glad if you review at your convenience.

raboof

Looks good to me, thanks for contributing this feature!

ktoso · 2018-05-07T01:15:18Z

Looking great now, thanks! Updated branch and can merge after validation

akka-ci · 2018-05-07T01:24:42Z

Test PASSed.

ktoso · 2018-05-07T01:35:52Z

Thanks a lot and sorry this took such a long time to merge! :-)

shnmorimoto · 2018-05-07T04:26:28Z

@ktoso @raboof @johanandren
Thank you for merge.
And thank you for advising me on the review.

ktoso self-assigned this Mar 16, 2018

akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR that is currently being validated by Jenkins labels Mar 16, 2018

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Mar 16, 2018

raboof reviewed Mar 16, 2018

View reviewed changes

akka-ci added validating PR that is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Mar 17, 2018

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels Mar 17, 2018

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Mar 17, 2018

Shinichi Morimoto and others added 10 commits April 25, 2018 13:11

check ignore header name insensitive #687

9ec538b

Update 10.1.0.backwards.excludes

bc2e09e

put case class back for ErrorInfo. and add annotation @internalapi #687

8b14a92

fix test #687

1e5a325

delete InternalApi annnotation from ErrorInfo #687

f577046

create 10.1.2.backwards.excludes #687

a9585d4

Chainge ErrorInfo from caseclass to class for bin incomp #687

48954f0

Update reference.conf

8c04d0c

add tests #687

0d01b92

use parseLine #687

8e82d9b

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Apr 25, 2018

raboof approved these changes Apr 30, 2018

View reviewed changes

Update reference.conf

1579427

ktoso approved these changes May 7, 2018

View reviewed changes

Merge branch 'master' into feat_add_setting_ignore_illegal_header_for

edf0180

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels May 7, 2018

ktoso merged commit 78e5b1f into akka:master May 7, 2018

marhel mentioned this pull request Jul 2, 2018

[C# et al] Default user-agent violates RFC swagger-api/swagger-codegen#8376

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add setting ignore-illegal-header-for #687 #1942

add setting ignore-illegal-header-for #687 #1942

shnmorimoto commented Mar 16, 2018

akka-ci commented Mar 16, 2018

ktoso commented Mar 16, 2018

ktoso commented Mar 16, 2018

akka-ci commented Mar 16, 2018

raboof commented Mar 16, 2018

shnmorimoto commented Mar 16, 2018

akka-ci commented Mar 16, 2018

shnmorimoto commented Mar 16, 2018

raboof left a comment

raboof Mar 16, 2018

shnmorimoto Mar 17, 2018

raboof Mar 16, 2018

shnmorimoto Mar 17, 2018

raboof Mar 16, 2018

shnmorimoto Mar 17, 2018

raboof Mar 16, 2018

shnmorimoto Mar 17, 2018

raboof Mar 16, 2018

shnmorimoto Mar 17, 2018

shnmorimoto commented Mar 17, 2018 •

edited

Loading

akka-ci commented Mar 17, 2018

ktoso commented Mar 17, 2018

shnmorimoto commented Mar 17, 2018

akka-ci commented Mar 17, 2018

akka-ci commented Apr 25, 2018

shnmorimoto commented Apr 25, 2018

raboof left a comment

ktoso commented May 7, 2018

akka-ci commented May 7, 2018

ktoso commented May 7, 2018

shnmorimoto commented May 7, 2018

add setting ignore-illegal-header-for #687 #1942

add setting ignore-illegal-header-for #687 #1942

Conversation

shnmorimoto commented Mar 16, 2018

akka-ci commented Mar 16, 2018

ktoso commented Mar 16, 2018

ktoso commented Mar 16, 2018

akka-ci commented Mar 16, 2018

raboof commented Mar 16, 2018

shnmorimoto commented Mar 16, 2018

akka-ci commented Mar 16, 2018

shnmorimoto commented Mar 16, 2018

raboof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shnmorimoto commented Mar 17, 2018 • edited Loading

akka-ci commented Mar 17, 2018

ktoso commented Mar 17, 2018

shnmorimoto commented Mar 17, 2018

akka-ci commented Mar 17, 2018

akka-ci commented Apr 25, 2018

shnmorimoto commented Apr 25, 2018

raboof left a comment

Choose a reason for hiding this comment

ktoso commented May 7, 2018

akka-ci commented May 7, 2018

ktoso commented May 7, 2018

shnmorimoto commented May 7, 2018

shnmorimoto commented Mar 17, 2018 •

edited

Loading