-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
views: signposting: fix: fallback to level 2 linkset if collections size too big to control link header size #1966
base: master
Are you sure you want to change the base?
Conversation
ptamarit
commented
Feb 24, 2025
•
edited
Loading
edited
- Bug fixes FAIR signposting level 1 support (HTTP Link headers & link rel item) invenio-app-rdm#2937
- On a deployed environment, causing nginx to throw a generic 502 Bad Gateway.
- Fails on deployed setups with nginx since there is a limit on the size of the HTTP headers
invenio_rdm_records/resources/serializers/signposting/schema.py
Outdated
Show resolved
Hide resolved
b5776a4
to
aa0c5f2
Compare
""" | ||
for key in ["author", "item", "license"]: | ||
if key in data: | ||
data[key] = data[key][:5] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of an arbitrary limit:
- For
author
, we only include the ORCID URL, so the length is under control - For
item
, filenames have a limit of 255 characters, so we should test on a deployed environment with filenames of this length - For
license
, custom ones can have very long URLs (I tried with 5000 characters), since the marshmallow URL validator does not check for length, so this might be a big problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine in this case to have the "magic" 5, since passing config to the serializers from the app is quite cumbersome (and breaks the assumption that serializers don't depend on Flask application context).
…ize too big to control link header size
aa0c5f2
to
9e95ed5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, and thanks for the nice tests :)
Another way of avoid this problem is to configure nginx to allow for bigger response headers with:
|