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(router/atc): uri replace didn't works well if contains the optional group #13024

Merged
merged 26 commits into from
May 27, 2024

Conversation

git-hulk
Copy link
Contributor

@git-hulk git-hulk commented May 13, 2024

Summary

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

Checklist

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

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
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.

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
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
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
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
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.

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
kong/router/atc.lua Outdated Show resolved Hide resolved
kong/router/atc.lua Outdated Show resolved Hide resolved
kong/router/atc.lua Show resolved Hide resolved
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
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?

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
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
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