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

fix 6074 (mod_alias failed to handle re_write config) #6892

Merged
merged 2 commits into from Jun 9, 2023

Conversation

MarkoMin
Copy link
Contributor

Fixed functionallity of re_write property (Issue #6074).
Previous code coudn't work because there was "^"++FakeName line,
but FakeName could be MatchPattern so this would fail.
I changed the behaviour to append "^" while storeing the re_write, alias or script_alias proplist.

Also, I included one simple re_write property into test, to make sure everything works correcty.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 19, 2023

CT Test Results

    2 files    21 suites   14m 0s ⏱️
336 tests 331 ✔️ 5 💤 0
561 runs  553 ✔️ 8 💤 0

Results for commit dcb3a32.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@u3s u3s added the team:PS Assigned to OTP team PS label Feb 20, 2023
@u3s
Copy link
Contributor

u3s commented Feb 22, 2023

Thanks for contribution and fix proposal.
Unfortunately, inets work has lower priority so it might take a while before we process it.

@RaimoNiskanen RaimoNiskanen added the stalled waiting for input by the Erlang/OTP team label Mar 1, 2023
@u3s u3s changed the title fix 6074 fix 6074 (mod_alias failed to handle re_write config) Mar 15, 2023
@u3s
Copy link
Contributor

u3s commented May 18, 2023

I've rebased the PR to current maint branch, so that we can test PR.

Fixed functionallity of re_write property.
Previous code coudn't work because there was "^"++FakeName line,
   but FakeName could be MatchPattern so this would fail.
I changed the behaviour to append "^" while storeing the re_write,
alias or script_alias proplist.

Also, I included one simple re_write property into test, to make sure
everything works correcty.
@u3s u3s added bug Issue is reported as a bug testing currently being tested, tag is used by OTP internal CI and removed stalled waiting for input by the Erlang/OTP team labels May 19, 2023
@u3s
Copy link
Contributor

u3s commented May 31, 2023

I think the PR looks good.

  • Please let me know if it would be possible to cover also script_re_write option in alias test case?
  • inets has low test coverage this would be a small step in wanted direction and inline with functionality touched in this PR

Such calls could be added in alias test case together with setting script_re_write option earlier:
GET /cgi-bin/erl/httpd_example:get
GET /cgi-UNWANTED-bin/erl/httpd_example:get

@MarkoMin
Copy link
Contributor Author

MarkoMin commented Jun 6, 2023

Is this what you meant? I'm not sure I understood "UNWANTED" part. Should it return 404?

@u3s
Copy link
Contributor

u3s commented Jun 7, 2023

Is this what you meant? I'm not sure I understood "UNWANTED" part. Should it return 404?

:-D. I just thought about something that is pattern matched successfully. So we have a URI towards script that is expected to pattern match and rewritten by httpd. Success HTTP code is expected as well.

"UNWANTED" was supposed to be something to exercise script_re_write

Sorry for confusion.

@MarkoMin
Copy link
Contributor Author

MarkoMin commented Jun 7, 2023

I misunderstood it... "/cgi-bin/erl/..." is actually for ESI scripts, while "script_re_write" is only for CGI scripts. I've amended changes to the latest commit, take a look :D

@u3s
Copy link
Contributor

u3s commented Jun 7, 2023

looks as I wished. I will merge it soon.
thank you!

@IngelaAndin IngelaAndin self-requested a review June 9, 2023 07:31
@u3s u3s removed the testing currently being tested, tag is used by OTP internal CI label Jun 9, 2023
@u3s u3s merged commit 9aa83c2 into erlang:maint Jun 9, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:PS Assigned to OTP team PS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants