In /graph_json, check internal_only on Test entity and skip test for all Row entities.

In /graph_json, check internal_only on Test entity and skip test for all Row entities.

This removes the need for many duplicate internal_only indexes.

BUG=catapult:#1863

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/58d01d4a8c8c0ce0806e0b3cc7e4e1d3a7ee2b6d

[sullivan, 2015-12-22 19:28:34]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-22 19:28:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/1

[commit-bot: I haz the power, 2015-12-22 19:32:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-22 19:32:47]

Dry run: Try jobs failed on following builders:
  Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)

[sullivan, 2015-12-22 19:35:28]

sullivan@chromium.org changed reviewers:
+ qyearsley@chromium.org

[sullivan, 2015-12-22 19:35:29]

Quinten, what do you think about this approach to reducing index sizes?

I still have to finish the unit tests, but wanted feedback on the approach. This
brings the graphs back for logged out users. Example:
https://dev-sullivan-0f5100f4-dot-chromeperf.appspot.com/report?sid=f08311400...

[qyearsley, 2015-12-22 19:45:14]

I like this idea :-)

https://codereview.chromium.org/1549583002/diff/1/dashboard/dashboard/datasto...
File dashboard/dashboard/datastore_hooks.py (right):

https://codereview.chromium.org/1549583002/diff/1/dashboard/dashboard/datasto...
dashboard/dashboard/datastore_hooks.py:67: request.registry['single_privileged']
= True
In this function could we add a check for utils.IsInternalUser() here? This
implies doing a datastore get() for the "internal only domain" (google.com) and
checking the user email, but I don't *think* that this will add any latency.

[sullivan, 2015-12-22 19:47:22]

https://codereview.chromium.org/1549583002/diff/1/dashboard/dashboard/datasto...
File dashboard/dashboard/datastore_hooks.py (right):

https://codereview.chromium.org/1549583002/diff/1/dashboard/dashboard/datasto...
dashboard/dashboard/datastore_hooks.py:67: request.registry['single_privileged']
= True
On 2015/12/22 19:45:14, qyearsley wrote:
> In this function could we add a check for utils.IsInternalUser() here? This
> implies doing a datastore get() for the "internal only domain"
(http://google.com) and
> checking the user email, but I don't *think* that this will add any latency.

We actually want this to work for external users--the caller does the check to
see if it's internal user or internal data.

[sullivan, 2015-12-22 19:58:23]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-22 19:58:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/20001

[commit-bot: I haz the power, 2015-12-22 20:02:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-22 20:02:32]

Dry run: Try jobs failed on following builders:
  Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)

[qyearsley, 2015-12-22 20:17:24]

https://codereview.chromium.org/1549583002/diff/1/dashboard/dashboard/datasto...
File dashboard/dashboard/datastore_hooks.py (right):

https://codereview.chromium.org/1549583002/diff/1/dashboard/dashboard/datasto...
dashboard/dashboard/datastore_hooks.py:67: request.registry['single_privileged']
= True
On 2015/12/22 19:47:22, sullivan wrote:
> On 2015/12/22 19:45:14, qyearsley wrote:
> > In this function could we add a check for utils.IsInternalUser() here? This
> > implies doing a datastore get() for the "internal only domain"
> (http://google.com) and
> > checking the user email, but I don't *think* that this will add any latency.
> 
> We actually want this to work for external users--the caller does the check to
> see if it's internal user or internal data.

Oh, right -- this should have no effect for internal users, since the query will
not have the internal_only filter anyway.

So, we want this to be called for external users, but only after checking that
the data that we're about to fetch should in fact be public (e.g. Row entities
for a public Test).

https://codereview.chromium.org/1549583002/diff/20001/dashboard/dashboard/dat...
File dashboard/dashboard/datastore_hooks_test.py (right):

https://codereview.chromium.org/1549583002/diff/20001/dashboard/dashboard/dat...
dashboard/dashboard/datastore_hooks_test.py:184: self._CheckQueryResults(False)
Might be clearer to write

self._CheckQueryResults(include_internal=True)
self._CheckQueryResults(include_internal=False)

https://codereview.chromium.org/1549583002/diff/20001/dashboard/dashboard/gra...
File dashboard/dashboard/graph_json.py (right):

https://codereview.chromium.org/1549583002/diff/20001/dashboard/dashboard/gra...
dashboard/dashboard/graph_json.py:138: assert(not parent_test.internal_only or
utils.IsInternalUser())
What normally happens here when the test is internal only and the user is not
logged in? I guess that the test should never have been successfully fetched in
the first place since?

[sullivan, 2015-12-22 20:50:07]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[sullivan, 2015-12-22 20:50:12]

https://codereview.chromium.org/1549583002/diff/20001/dashboard/dashboard/dat...
File dashboard/dashboard/datastore_hooks_test.py (right):

https://codereview.chromium.org/1549583002/diff/20001/dashboard/dashboard/dat...
dashboard/dashboard/datastore_hooks_test.py:184: self._CheckQueryResults(False)
On 2015/12/22 20:17:24, qyearsley wrote:
> Might be clearer to write
> 
> self._CheckQueryResults(include_internal=True)
> self._CheckQueryResults(include_internal=False)

Done.

https://codereview.chromium.org/1549583002/diff/20001/dashboard/dashboard/gra...
File dashboard/dashboard/graph_json.py (right):

https://codereview.chromium.org/1549583002/diff/20001/dashboard/dashboard/gra...
dashboard/dashboard/graph_json.py:138: assert(not parent_test.internal_only or
utils.IsInternalUser())
On 2015/12/22 20:17:24, qyearsley wrote:
> What normally happens here when the test is internal only and the user is not
> logged in? I guess that the test should never have been successfully fetched
in
> the first place since?

Currently, no results are returned by datastore_hooks. Changing it to throw an
exception causes a red alert in the UI reminding the user to log in.

[commit-bot: I haz the power, 2015-12-22 20:50:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/40001

[commit-bot: I haz the power, 2015-12-22 21:00:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-22 21:00:40]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2015-12-22 21:23:23]

lgtm :-)

[sullivan, 2015-12-22 21:27:39]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-22 21:27:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/60001

[commit-bot: I haz the power, 2015-12-22 21:32:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-22 21:32:23]

Dry run: Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[sullivan, 2015-12-22 21:37:20]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-22 21:37:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/80001

[commit-bot: I haz the power, 2015-12-22 21:41:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-22 21:41:52]

Dry run: Try jobs failed on following builders:
  Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)

[sullivan, 2015-12-22 22:30:14]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-22 22:30:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/100001

[commit-bot: I haz the power, 2015-12-22 22:34:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-22 22:34:58]

Dry run: Try jobs failed on following builders:
  Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...)

[sullivan, 2015-12-22 22:36:13]

Quinten, I updated this so that all user-facing pages that query rows use the
new API, and all admin/cron pages use the SetPrivilegedRequest API, so that we
should be able to delete all indexes on internal_only. Still one test failing,
but I don't expect major changes for the fix.

PTAL. Also please review Patch Set 6, I will be uploading others because these
tests pass for me locally.

[sullivan, 2015-12-22 22:58:36]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-22 22:58:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/120001

[commit-bot: I haz the power, 2015-12-22 23:01:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-22 23:01:29]

Dry run: Try jobs failed on following builders:
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

[sullivan, 2015-12-22 23:06:53]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-22 23:07:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/140001

[commit-bot: I haz the power, 2015-12-22 23:10:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-22 23:10:05]

Dry run: Try jobs failed on following builders:
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

[sullivan, 2015-12-22 23:18:10]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-22 23:18:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/130015
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/130015

[commit-bot: I haz the power, 2015-12-22 23:21:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-22 23:21:04]

Dry run: Try jobs failed on following builders:
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

[sullivan, 2015-12-22 23:30:05]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-22 23:30:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/170001

[commit-bot: I haz the power, 2015-12-22 23:34:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-22 23:34:14]

Dry run: Try jobs failed on following builders:
  Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)

[sullivan, 2015-12-22 23:36:35]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-22 23:36:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/190001

[sullivan, 2015-12-22 23:39:50]

Okay, this is ready for full review.

[commit-bot: I haz the power, 2015-12-22 23:47:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-22 23:47:25]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2015-12-28 22:50:57]

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/da...
File dashboard/dashboard/datastore_hooks.py (right):

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/da...
dashboard/dashboard/datastore_hooks.py:72: """
Closing """ can go on previous line

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/da...
File dashboard/dashboard/datastore_hooks_test.py (right):

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/da...
dashboard/dashboard/datastore_hooks_test.py:187: graph_data.Row.value ==
20).fetch()
Why `ndb.OR(graph_data.Row.revision < 20, graph_data.Row.revision > 70)`

and not just `graph_data.Row.revision < 20`?

It looks like the ndb.OR was used above in another test method, but I'm not sure
why. Does the test also pass if you change this line (and the one below) to the
following?

  `rows = graph_data.Row.query(graph_data.Row.revision == 10).fetch()`

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/de...
File dashboard/dashboard/debug_alert.py (right):

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/de...
dashboard/dashboard/debug_alert.py:244: test_key: A Test entity key to fetch Row
entities for.
This line should be updated.

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/gr...
File dashboard/dashboard/graph_csv.py (right):

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/gr...
dashboard/dashboard/graph_csv.py:45: assert(utils.IsInternalUser or not
test.internal_only)
utils.IsInternalUser -> utils.IsInternalUser()

Possible alternate phrasing:
  assert not (test.internal_only and not utils.IsInternalUser())

(I find both of these approximately the same in terms of difficulty to
understand.)

Either way, it should be consistent with the assertion in graph_json.

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/gr...
File dashboard/dashboard/graph_revisions.py (right):

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/gr...
dashboard/dashboard/graph_revisions.py:83: return
Should we return [] here? Or explicitly return None and mention this in the
Returns part of the docstring above?

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/te...
File dashboard/dashboard/testing_common.py (right):

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/te...
dashboard/dashboard/testing_common.py:28: """Fake Request object which can be
used by datastore_hooks mocks."""
Could add a blank line before the method definition below

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/te...
dashboard/dashboard/testing_common.py:121: def PatchDatastoreHooksRequest(self,
remote_addr=None):
Could add a docstring, if you think it makes it clearer.

As far as I understand this patches the request objects gotten by the handler in
the test method in which it applies, so that a IP address can be set for the
request -- but it's not necessarily related to datastore hooks so maybe renaming
it as PatchRequestObject or something like that would also make sense?

[sullivan, 2015-12-29 03:36:03]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-29 03:36:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/210001

[sullivan, 2015-12-29 03:36:07]

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/da...
File dashboard/dashboard/datastore_hooks.py (right):

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/da...
dashboard/dashboard/datastore_hooks.py:72: """
On 2015/12/28 22:50:56, qyearsley wrote:
> Closing """ can go on previous line

Done.

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/da...
File dashboard/dashboard/datastore_hooks_test.py (right):

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/da...
dashboard/dashboard/datastore_hooks_test.py:187: graph_data.Row.value ==
20).fetch()
On 2015/12/28 22:50:56, qyearsley wrote:
> Why `ndb.OR(graph_data.Row.revision < 20, graph_data.Row.revision > 70)`
> 
> and not just `graph_data.Row.revision < 20`?
> 
> It looks like the ndb.OR was used above in another test method, but I'm not
sure
> why. Does the test also pass if you change this line (and the one below) to
the
> following?
> 
>   `rows = graph_data.Row.query(graph_data.Row.revision == 10).fetch()`

I modified this test to the simpler query you suggested.

I think the original test is trying to check that the hooks work properly even
if there are multiple conditions in the query. Not needed here.

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/de...
File dashboard/dashboard/debug_alert.py (right):

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/de...
dashboard/dashboard/debug_alert.py:244: test_key: A Test entity key to fetch Row
entities for.
On 2015/12/28 22:50:56, qyearsley wrote:
> This line should be updated.

Done.

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/gr...
File dashboard/dashboard/graph_csv.py (right):

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/gr...
dashboard/dashboard/graph_csv.py:45: assert(utils.IsInternalUser or not
test.internal_only)
On 2015/12/28 22:50:56, qyearsley wrote:
> utils.IsInternalUser -> utils.IsInternalUser()
> 
> Possible alternate phrasing:
>   assert not (test.internal_only and not utils.IsInternalUser())
> 
> (I find both of these approximately the same in terms of difficulty to
> understand.)
> 
> Either way, it should be consistent with the assertion in graph_json.

Done.

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/gr...
File dashboard/dashboard/graph_revisions.py (right):

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/gr...
dashboard/dashboard/graph_revisions.py:83: return
On 2015/12/28 22:50:56, qyearsley wrote:
> Should we return [] here? Or explicitly return None and mention this in the
> Returns part of the docstring above?

I changed it to []

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/te...
File dashboard/dashboard/testing_common.py (right):

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/te...
dashboard/dashboard/testing_common.py:28: """Fake Request object which can be
used by datastore_hooks mocks."""
On 2015/12/28 22:50:56, qyearsley wrote:
> Could add a blank line before the method definition below

Done.

https://codereview.chromium.org/1549583002/diff/190001/dashboard/dashboard/te...
dashboard/dashboard/testing_common.py:121: def PatchDatastoreHooksRequest(self,
remote_addr=None):
On 2015/12/28 22:50:56, qyearsley wrote:
> Could add a docstring, if you think it makes it clearer.
> 
> As far as I understand this patches the request objects gotten by the handler
in
> the test method in which it applies, so that a IP address can be set for the
> request -- but it's not necessarily related to datastore hooks so maybe
renaming
> it as PatchRequestObject or something like that would also make sense?

Done.

[commit-bot: I haz the power, 2015-12-29 03:40:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-29 03:40:23]

Dry run: Try jobs failed on following builders:
  Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)

[sullivan, 2015-12-29 03:58:29]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-29 03:58:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/230001

[commit-bot: I haz the power, 2015-12-29 04:10:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-29 04:10:03]

Dry run: This issue passed the CQ dry run.

[sullivan, 2015-12-29 21:15:34]

The CQ bit was checked by sullivan@chromium.org

[sullivan, 2015-12-29 21:15:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
qyearsley@chromium.org
Link to the patchset: https://codereview.chromium.org/1549583002/#ps230001
(title: "Fixed assertions")

[commit-bot: I haz the power, 2015-12-29 21:15:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549583002/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549583002/230001

[commit-bot: I haz the power, 2015-12-29 21:16:52]

Description was changed from

==========
In /graph_json, check internal_only on Test entity and skip test for all Row
entities.

This removes the need for many duplicate internal_only indexes.

BUG=catapult:#1863
==========

to

==========
In /graph_json, check internal_only on Test entity and skip test for all Row
entities.

This removes the need for many duplicate internal_only indexes.

BUG=catapult:#1863

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2015-12-29 21:16:54]

Committed patchset #13 (id:230001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...