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

Add access to regexp capturing group Inside URL rewriter #1973

Merged
merged 3 commits into from
Nov 14, 2018

Conversation

dskoval
Copy link
Contributor

@dskoval dskoval commented Nov 8, 2018

  • add additional level for $tyc_context trigger keys
  • $tyk_context.trigger-{n}-{name}-{i}-{groupNum} holds group value

Fixes #1833

- add additional level  for $tyc_context trigger keys
- $tyk_context.trigger-{n}-{name}-{i}-{groupNum}  holds group value

Fixes TykTechnologies#1833
@dskoval
Copy link
Contributor Author

dskoval commented Nov 8, 2018

@buger
will also try to remove some code duplication in rewrite middleware

@buger
Copy link
Member

buger commented Nov 8, 2018

Nice one!

@dencoded can you pls review?

@buger buger requested a review from dencoded November 8, 2018 16:50
@buger
Copy link
Member

buger commented Nov 9, 2018

Thank you a lot for all changes!

@dencoded does it looks good to you?

@dskoval
Copy link
Contributor Author

dskoval commented Nov 9, 2018

Hi, at this very moment i'm removing repeating/unused code in cached regexp implementation.
At bare minimum i still have to compare benchmarks between mine and original implementation.
If it will look promising, should i add it to this PR, create issue for it or just create new PR?
Asking because i'm new to GitHub and doesn't want to create mess

@buger
Copy link
Member

buger commented Nov 9, 2018

Better new PR, to separate feature thing from refactoring itself. Thank you!

mw_url_rewrite.go Outdated Show resolved Hide resolved
return strings.Join(parts, triggerKeySep)
}

func addGroupsToContextData(cd *map[string]interface{}, keyPrefix string, groups []string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need to pass map as pointer as maps (and slices) are passed by reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, for me this one looks suspicious.
Intent of addGroupsToContextData is to modify (add items to) passed map.
As far as i know, in golang everything is value and passed by value (copied). Slices can look like passed by reference, because internally they holds pointer to baking array, first item index, length and capacity.
It can feels like they passed by reference (in case of modifying existing values), but in case of adding items they not. What's why append() returns updated slice.
https://play.golang.org/p/arTxwX1w9mf

I've checked same approach with map and it works, probably because in essence map is just a pointer to underlying structure - i don't know.

Will remove pointers to map as requested

Copy link
Contributor

Choose a reason for hiding this comment

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

this is tricky one - slices are different because they have len and cap in addition to pointer to underlaying array with elements, so when we pass it to function it receives new "descriptor" of slice but original pointer to elements.

if we do append inside function we have to return slice and re-assign result on calling side, otherwise calling part will hold slice-descriptor with appended elements but old len and cap (thats why Println outputs only one element). you can find more info about that situation here https://golang.org/doc/effective_go.html#slices

Changing elements would work if your slice is pre-allocated, i.e.:
https://play.golang.org/p/ZoK7WLz_Xtw

That's actually how all functions like func Read(slice []interface{}) (int, error) reading data into passed slice without any explicit pointers.

But my comment was about maps, they work differently.

@dencoded
Copy link
Contributor

@buger @dskoval left some comments which I believe need to be addressed

- extract regexp match  handling into function
@dskoval
Copy link
Contributor Author

dskoval commented Nov 12, 2018

@buger @dencoded implemented requested changes in last commit
68e92f4

@buger buger merged commit a2aade0 into TykTechnologies:master Nov 14, 2018
buger pushed a commit that referenced this pull request Nov 14, 2018
- add additional level  for $tyc_context trigger keys
- $tyk_context.trigger-{n}-{name}-{i}-{groupNum}  holds group value

Fixes #1833
buger pushed a commit that referenced this pull request Nov 14, 2018
- add additional level  for $tyc_context trigger keys
- $tyk_context.trigger-{n}-{name}-{i}-{groupNum}  holds group value

Fixes #1833
buger pushed a commit that referenced this pull request Jan 15, 2019
- add additional level  for $tyc_context trigger keys
- $tyk_context.trigger-{n}-{name}-{i}-{groupNum}  holds group value

Fixes #1833

(cherry picked from commit a2aade0)
asoorm pushed a commit to asoorm/tyk that referenced this pull request Feb 27, 2019
…gies#1973)

- add additional level  for $tyc_context trigger keys
- $tyk_context.trigger-{n}-{name}-{i}-{groupNum}  holds group value

Fixes TykTechnologies#1833
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.

None yet

3 participants