Page MenuHomeSoftware Heritage

Web API: /known/ input size limit
ClosedPublic

Authored by DanSeraf on Feb 12 2020, 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
Branch
known-endpoint-limit-input-size
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10621
Build 15893: Cypress tests for swh-web diffsJenkins
Build 15892: tox-on-jenkinsJenkins
Build 15891: arc lint + arc unit

Event Timeline

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

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.Feb 12 2020, 3:33 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.Feb 12 2020, 3:41 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.EditedFeb 12 2020, 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
46

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.

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

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

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

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

limit is set inside the endpoint

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