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 Web.pm find the correct share dir for IAs that are substring of oth... #237

Merged
merged 1 commit into from Mar 12, 2015

Conversation

Projects
None yet
3 participants
@pfirsichbluete
Contributor

pfirsichbluete commented Mar 11, 2015

In duckduckgo/zeroclickinfo-spice the Time spice's name is a substring
of the Timer spice's name. When looking for resources for the Timer
IA, paths of the form

.../timer/...

got mangled to

.../time/r/...

because the matcher for IAs share directories did not check for the
trailing path separator. Hence, the share path of the Time (no trailing
“r” in Time) matched for Timer (trailing “r” in Timer) resources, and
path handling got confused.

By making the guard for identifying the relevant the share directory
also check on the trailing '/', the share directories of Time and
Timer are properly matched and the Timer IA again properly loads for
'timer 10 seconds'.

Fixes duckduckgo/zeroclickinfo-spice#1625

Make Web.pm find the correct share dir for IAs that are substring of …
…others

In duckduckgo/zeroclickinfo-spice the Time spice's name is a substring
of the Timer spice's name. When looking for resources for the Timer
IA, paths of the form

  .../timer/...

got mangled to

  .../time/r/...

because the matcher for IAs share directories did not check for the
trailing path separator. Hence, the share path of the Time (no trailing
“r” in Time) matched for Timer (trailing “r” in Timer) resources, and
path handling got confused.

By making the guard for identifying the relevant the share directory
also check on the trailing '/', the share directories of Time and
Timer are properly matched and the Timer IA again properly loads for
'timer 10 seconds'.

Fixes duckduckgo/zeroclickinfo-spice#1625
@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Mar 11, 2015

Member

@pfirsichbluete good catch! And thanks for taking the time to fix this.

I believe the same bug exists on line 134 as well? https://github.com/duckduckgo/p5-app-duckpan/blob/master/lib/App/DuckPAN/Web.pm#L134

Member

moollaza commented Mar 11, 2015

@pfirsichbluete good catch! And thanks for taking the time to fix this.

I believe the same bug exists on line 134 as well? https://github.com/duckduckgo/p5-app-duckpan/blob/master/lib/App/DuckPAN/Web.pm#L134

@pfirsichbluete

This comment has been minimized.

Show comment
Hide comment
@pfirsichbluete

pfirsichbluete Mar 11, 2015

Contributor

I believe the same bug exists on line 134 as well?

I could not reproduce on my end.

For me, the keys of $self->_path_hash (the relevant hash of line 134) already end in a /. Like

  • /js/spice/mtg/
  • /js/spice/astrobin/fetch_id/
  • [...]

(while the keys of $self->_share_dir_hash did not).

Hence, the $_ of

if ($request->request_uri =~ m/^$_/g) {

already contains the needed trailing / and is non susceptive to the bug.

Can you trigger the issue for line 134?

Contributor

pfirsichbluete commented Mar 11, 2015

I believe the same bug exists on line 134 as well?

I could not reproduce on my end.

For me, the keys of $self->_path_hash (the relevant hash of line 134) already end in a /. Like

  • /js/spice/mtg/
  • /js/spice/astrobin/fetch_id/
  • [...]

(while the keys of $self->_share_dir_hash did not).

Hence, the $_ of

if ($request->request_uri =~ m/^$_/g) {

already contains the needed trailing / and is non susceptive to the bug.

Can you trigger the issue for line 134?

@killerfish

This comment has been minimized.

Show comment
Hide comment
@killerfish

killerfish Mar 11, 2015

Contributor

great work @pfirsichbluete I was just about to submit a pr for this 😄

the keys of $self->_path_hash (the relevant hash of line 134) already end in a /

Yup, we shouldnt have an issue with that hash.

Contributor

killerfish commented Mar 11, 2015

great work @pfirsichbluete I was just about to submit a pr for this 😄

the keys of $self->_path_hash (the relevant hash of line 134) already end in a /

Yup, we shouldnt have an issue with that hash.

@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Mar 12, 2015

Member

@pfirsichbluete yes, my mistake you're correct. Tested and LGTM 👍

Member

moollaza commented Mar 12, 2015

@pfirsichbluete yes, my mistake you're correct. Tested and LGTM 👍

moollaza added a commit that referenced this pull request Mar 12, 2015

Merge pull request #237 from pfirsichbluete/web-share-disambiguator
Make Web.pm find the correct share dir for IAs that are substring of oth...

@moollaza moollaza merged commit 7f2da61 into duckduckgo:master Mar 12, 2015

@pfirsichbluete pfirsichbluete deleted the pfirsichbluete:web-share-disambiguator branch Mar 17, 2015

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