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

Ignore extra fields instead of forbidding #176

Closed
wants to merge 3 commits into from

Conversation

tonycsoka
Copy link

The spec for lexicon, indicates that extra fields should be ignored.

Unexpected fields in data which otherwise conforms to the Lexicon should be ignored. When doing schema validation, they should be treated at worst as warnings. This is necessary to allow evolution of the schema by the controlling authority, and to be robust in the case of out-of-date Lexicons.

ATProto Lexicon#authority-and-control

Although this may change in future

The validation rules for unexpected additional fields may change. For example, a mechanism for Lexicons to indicate that the schema is "closed" and unexpected fields are not allowed, or a convention around field name prefixes (x-) to indicate unofficial extension.

ATProto Lexicon#possible-future-changes

At the moment, extra fields are causing an error, due to Pydantic being configured to 'forbid' these.
Changing the base mode ConfigDict with extra='ignore' and keeping strict=True, will keep validation, but allow extra fields.

Change model to ignore, this matches lexicon specs
@MarshalX
Copy link
Owner

Thank you for PR! Unit tests and documentation should be updated with this PR too

@tonycsoka
Copy link
Author

Tests fixed, what needs to be done with the docs?

@MarshalX
Copy link
Owner

MarshalX commented Oct 10, 2023

General question. How users can now access to extra fields? Previously it was easily possible with DotDict. Now tests doesn't verify that extra field value is "kek"

@tonycsoka
Copy link
Author

General question. How users can now access to extra fields? Previously it was easily possible with DotDict. Now tests doesn't verify that extra field value is "kek"

As in the first comment, and unless I'm reading it wrong, the specs suggest that extra fields should be ignored.

From ATProto Lexicon#authority-and-control

Unexpected fields in data which otherwise conforms to the Lexicon should be ignored. When doing schema validation, they should be treated at worst as warnings. This is necessary to allow evolution of the schema by the controlling authority, and to be robust in the case of out-of-date Lexicons.

@MarshalX
Copy link
Owner

In the original SDK (js one) data is not dropped as I know. You are still able to access it. But it's out of type annotations (type script's one). I want to provide a way to access this data too

We are a not writing client for bsky. We should not limit users to a strict lexicon when protocol allows extra fields. SDK provides API to create records with extra fields. SDK should keep providing a way to work with extra fields

There are many records that already uses extra fields (non official ones). And someone could be interesting working or analysing it

We need to choose the best way that will work perfectly for both cases

@tonycsoka
Copy link
Author

Fair enough, I'll think it over and drop this PR for now.

@tonycsoka tonycsoka closed this Oct 10, 2023
@MarshalX MarshalX mentioned this pull request Dec 14, 2023
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.

2 participants