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.

Sign up for GitHub

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

Jump to bottom

fix(router/atc): uri replace didn't works well if contains the optional group #13024

Merged
merged 26 commits into from
May 27, 2024
Merged

fix(router/atc): uri replace didn't works well if contains the optional group #13024

merged 26 commits into from
May 27, 2024

Conversation

git-hulk
Copy link
Contributor

@git-hulk git-hulk commented May 13, 2024 •
edited by StarlightIbuki

Summary

URI captures are not usable if the first capture is an empty string.

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Ticket reference

Fix #13014, KAG-4474

@github-actions github-actions bot added core/router cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels May 13, 2024
@team-eng-enablement team-eng-enablement added the author/community PRs from the open-source community (not Kong Inc) label May 13, 2024
@git-hulk git-hulk marked this pull request as draft May 13, 2024 14:01
@git-hulk
Copy link
Contributor Author

git-hulk commented May 13, 2024 •
edited

This PR is ready to review now. Could you please take a look while you have time? I'm not sure if I get your point correctly.

@git-hulk git-hulk force-pushed the fix/request-transformer-uri-replace branch 2 times, most recently from a78738a to 427967d Compare May 14, 2024 02:20
@git-hulk git-hulk marked this pull request as ready for review May 14, 2024 02:21
@git-hulk git-hulk force-pushed the fix/request-transformer-uri-replace branch from 427967d to 23ebd29 Compare May 14, 2024 02:22
StarlightIbuki
StarlightIbuki requested changes May 14, 2024
View reviewed changes
Copy link
Contributor

@StarlightIbuki StarlightIbuki left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! The PR is correctly done, except for some minor style issues.

kong/router/atc.lua Outdated Show resolved Hide resolved
spec/01-unit/08-router_spec.lua Show resolved Hide resolved
spec/03-plugins/36-request-transformer/02-access_spec.lua Outdated Show resolved Hide resolved
spec/03-plugins/36-request-transformer/02-access_spec.lua Outdated Show resolved Hide resolved
@git-hulk
Copy link
Contributor Author

@StarlightIbuki Thank you, I did enable the .editorconfig plugin in my IDE, but it seems didn't work correctly. Will take care of this next time.

chronolaw
chronolaw reviewed May 14, 2024
View reviewed changes
changelog/unreleased/kong/fix-request-transformer-uri-replace.yml Outdated Show resolved Hide resolved
kong/router/atc.lua Outdated Show resolved Hide resolved
kong/router/atc.lua Outdated Show resolved Hide resolved
@chronolaw chronolaw changed the title fix(request-transformer): uri replace didn't works well if contains the optional group fix(router/atc): uri replace didn't works well if contains the optional group May 14, 2024
chronolaw
chronolaw reviewed May 14, 2024
View reviewed changes
spec/01-unit/08-router_spec.lua Outdated Show resolved Hide resolved
spec/01-unit/08-router_spec.lua Outdated Show resolved Hide resolved
spec/03-plugins/36-request-transformer/02-access_spec.lua Outdated Show resolved Hide resolved
nowNick
nowNick approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@nowNick nowNick left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you for your contribution 🚀

spec/01-unit/08-router_spec.lua Outdated Show resolved Hide resolved
@git-hulk git-hulk requested a review from chronolaw May 14, 2024 08:32
StarlightIbuki
StarlightIbuki requested changes May 14, 2024
View reviewed changes
Copy link
Contributor

@StarlightIbuki StarlightIbuki left a comment

Choose a reason for hiding this comment

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

Please use isempty instead.

kong/router/atc.lua Outdated Show resolved Hide resolved
kong/router/atc.lua Outdated Show resolved Hide resolved
chronolaw
chronolaw reviewed May 14, 2024
View reviewed changes
kong/router/atc.lua Outdated Show resolved Hide resolved
spec/01-unit/08-router_spec.lua Show resolved Hide resolved
@git-hulk git-hulk requested a review from chronolaw May 14, 2024 09:45
@git-hulk
Copy link
Contributor Author

@StarlightIbuki @chronolaw Please take a look at the last commit again.

chronolaw
chronolaw reviewed May 14, 2024
View reviewed changes
kong/router/atc.lua Outdated Show resolved Hide resolved
kong/router/atc.lua Outdated Show resolved Hide resolved
spec/01-unit/08-router_spec.lua Outdated Show resolved Hide resolved
@StarlightIbuki StarlightIbuki force-pushed the fix/request-transformer-uri-replace branch from 0a5df57 to 96e3b95 Compare May 23, 2024 03:00
ADD-SP
ADD-SP reviewed May 23, 2024
View reviewed changes
kong/router/atc.lua Outdated Show resolved Hide resolved
@StarlightIbuki @git-hulk
Update kong/router/atc.lua
0db461e 
Co-authored-by: hulk <hulk.website@gmail.com>
chronolaw
chronolaw reviewed May 23, 2024
View reviewed changes
kong/router/atc.lua Outdated Show resolved Hide resolved
kong/router/atc.lua Show resolved Hide resolved
git-hulk
git-hulk commented May 23, 2024
View reviewed changes
kong/router/atc.lua Outdated Show resolved Hide resolved
@git-hulk git-hulk requested review from dndx and ADD-SP May 23, 2024 06:48
StarlightIbuki
StarlightIbuki reviewed May 23, 2024
View reviewed changes
kong/router/atc.lua Outdated Show resolved Hide resolved
@git-hulk
Copy link
Contributor Author

@dndx @nowNick @ADD-SP @samugi Would you mind taking a look again?

ADD-SP
ADD-SP approved these changes May 27, 2024
View reviewed changes
Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

@git-hulk Thank you for your contribution!

@ADD-SP ADD-SP merged commit 1d1056e into Kong:master May 27, 2024
25 checks passed
@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

@git-hulk
Copy link
Contributor Author

Thanks all for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StarlightIbuki StarlightIbuki StarlightIbuki approved these changes

@chronolaw chronolaw chronolaw approved these changes

@ADD-SP ADD-SP ADD-SP approved these changes

@samugi samugi Awaiting requested review from samugi

@nowNick nowNick Awaiting requested review from nowNick

@dndx dndx Awaiting requested review from dndx

Assignees
No one assigned
Labels
author/community PRs from the open-source community (not Kong Inc) cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/router size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optional capture groups are broken with the request-transformer plugin and traditional_compatible router
8 participants
@git-hulk @team-gateway-bot @dndx @chronolaw @nowNick @ADD-SP @StarlightIbuki @team-eng-enablement