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

Handle nested parens #22

Closed
wants to merge 1 commit into from
Closed

Handle nested parens #22

wants to merge 1 commit into from

Conversation

iko
Copy link

@iko iko commented Aug 19, 2015

Some urls (for example Graphite) have some nested parens in the parameters. An url with nested parens would miss the last closing paren and the linkified result would look something like this:

http://example.com/render?target=summarize(deriviative(app.numUsers),%221min%22)&title=New_Users_Per_Minute

I do realize that this fix will allow unbalanced parens. Is that a problem?

@puzrin
Copy link
Member

puzrin commented Aug 20, 2015

Example is acceptable, but i don't like implementation. It will ignore all intermetiate conditions between 2 scoped groups:

(foo)ignored---content(bar)

@iko
Copy link
Author

iko commented Aug 21, 2015

Aha! Didn't catch that. I'll work on another way of doing this. Thanks.

@puzrin
Copy link
Member

puzrin commented Aug 21, 2015

@iko, honestly, i'm not sure this task can be solved without side effects like reDoS.

How critical is it to detect such links properly?

@iko
Copy link
Author

iko commented Aug 24, 2015

How about limiting the nesting? Twitter-text for example has a hard limit of one level. I haven't had the time to look into this yet, though.

Detecting urls with nested parens is not the most critical feature at the moment, but eventually we'll probably need it to work.

@puzrin
Copy link
Member

puzrin commented Aug 24, 2015

Idea looks nice.

@batmat
Copy link

batmat commented Sep 8, 2015

Hi guys, another example of complex/long url that the parser is unable to grok:

 http://sv-t-vnl-forge-metrics:5601/?#/dashboard/Logs-Marmotte?_a=(filters:!(),panels:!((col:1,columns:!(message,host,tags),id:Marmotte-logs,row:1,size_x:12,size_y:8,sort:!('@timestamp',asc),type:search)),query:(query_string:(analyze_wildcard:!t,query:'*')),title:'Logs%20Marmotte')&_g=(refreshInterval:(display:'10%20seconds',pause:!f,section:1,value:10000),time:(from:now-15m,mode:quick,to:now))

FYI, this URL is one generated by the "share" link of Kibana 4.

@puzrin
Copy link
Member

puzrin commented Sep 8, 2015

I summarized all in #23 for better tracking. May be worth to close PR if no updates expected soon.

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.

3 participants