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

Make filtering allowed headers case-insensitive #10

Merged
merged 1 commit into from
Feb 19, 2020

Conversation

dragarthPl
Copy link
Contributor

@dragarthPl dragarthPl commented Feb 11, 2020

Description

For a reason we can use:

AllowedHeadersFilter.WildcardCaseInsensitive.create(strings);

I done refactor usage of AllowedHeadersFilter

Motivation and Context

#7 and PR-8

Screenshots (if appropriate)

Upgrade notes (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

I hereby agree to the terms of the Knot.x Contributor License Agreement.

Copy link
Member

@malaskowski malaskowski left a comment

Choose a reason for hiding this comment

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

May I suggest renaming the PR:

Make filtering allowed headers case-insensitive
?
And please update the changelog with this info (and squash commits as always please).

Copy link
Member

@malaskowski malaskowski left a comment

Choose a reason for hiding this comment

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

@dragarth-7
I'm reviewing https://github.com/Knotx/knotx-commons/pull/8/files and now I wonder if there could be more tests for headers with wildcards here:
https://github.com/Knotx/knotx-commons/blob/master/src/test/java/io/knotx/commons/http/request/AllowedHeadersFilterTest.java#L33
?
E.g. Host-* or Content-*

@dragarthPl dragarthPl changed the title Unit Test HTTP request commons Make filtering allowed headers case-insensitive Feb 12, 2020
@dragarthPl
Copy link
Contributor Author

May I suggest renaming the PR:

Make filtering allowed headers case-insensitive
?
And please update the changelog with this info (and squash commits as always please).

Done

@dragarth-7
I'm reviewing https://github.com/Knotx/knotx-commons/pull/8/files and now I wonder if there could be more tests for headers with wildcards here:
https://github.com/Knotx/knotx-commons/blob/master/src/test/java/io/knotx/commons/http/request/AllowedHeadersFilterTest.java#L33
?
E.g. Host-* or Content-*

I can extend test by more cases. Are you want to discuss about that somewhere ? How will we process it ? By new tickets/topic ?

@malaskowski
Copy link
Member

Depends what we decide on: Knotx/knotx-fragments#100 (comment)

Good start could be creating an issue in the knotx-commons to create more unit tests, or you may directly create a PR with more cases (that's also fine). We can discuss there if any more cases should be covered.

Copy link
Member

@malaskowski malaskowski 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 @dragarth-7

@malaskowski malaskowski merged commit 9c0d048 into Knotx:master Feb 19, 2020
@dragarthPl
Copy link
Contributor Author

@Skejven You need also merge changes from: Knotx/knotx-commons#14 if you want this changes working.

But i also must remark that in PR-14 from knotx-common we have discussion about that how wildcards/regex mechanizm should work.

See also other related changes that I prepared as PR:
Knotx/knotx-server-http#46
Knotx/knotx-fragments#100
and mentioned above:
Knotx/knotx-commons#14

@malaskowski
Copy link
Member

malaskowski commented Feb 19, 2020

Right, thanks my fingers were too quick with merging :)
I will revert and try to re-open this.
I was trusting too much in CI and just discovered this effect: Knotx/knotx-aggregator#25

@malaskowski
Copy link
Member

@dragarth-7 may I ask you to open a new Pull Request (I can't reopen unfortunately). Sorry for the trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants