Docserver: Fix 3 issues relating to finding API references in sample files,

Docserver: Fix 3 issues relating to finding API references in sample files,
contributing to too-long database keys. Most importantly, match whole words
when doing variable name replacement, not just character sequences. Minified
files with variable names like "a" were exploding. Secondly, don't cache the
title of links in reference resolver, it's pointless and leading to even
longer database keys. Thirdly, truncate database keys to avoid crashing the
server from this ever again; log error instead.

BUG=314102
R=jyasskin@chromium.org
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232856

[not at google - send to devlin, 2013-11-01 17:12:04]

Before you ask for a test - the samples are woefully undertested. They're awful.
Definitely a candidate for deleting all the tests and starting again. I had a
look at adding a test but it'd take more time than I have (need to get this
patch in so that I can start running the server again).

However, I have checked that rendering the samples page before and after (for
both apps and extensions) is all-but the same.

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/reference_resolver.py (right):

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver.py:36: def
_MakeKey(namespace, ref):
caching the title here is silly. the expensive computation is resolving links,
not assigning a title to them.

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver.py:153: if title is not
None:
this is all-but-identical code. rietveld is just being silly.

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/samples_data_source.py:66: js_file =
re.compile(r'\b%s\b' % var_name).sub(value, js_file)
this is the most important change.

[Jeffrey Yasskin, 2013-11-01 18:00:24]

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/reference_resolver.py (right):

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver.py:152: link =
deepcopy(link)
Does the object store actually return the same object to multiple callers?

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/samples_data_source.py:65: for var_name,
value in assigned_vars.iteritems():
You might be able to make this loop more efficient by joining all the
|var_name|s into a single regex, and then using a function argument to re.sub()
to pick the right value to substitute. Or the function calls might make it less
efficient than copying the string N times. But given the size of the samples,
probably the function would be faster.

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/samples_data_source.py:66: js_file =
re.compile(r'\b%s\b' % var_name).sub(value, js_file)
Instead of re.compile(variable_string).sub(...), use re.sub(variable_string,
value, js_file)
http://docs.python.org/2/library/re.html#re.sub

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/samples_data_source.py:66: js_file =
re.compile(r'\b%s\b' % var_name).sub(value, js_file)
You probably want re.escape(var_name) in case it contains '\' or other special
characters. Otherwise assert that it's purely alphanumeric, or guarantee it with
something stronger than [^\s]+ in the assign_vars re above.

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/samples_data_source.py:67: return
calls.union(re.findall(chrome_regex, js_file))
Wait a second, you're just trying to find the APIs used in the file, so why go
to the expense of replacing things at all? You can use re.finditer to find each
use of an API that's been assigned into a variable, and then
re.compile(r'\.[a-zA-Z0-9\.]+').match(js_file, var_match.end()) to find the
suffix. (share the suffix re across iterations, of course.)

[not at google - send to devlin, 2013-11-04 21:51:00]

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/reference_resolver.py (right):

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver.py:152: link =
deepcopy(link)
On 2013/11/01 18:00:24, Jeffrey Yasskin wrote:
> Does the object store actually return the same object to multiple callers?

The in-memory one does. Copy-if-you-need-to[-mutate-it].

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://codereview.chromium.org/55913003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/samples_data_source.py:67: return
calls.union(re.findall(chrome_regex, js_file))
On 2013/11/01 18:00:24, Jeffrey Yasskin wrote:
> Wait a second, you're just trying to find the APIs used in the file, so why go
> to the expense of replacing things at all? You can use re.finditer to find
each
> use of an API that's been assigned into a variable, and then
> re.compile(r'\.[a-zA-Z0-9\.]+').match(js_file, var_match.end()) to find the
> suffix. (share the suffix re across iterations, of course.)

I got a little bit lost in your comments there + the deeply confusing re
library, but, how does this look? I re-wrote it rather than trying to patch it.

I've removed the actual replacement and just done plain searching; although now
it's O(T + T*V) where T is the length of the text and V is the number of
variable matches. That's probably fine since there will most likely be V=0 which
should offset the cost of doing text replacement; I presume.

[Jeffrey Yasskin, 2013-11-04 22:50:30]

lgtm

https://codereview.chromium.org/55913003/diff/50001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://codereview.chromium.org/55913003/diff/50001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/samples_data_source.py:68: for var_match
in re.finditer(r'\b%s\.([\w.]+)\b' % re.escape(var_name),
You could probably speed this up a bit (remove the |V| factor) by replacing the
first %s with '('+'|'.join(map(re.escape), all_var_names)+')', and then looking
up the first group in the dictionary, but what you have is probably still faster
than copying js_file a bunch of times.

https://codereview.chromium.org/55913003/diff/50001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/samples_data_source.py:70: api_suffix, =
var_match.groups()
You could replace the unpacking with var_match.group(1).

[not at google - send to devlin, 2013-11-04 23:39:41]

https://codereview.chromium.org/55913003/diff/50001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://codereview.chromium.org/55913003/diff/50001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/samples_data_source.py:68: for var_match
in re.finditer(r'\b%s\.([\w.]+)\b' % re.escape(var_name),
On 2013/11/04 22:50:30, Jeffrey Yasskin wrote:
> You could probably speed this up a bit (remove the |V| factor) by replacing
the
> first %s with '('+'|'.join(map(re.escape), all_var_names)+')', and then
looking
> up the first group in the dictionary, but what you have is probably still
faster
> than copying js_file a bunch of times.

Yeah. Constructing regex's like this does scare me. I'm not confident in how the
engines are implemented.

Highly likely it wouldn't matter; but still, like you say, this works well
enough.

https://codereview.chromium.org/55913003/diff/50001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/samples_data_source.py:70: api_suffix, =
var_match.groups()
On 2013/11/04 22:50:30, Jeffrey Yasskin wrote:
> You could replace the unpacking with var_match.group(1).

I kind of like way that it matches line 67 here; and the group-1 thing creeps me
out. Even if the trailing comma looks like a typo.

[commit-bot: I haz the power, 2013-11-05 00:24:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/55913003/50001

[commit-bot: I haz the power, 2013-11-05 00:24:38]

Failed to apply patch for chrome/common/extensions/docs/server2/app.yaml:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/common/extensions/docs/server2/app.yaml
  Hunk #1 FAILED at 1.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/common/extensions/docs/server2/app.yaml.rej

Patch:       chrome/common/extensions/docs/server2/app.yaml
Index: chrome/common/extensions/docs/server2/app.yaml
diff --git a/chrome/common/extensions/docs/server2/app.yaml
b/chrome/common/extensions/docs/server2/app.yaml
index
b7282106d0659aa68f17a49730505371d0542eb8..db6b7905d89d904e509e85e6366117fe49fc26aa
100644
--- a/chrome/common/extensions/docs/server2/app.yaml
+++ b/chrome/common/extensions/docs/server2/app.yaml
@@ -1,5 +1,5 @@
 application: chrome-apps-doc
-version: 2-34-1
+version: 2-35-0
 runtime: python27
 api_version: 1
 threadsafe: false

[not at google - send to devlin, 2013-11-05 00:53:41]

Committed patchset #3 manually as r232856.