lexicons: more string limits #1994
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Runs the risk of invalidating some existing records but I dont know how many without, you know, scanning the whole network. I'd lean towards yoloing it. |
|
The limits look fine to me btw |
The motivation is to have *some* size limit on every string in post records, to maximize interoperation. For example, we currently have a CBOR library rejecting some records because of too-long strings. We don't want to limit the ability of folks to be very descriptive in alt text, specifically, so chose what seems to be a very large limit. If this is not large enough, based on feedback, we can bump it even higher. As context this is the largest string length limit in all of our lexicons.
This mostly results in checks against the string being empty, or unlimited size.
bnewbold
force-pushed
the
bnewbold/lex-post-string-limits
branch
from
eddda66 to
5a2f140
Compare
|
Updated PR with codegen and changeset. My current disposition is to merge, and some small dev comms. |
Closed
estrattonbailey
added a commit
that referenced
this pull request
Jan 5, 2024
…andling * origin/main: Revert "lexicons: more string limits (#1994)"
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The most sensitive thing here is putting a (very long!) length limit on alt text. I believe this is long enough to not cause accessibility issues, but can update if needed.
The motivation is that an issue with very long embed image descriptions came up that has been causing records to be discarded by golang code. We'll separately work around that by bumping limits in the CBOR library (cborgen), but I think communicating some reasonable limit on these fields is helpful. This isn't intended to be antagonistic; the current problematic records seem to have huge values by mistake not intentionally.
While I was at it I did an informal review of all the "string" fields in records (I might have missed a couple, this was an informal scan).
These are technically violations of Lexicon stability, so need a bunch of review and buy-in on these. I'm not strident about these changes, but I think they are worth re-visiting, and this is probably our last chance to push them through.
The specific values are somewhat arbitrary. I chose "large" ones to minimize the change in behavior (given that there are no limits right now).
Haven't run codegen, would do that just before merging.
cc: @whyrusleeping w/r/t embed description length