Remove _paginate_issue()

[Marc-Antoine Ruel]

Hi

I feel support ?offset=<number> should be deleted. We got harvested recently and the fact that offset can be used took a lot of datastore processing power from our instance.

So my take:

1. Add to def search() the option to sort in decreasing timestamp. Let's say it's "order=decreasing" here for simplicity.
2. Fix /search UI to be consistent.
3. Links "Open Issues", "Closed Issues" and "All Issues" should be updated to be redirected to /search?<relevant_query>.
4. def index() should redirect to /search?closed=3&order=decreasing. Determine: Handle old query args or don't bother?
5. def all() should redirect to /search?closed=1&order=decreasing. Determine: Handle old query args or don't bother?
6. def _paginate_issue() can then be deleted. Yay!

Any opinion?

M-A

[Guido van Rossum]

I feel dense -- I do not see how you would resolve viewing the rest of
the issues if the first page doesn't hold them all.

FWIW if you want to kill offset shouldn't you also kill limit? It can
also be abused.

> --
> You received this message because you are subscribed to the Google Groups
> "codereview-discuss" group.
> To post to this group, send email to coderevie...@googlegroups.com.
> To unsubscribe from this group, send email to
> codereview-disc...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/codereview-discuss?hl=en.

--
--Guido van Rossum (python.org/~guido)

[anatoly techtonik]

On Mon, Jan 30, 2012 at 8:45 PM, Marc-Antoine Ruel <mar...@chromium.org> wrote:

Hi

I feel support ?offset=<number> should be deleted. We got harvested recently and the fact that offset can be used took a lot of datastore processing power from our instance.

Is there any way to guard AppEngine apps from such attacks?

So my take:

1. Add to def search() the option to sort in decreasing timestamp. Let's say it's "order=decreasing" here for simplicity.
2. Fix /search UI to be consistent.
3. Links "Open Issues", "Closed Issues" and "All Issues" should be updated to be redirected to /search?<relevant_query>.
4. def index() should redirect to /search?closed=3&order=decreasing. Determine: Handle old query args or don't bother?
5. def all() should redirect to /search?closed=1&order=decreasing. Determine: Handle old query args or don't bother?
6. def _paginate_issue() can then be deleted. Yay!

Any opinion?

I'd start with porting to Python 2.7 http://code.google.com/p/rietveld/issues/detail?id=348
then to Django 1.3,
then make `closed` parameter intuitive http://code.google.com/p/rietveld/issues/detail?id=298
then try to make further enhancements.

In general making all() and index() to be redirects to unified and paginated search output seems ok, but it will also double amount of requests unless all menu links are replaced with /search URLs.
--
anatoly t.

[Marc-Antoine Ruel]

Le 30 janvier 2012 13:12, Guido van Rossum <gu...@python.org> a écrit :

I feel dense -- I do not see how you would resolve viewing the rest of
the issues if the first page doesn't hold them all.

FWIW if you want to kill offset shouldn't you also kill limit? It can
also be abused.

limit can't be abused in the sense that it's limited to 1000. For some odd reason, it seems that offset > 1000 isn't refused right away and instead caused the instance to spin for 60 seconds, on every request. The guy was eventually using offset over 100000. The online log dashboard isn't useful to know what happened earlier, I can only see last few hours.

The main difference between _paginate_issue() and _paginate_issues_with_cursor() is that a cursor is much more efficient and coherent than an offset.

Le 30 janvier 2012 13:26, anatoly techtonik <tech...@gmail.com> a écrit :

Is there any way to guard AppEngine apps from such attacks?

Yes, dos.yaml. I used it but it's manual.

There could be a middleware to throttle some kinds of heavy weight requests from a single IP (let's ignore ddos here) but even that is lower priority to me.

Le 30 janvier 2012 13:26, anatoly techtonik <tech...@gmail.com> a écrit :

I'd start with porting to Python 2.7 http://code.google.com/p/rietveld/issues/detail?id=348

then to Django 1.3,
then make `closed` parameter intuitive http://code.google.com/p/rietveld/issues/detail?id=298
then try to make further enhancements.

In general making all() and index() to be redirects to unified and paginated search output seems ok, but it will also double amount of requests unless all menu links are replaced with /search URLs.

Replacing the links was specified as step 3.

The rest is priorization.

M-A

[Guido van Rossum]

On Mon, Jan 30, 2012 at 11:18 AM, Marc-Antoine Ruel <mar...@chromium.org> wrote:
> Le 30 janvier 2012 13:12, Guido van Rossum <gu...@python.org> a écrit :
>
>> I feel dense -- I do not see how you would resolve viewing the rest of
>> the issues if the first page doesn't hold them all.
>>
>> FWIW if you want to kill offset shouldn't you also kill limit? It can
>> also be abused.
>
>
> limit can't be abused in the sense that it's limited to 1000. For some odd
> reason, it seems that offset > 1000 isn't refused right away and instead
> caused the instance to spin for 60 seconds, on every request. The guy was
> eventually using offset over 100000. The online log dashboard isn't useful
> to know what happened earlier, I can only see last few hours.

I guess he was just following all links in the UI?

We could replace the next/prev links with links based on cursors, then
there would be no cost to skip to the desired point in the query.

> The main difference between _paginate_issue() and
> _paginate_issues_with_cursor() is that a cursor is much more efficient and
> coherent than an offset.

Duh, so we already use cursors? Forget what I said then. :-)

[Marc-Antoine Ruel]

/all uses offsets, /search uses cursors. This results in mostly similar code. Hence my original proposition.

M-A 

[Guido van Rossum]

+1, then. :-)

--
--Guido van Rossum (python.org/~guido)

[anatoly techtonik]

On Mon, Jan 30, 2012 at 10:18 PM, Marc-Antoine Ruel <mar...@chromium.org> wrote:

Le 30 janvier 2012 13:12, Guido van Rossum <gu...@python.org> a écrit :

I feel dense -- I do not see how you would resolve viewing the rest of
the issues if the first page doesn't hold them all.

FWIW if you want to kill offset shouldn't you also kill limit? It can
also be abused.

limit can't be abused in the sense that it's limited to 1000. For some odd reason, it seems that offset > 1000 isn't refused right away and instead caused the instance to spin for 60 seconds, on every request. The guy was eventually using offset over 100000. The online log dashboard isn't useful to know what happened earlier, I can only see last few hours.

The main difference between _paginate_issue() and _paginate_issues_with_cursor() is that a cursor is much more efficient and coherent than an offset.

You didn't mention cursors in the first place, so that's ok for me. =) If I understand correctly, cursor just doesn't allow to go to 15th page directly, is that right?

 

Le 30 janvier 2012 13:26, anatoly techtonik <tech...@gmail.com> a écrit :

Is there any way to guard AppEngine apps from such attacks?

Yes, dos.yaml. I used it but it's manual.

There could be a middleware to throttle some kinds of heavy weight requests from a single IP (let's ignore ddos here) but even that is lower priority to me.

I wonder if such middleware should be provided by AppEngine to protect customers from attacks that directly empty their bank accounts? 

-- 

anatoly t.

[Marc-Antoine Ruel]

Le 31 janvier 2012 03:13, anatoly techtonik <tech...@gmail.com> a écrit :

On Mon, Jan 30, 2012 at 10:18 PM, Marc-Antoine Ruel <mar...@chromium.org> wrote:

The main difference between _paginate_issue() and _paginate_issues_with_cursor() is that a cursor is much more efficient and coherent than an offset.

You didn't mention cursors in the first place, so that's ok for me. =) If I understand correctly, cursor just doesn't allow to go to 15th page directly, is that right?

Exactly, you have to follow the pages one at a time and can't skip to a random offset. It's a minor annoyance for greater good.

 

Sorry I somehow took cursor usage as "common knowledge". :)

 

Anyway, I was just touching water, I'll take a look how messy it is to complete.

 

Le 30 janvier 2012 13:26, anatoly techtonik <tech...@gmail.com> a écrit :

Is there any way to guard AppEngine apps from such attacks?

Yes, dos.yaml. I used it but it's manual.

There could be a middleware to throttle some kinds of heavy weight requests from a single IP (let's ignore ddos here) but even that is lower priority to me.

I wonder if such middleware should be provided by AppEngine to protect customers from attacks that directly empty their bank accounts? 

I don't think it's AE's duty to do that. But I wouldn't be surprised if someone had already written something like that for django. Something that piggies back on appstats and detect 10 minutes average sudden increase or send IM on sudden error increases.

M-A