Details

Reviewers

vlorentz
anlambert

Group Reviewers

Reviewers

Commits

rDWAPPS792f7263ffc6: input size limit for /known/ endpoint

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.Feb 12 2020, 3:17 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptFeb 12 2020, 3:17 PM

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

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/929/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/929/console

Harbormaster failed remote builds in B10574: Diff 9513!Feb 12 2020, 3:24 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.Feb 12 2020, 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.Feb 12 2020, 3:33 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/538/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/538/console

DanSeraf added inline comments.Feb 12 2020, 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.Feb 12 2020, 3:41 PM

vlorentz added inline comments.Feb 12 2020, 3:43 PM

swh/web/config.py
50 ↗	(On Diff #9513)	Yes, like you already do, it just doesn't need a config value

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.

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/931/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/931/console

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/540/ for more details.

DanSeraf added inline comments.Feb 12 2020, 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.Feb 12 2020, 4:11 PM

swh/web/config.py
50 ↗	(On Diff #9513)	Alright. But use a reasonable default value then (eg. 1000 or 10000)

rebase

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/932/ for more details.

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/541/ for more details.

Harbormaster completed remote builds in B10578: Diff 9517.Feb 12 2020, 4:53 PM

anlambert mentioned this in D2668: api/identifiers: Add /known/ endpoint documentation.Feb 12 2020, 6:02 PM

zack added a subscriber: zack.Feb 13 2020, 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