Skip to content
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

Enable creation of unknown record types #2171

Merged
merged 5 commits into from Feb 21, 2024
Merged

Enable creation of unknown record types #2171

merged 5 commits into from Feb 21, 2024

Conversation

dholms
Copy link
Collaborator

@dholms dholms commented Feb 12, 2024

Enables the creation/update of records of unknown lexicon.

By default, a write is still "validated" which requires the existence of the lexicon. However, if the request to the pds is set to validated: false, then the write will be let through. This is higher friction than we want it to be long term, however we don't have any form of dynamic schema resolution yet and I think the extra bit of friction is worth it such that the lack of guardrails is better surfaced to developers.

We generically walk the record to find and associate any blobrefs, even for records that we do not know the schema for.

Some other generic behavior is enforced as well:

  • rkey syntax
  • record $type matching collection name
  • createdAt adheres to the proper date format
  • ensuring that blob-refs are not in the legacy format
  • generally being able to encode/decode from JSON <> Lex <> CBOR

path: string[] = [],
layer = 0,
): FoundBlobRef[] => {
if (layer > 10) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a gut check that 10 layers is enough for e.g. the images within a record-with-media (counting in my head I suppose it's probably fine, close to 5?). It just feels a little conservative, I'd be down to increase this a bit either way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup fair point, I can bump it up 👍

Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💎

@bnewbold
Copy link
Collaborator

FYI, I have this parallel PR with some additional data check test cases:
https://github.com/bluesky-social/atproto/pull/2182/files

It checks for things like $link and $bytes objects having the correct data types and no extra fields. Your round-trip tests may already catch these.

Should we consider preventing updates to app.bsky.feed.post, specifically, for now? I think there is already a loophole with delete/create, so maybe that ship has sailed. We just don't have super robust handling of this yet and it could break user expectations until there is more robust client support.

@dholms dholms merged commit 6dfc899 into main Feb 21, 2024
10 checks passed
@dholms dholms deleted the 3p-lexicons branch February 21, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants