Page MenuHomeSoftware Heritage

Web API: /known/ input size limit
ClosedPublic

Authored by DanSeraf on Wed, Feb 12, 3:17 PM.

Details

Summary

The number of PID's the /known/ endpoint can receive is specified in swh.web.config.
If the limit is exceeded, an InputSizeExc is raised with the error code 413.

Related (T2276)

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

DanSeraf created this revision.Wed, Feb 12, 3:17 PM
DanSeraf edited the summary of this revision. (Show Details)Wed, Feb 12, 3:18 PM

Don't worry about the test failures, they are not related to your changes. I just restarted them.

vlorentz requested changes to this revision.Wed, Feb 12, 3:33 PM
vlorentz added inline comments.
swh/web/config.py
50 ↗(On Diff #9513)

You don't need to make this configurable; most API endpoints aren't. (eg. look for limit in swh/web/api/views/origin.py)

This revision now requires changes to proceed.Wed, Feb 12, 3:33 PM
DanSeraf added inline comments.Wed, Feb 12, 3:38 PM
swh/web/config.py
50 ↗(On Diff #9513)

So, should i limit the request directly inside the endpoint?

DanSeraf marked an inline comment as not done.Wed, Feb 12, 3:41 PM
vlorentz added inline comments.Wed, Feb 12, 3:43 PM
swh/web/config.py
50 ↗(On Diff #9513)

Yes, like you already do, it just doesn't need a config value

anlambert requested changes to this revision.EditedWed, Feb 12, 3:43 PM
anlambert added a subscriber: anlambert.

A new test should be added to check the 413 code is returned when the input pid list is too large..

swh/web/common/exc.py
47

LargePayloadExc seems a more generic name to me.

swh/web/config.py
50 ↗(On Diff #9513)

Adding this in configuration is not a shocker to me.

pids_list_max_size is also more explicit.

DanSeraf added inline comments.Wed, Feb 12, 4:09 PM
swh/web/config.py
50 ↗(On Diff #9513)

It would be hard to test without a config value, i can't get the limit from the endpoint

vlorentz added inline comments.Wed, Feb 12, 4:11 PM
swh/web/config.py
50 ↗(On Diff #9513)

Alright. But use a reasonable default value then (eg. 1000 or 10000)

zack added a subscriber: zack.Thu, Feb 13, 9:10 AM
zack added inline comments.
swh/web/config.py
50 ↗(On Diff #9513)

You don't need to make this configurable; most API endpoints aren't.

well, this isn't great, they should be configurable :-)

(but ack, I agree it is pointless to diverge and make only one of them configurable at this point)

making all endpoints configurable might be a good easy hack task to file

DanSeraf updated this revision to Diff 9526.Fri, Feb 14, 11:12 AM

limit is set inside the endpoint

vlorentz accepted this revision.Fri, Feb 14, 12:08 PM
anlambert accepted this revision.Fri, Feb 14, 4:10 PM
This revision is now accepted and ready to land.Fri, Feb 14, 4:10 PM
This revision was landed with ongoing or failed builds.Mon, Feb 17, 11:54 AM
This revision was automatically updated to reflect the committed changes.